From 3879d573d570d0654f9a561b32151b64533faea0 Mon Sep 17 00:00:00 2001
From: PixelyIon <pixelyion@protonmail.com>
Date: Tue, 14 Sep 2021 21:15:23 +0530
Subject: [PATCH] Fix Command Buffer Allocation & `FenceCycle`

This commit fixes a major issue with command buffer allocation which would result in only being able to utilize a command buffer slot on the 2nd attempt to use it after it's freed, this would lead to a significantly larger amount of command buffers being created than necessary. It also fixes an issue with the command buffers not being reset after they were utilized which results in UB eventually.

Another issue was fixed with `FenceCycle` where all dependencies are only destroyed on destruction of the `FenceCycle` itself rather than the function where the `VkFence` was found to be signalled.
---
 .../main/cpp/skyline/gpu/command_scheduler.cpp   |  6 ++++--
 app/src/main/cpp/skyline/gpu/command_scheduler.h | 16 +++++++++++++++-
 app/src/main/cpp/skyline/gpu/fence_cycle.h       |  9 +++++----
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/app/src/main/cpp/skyline/gpu/command_scheduler.cpp b/app/src/main/cpp/skyline/gpu/command_scheduler.cpp
index 809f8296..10dacb02 100644
--- a/app/src/main/cpp/skyline/gpu/command_scheduler.cpp
+++ b/app/src/main/cpp/skyline/gpu/command_scheduler.cpp
@@ -8,8 +8,9 @@ namespace skyline::gpu {
     CommandScheduler::CommandBufferSlot::CommandBufferSlot(vk::raii::Device &device, vk::CommandBuffer commandBuffer, vk::raii::CommandPool &pool) : device(device), commandBuffer(device, commandBuffer, pool), fence(device, vk::FenceCreateInfo{}), cycle(std::make_shared<FenceCycle>(device, *fence)) {}
 
     bool CommandScheduler::CommandBufferSlot::AllocateIfFree(CommandScheduler::CommandBufferSlot &slot) {
-        if (slot.active.test_and_set(std::memory_order_acq_rel)) {
+        if (!slot.active.test_and_set(std::memory_order_acq_rel)) {
             if (slot.cycle->Poll()) {
+                slot.commandBuffer.reset();
                 slot.cycle = std::make_shared<FenceCycle>(slot.device, *slot.fence);
                 return true;
             } else {
@@ -25,11 +26,12 @@ namespace skyline::gpu {
     }) {}
 
     CommandScheduler::ActiveCommandBuffer CommandScheduler::AllocateCommandBuffer() {
+        std::scoped_lock lock(mutex);
         auto slot{std::find_if(commandBuffers.begin(), commandBuffers.end(), CommandBufferSlot::AllocateIfFree)};
+        auto slotId{std::distance(commandBuffers.begin(), slot)};
         if (slot != commandBuffers.end())
             return ActiveCommandBuffer(*slot);
 
-        std::scoped_lock lock(mutex);
         vk::CommandBuffer commandBuffer;
         vk::CommandBufferAllocateInfo commandBufferAllocateInfo{
             .commandPool = *vkCommandPool,
diff --git a/app/src/main/cpp/skyline/gpu/command_scheduler.h b/app/src/main/cpp/skyline/gpu/command_scheduler.h
index 2b0f6b28..de24f675 100644
--- a/app/src/main/cpp/skyline/gpu/command_scheduler.h
+++ b/app/src/main/cpp/skyline/gpu/command_scheduler.h
@@ -41,7 +41,7 @@ namespace skyline::gpu {
             constexpr ActiveCommandBuffer(CommandBufferSlot &slot) : slot(slot) {}
 
             ~ActiveCommandBuffer() {
-                slot.active.clear();
+                slot.active.clear(std::memory_order_release);
             }
 
             vk::Fence GetFence() {
@@ -93,5 +93,19 @@ namespace skyline::gpu {
             SubmitCommandBuffer(*commandBuffer, commandBuffer.GetFence());
             return commandBuffer.GetFenceCycle();
         }
+
+        /**
+         * @note Same as Submit but with FenceCycle as an argument rather than return value
+         */
+        template<typename RecordFunction>
+        void SubmitWithCycle(RecordFunction recordFunction) {
+            auto commandBuffer{AllocateCommandBuffer()};
+            commandBuffer->begin(vk::CommandBufferBeginInfo{
+                .flags = vk::CommandBufferUsageFlagBits::eOneTimeSubmit,
+            });
+            recordFunction(*commandBuffer, commandBuffer.GetFenceCycle());
+            commandBuffer->end();
+            SubmitCommandBuffer(*commandBuffer, commandBuffer.GetFence());
+        }
     };
 }
diff --git a/app/src/main/cpp/skyline/gpu/fence_cycle.h b/app/src/main/cpp/skyline/gpu/fence_cycle.h
index c5c054a5..adc40fed 100644
--- a/app/src/main/cpp/skyline/gpu/fence_cycle.h
+++ b/app/src/main/cpp/skyline/gpu/fence_cycle.h
@@ -60,7 +60,7 @@ namespace skyline::gpu {
             if (signalled.test(std::memory_order_consume))
                 return;
             while (device.waitForFences(fence, false, std::numeric_limits<u64>::max()) != vk::Result::eSuccess);
-            if (signalled.test_and_set(std::memory_order_release))
+            if (!signalled.test_and_set(std::memory_order_release))
                 DestroyDependencies();
         }
 
@@ -72,7 +72,7 @@ namespace skyline::gpu {
             if (signalled.test(std::memory_order_consume))
                 return true;
             if (device.waitForFences(fence, false, timeout.count()) == vk::Result::eSuccess) {
-                if (signalled.test_and_set(std::memory_order_release))
+                if (!signalled.test_and_set(std::memory_order_release))
                     DestroyDependencies();
                 return true;
             } else {
@@ -86,8 +86,9 @@ namespace skyline::gpu {
         bool Poll() {
             if (signalled.test(std::memory_order_consume))
                 return true;
-            if ((*device).getFenceStatus(fence, *device.getDispatcher()) == vk::Result::eSuccess) {
-                if (signalled.test_and_set(std::memory_order_release))
+            auto status{(*device).getFenceStatus(fence, *device.getDispatcher())};
+            if (status == vk::Result::eSuccess) {
+                if (!signalled.test_and_set(std::memory_order_release))
                     DestroyDependencies();
                 return true;
             } else {