From 850c0f4092af6995af58083d17584925c6c60d07 Mon Sep 17 00:00:00 2001
From: PixelyIon <pixelyion@protonmail.com>
Date: Sat, 6 Aug 2022 15:47:11 +0530
Subject: [PATCH] Make `Texture::SynchronizeGuest` Blocking

It was determined that `Texture::SynchronizeGuest`'s `TextureBufferCopy` had races that were exposed by the introduction of the cycle waiter thread, the synchronization did not take place under a locked context so the texture could be mutated at any point in addition to the destructor not being run during `FenceCycle::Wait` due to `shouldDestroy` being `false`.

This commit fixes the issue by making `SynchronizeGuest` entirely blocking as all usages of the function required blocking semantics regardless so it would be pointless to retain its async nature while solving any races that may arise from it being async.

Co-authored-by: Billy Laws <blaws05@gmail.com>
---
 .../main/cpp/skyline/gpu/texture/texture.cpp    | 17 ++++-------------
 app/src/main/cpp/skyline/gpu/texture/texture.h  | 14 +-------------
 2 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/app/src/main/cpp/skyline/gpu/texture/texture.cpp b/app/src/main/cpp/skyline/gpu/texture/texture.cpp
index f8cf7bfa..3ba08741 100644
--- a/app/src/main/cpp/skyline/gpu/texture/texture.cpp
+++ b/app/src/main/cpp/skyline/gpu/texture/texture.cpp
@@ -176,7 +176,6 @@ namespace skyline::gpu {
                 return false;
 
             texture->SynchronizeGuest(false, true); // We can skip trapping since the caller will do it
-            texture->WaitOnFence();
             return true;
         }, [weakThis] {
             TRACE_EVENT("gpu", "Texture::WriteTrap");
@@ -198,7 +197,6 @@ namespace skyline::gpu {
             if (!lock)
                 return false;
             texture->SynchronizeGuest(true, true); // We need to assume the texture is dirty since we don't know what the guest is writing
-            texture->WaitOnFence();
             return true;
         });
     }
@@ -455,13 +453,6 @@ namespace skyline::gpu {
         }
     }
 
-    Texture::TextureBufferCopy::TextureBufferCopy(std::shared_ptr<Texture> texture, std::shared_ptr<memory::StagingBuffer> stagingBuffer) : texture(std::move(texture)), stagingBuffer(std::move(stagingBuffer)) {}
-
-    Texture::TextureBufferCopy::~TextureBufferCopy() {
-        TRACE_EVENT("gpu", "Texture::TextureBufferCopy");
-        texture->CopyToGuest(stagingBuffer ? stagingBuffer->data() : std::get<memory::Image>(texture->backing).data());
-    }
-
     Texture::Texture(GPU &gpu, BackingType &&backing, texture::Dimensions dimensions, texture::Format format, vk::ImageLayout layout, vk::ImageTiling tiling, vk::ImageCreateFlags flags, vk::ImageUsageFlags usage, u32 levelCount, u32 layerCount, vk::SampleCountFlagBits sampleCount)
         : gpu(gpu),
           backing(std::move(backing)),
@@ -596,7 +587,6 @@ namespace skyline::gpu {
             gpu.state.nce->DeleteTrap(*trapHandle);
         if (alignedMirror.valid())
             munmap(alignedMirror.data(), alignedMirror.size());
-        WaitOnFence();
     }
 
     void Texture::lock() {
@@ -780,12 +770,13 @@ namespace skyline::gpu {
         if (tiling == vk::ImageTiling::eOptimal || !std::holds_alternative<memory::Image>(backing)) {
             auto stagingBuffer{gpu.memory.AllocateStagingBuffer(surfaceSize)};
 
+            WaitOnFence();
             auto lCycle{gpu.scheduler.Submit([&](vk::raii::CommandBuffer &commandBuffer) {
                 CopyIntoStagingBuffer(commandBuffer, stagingBuffer);
             })};
-            lCycle->AttachObject(std::make_shared<TextureBufferCopy>(shared_from_this(), stagingBuffer));
-            lCycle->ChainCycle(cycle);
-            cycle = lCycle;
+            lCycle->Wait(); // We block till the copy is complete
+
+            CopyToGuest(stagingBuffer->data());
         } else if (tiling == vk::ImageTiling::eLinear) {
             // We can optimize linear texture sync on a UMA by mapping the texture onto the CPU and copying directly from it rather than using a staging buffer
             WaitOnFence();
diff --git a/app/src/main/cpp/skyline/gpu/texture/texture.h b/app/src/main/cpp/skyline/gpu/texture/texture.h
index a0173f4e..1e6ab27e 100644
--- a/app/src/main/cpp/skyline/gpu/texture/texture.h
+++ b/app/src/main/cpp/skyline/gpu/texture/texture.h
@@ -416,18 +416,6 @@ namespace skyline::gpu {
          */
         void CopyToGuest(u8 *hostBuffer);
 
-        /**
-         * @brief A fence cycle dependency that copies the contents of a staging buffer or mapped image backing the texture to the guest texture
-         */
-        struct TextureBufferCopy {
-            std::shared_ptr<Texture> texture;
-            std::shared_ptr<memory::StagingBuffer> stagingBuffer;
-
-            TextureBufferCopy(std::shared_ptr<Texture> texture, std::shared_ptr<memory::StagingBuffer> stagingBuffer = {});
-
-            ~TextureBufferCopy();
-        };
-
         /**
          * @return A vector of all the buffer image copies that need to be done for every aspect of every level of every layer of the texture
          */
@@ -547,7 +535,7 @@ namespace skyline::gpu {
          * @brief Synchronizes the guest texture with the host texture after it has been modified
          * @param cpuDirty If true, the texture will be transitioned to being CpuDirty by this call
          * @param skipTrap If true, trapping/untrapping the guest mappings will be skipped and has to be handled by the caller
-         * @note This function is not blocking and the synchronization will not be complete until the associated fence is signalled, it can be waited on with WaitOnFence()
+         * @note This function is blocking and waiting on the fence is not required
          * @note The texture **must** be locked prior to calling this
          */
         void SynchronizeGuest(bool cpuDirty = false, bool skipTrap = false);