Finally work out that this one assert didn't make sense, and make everything wait on a TS instead of a Fence

devshgraphicsprogramming · devshgraphicsprogramming · commit e2b78526b6f4 · 2024-02-26T16:10:57.000+01:00
diff --git a/include/nbl/video/CVulkanSwapchain.h b/include/nbl/video/CVulkanSwapchain.h
@@ -30,8 +30,9 @@ class CVulkanSwapchain final : public ISwapchain
             const uint32_t imageCount,
             core::smart_refctd_ptr<CVulkanSwapchain>&& oldSwapchain,
             const VkSwapchainKHR swapchain,
-            const VkSemaphore* const _adaptorSemaphores,
-            const VkFence* const _prePresentFences
+            const VkSemaphore* const _acquireAdaptorSemaphores,
+            const VkSemaphore* const _prePresentSemaphores,
+            const VkSemaphore* const _presentAdaptorSemaphores
         );
         ~CVulkanSwapchain();
 
@@ -45,8 +46,9 @@ class CVulkanSwapchain final : public ISwapchain
         const VkSwapchainKHR m_vkSwapchainKHR;
         VkImage m_images[ISwapchain::MaxImages];
         VkSemaphore m_acquireAdaptorSemaphores[ISwapchain::MaxImages];
-        VkFence m_prePresentFences[ISwapchain::MaxImages];
+        VkSemaphore m_prePresentSemaphores[ISwapchain::MaxImages];
         VkSemaphore m_presentAdaptorSemaphores[ISwapchain::MaxImages];
+        uint64_t m_perImageAcquireCount[ISwapchain::MaxImages] = { 0 };
         // nasty way to fight UB of the Vulkan spec
         bool m_needToWaitIdle = true;
 };
diff --git a/include/nbl/video/ISwapchain.h b/include/nbl/video/ISwapchain.h
@@ -361,7 +361,6 @@ class ISwapchain : public IBackendObject
                 case ACQUIRE_IMAGE_RESULT::SUCCESS: [[fallthrough]];
                 case ACQUIRE_IMAGE_RESULT::SUBOPTIMAL:
                     m_acquireCounter++;
-                    //assert(!unacquired(out_imgIx)); // should happen in the impl anyway
                     {
                         auto semaphoreArray = core::make_refctd_dynamic_array<core::smart_refctd_dynamic_array<void_refctd_ptr>>(info.signalSemaphores.size());
                         for (auto i=0ull; i<info.signalSemaphores.size(); i++)
@@ -495,7 +494,7 @@ class ISwapchain : public IBackendObject
         virtual core::smart_refctd_ptr<ISwapchain> recreate_impl(SSharedCreationParams&& params) = 0;
         
         // The user needs to hold onto the old swapchain by themselves as well, because without the `KHR_swapchain_maintenance1` extension:
-        // - Images cannot be "unacquired", so all already acquired images of a swapchain (even if its retired) must be presented
+        // - Images cannot be "de-acquired", so all already acquired images of a swapchain (even if its retired) must be presented
         // - No way to query that the presentation is finished, so we can't destroy a swapchain just because we issued a present on all previously acquired images
         // This means new swapchain should hold onto the old swapchain for `getImageCount()` acquires or until an acquisition error.
         // But we don't handle the acquisition error case because we just presume swapchain will be recreated and eventually there will be one with enough acquisitions.
diff --git a/src/nbl/video/CVulkanSwapchain.cpp b/src/nbl/video/CVulkanSwapchain.cpp
@@ -122,36 +122,33 @@ core::smart_refctd_ptr<CVulkanSwapchain> CVulkanSwapchain::create(core::smart_re
         return nullptr;
 
     //
-    VkSemaphore adaptorSemaphores[2*ISwapchain::MaxImages];
-    {
-        VkSemaphoreCreateInfo info = {VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO,nullptr};
-        info.flags = 0u;
-        for (auto i=0u; i<2*imageCount; i++)
-        if (vk.vkCreateSemaphore(vk_device,&info,nullptr,adaptorSemaphores+i)!=VK_SUCCESS)
-        {
-            // handle successful allocs before failure
-            for (auto j=0u; j<i; j++)
-                vk.vkDestroySemaphore(vk_device,adaptorSemaphores[j],nullptr);
-            return nullptr;
-        }
-    }
-    VkFence prePresentFences[ISwapchain::MaxImages];
+    const VkSemaphoreTypeCreateInfo timelineType = {
+        .sType = VK_STRUCTURE_TYPE_SEMAPHORE_TYPE_CREATE_INFO,
+        .pNext = nullptr,
+        .semaphoreType = VK_SEMAPHORE_TYPE_TIMELINE,
+        .initialValue = 0
+    };
+    const VkSemaphoreCreateInfo timelineInfo = {
+        .sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO,
+        .pNext = &timelineType,
+        .flags = 0
+    };
+    const VkSemaphoreCreateInfo adaptorInfo = {
+        .sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO,
+        .pNext = nullptr,
+        .flags = 0
+    };
+    VkSemaphore semaphores[ISwapchain::MaxImages];
+    for (auto i=0u; i<3*imageCount; i++)
+    if (vk.vkCreateSemaphore(vk_device,i<imageCount ? (&timelineInfo):(&adaptorInfo),nullptr,semaphores+i)!=VK_SUCCESS)
     {
-        VkFenceCreateInfo info = {VK_STRUCTURE_TYPE_FENCE_CREATE_INFO,nullptr};
-        info.flags = VK_FENCE_CREATE_SIGNALED_BIT; // makes life easier on the very first acquire and in the destructor
-        for (auto i=0u; i<imageCount; i++)
-        if (vk.vkCreateFence(vk_device,&info,nullptr,prePresentFences+i)!=VK_SUCCESS)
-        {
-            // handle successful allocs before failure
-            for (auto j=0u; j<ISwapchain::MaxImages; j++)
-                vk.vkDestroySemaphore(vk_device,adaptorSemaphores[j],nullptr);
-            for (auto j=0u; j<i; j++)
-                vk.vkDestroyFence(vk_device,prePresentFences[j],nullptr);
-            return nullptr;
-        }
+        // handle successful allocs before failure
+        for (auto j=0u; j<i; j++)
+            vk.vkDestroySemaphore(vk_device,semaphores[j],nullptr);
+        return nullptr;
     }
 
-    return core::smart_refctd_ptr<CVulkanSwapchain>(new CVulkanSwapchain(std::move(device),std::move(params),imageCount,std::move(oldSwapchain),vk_swapchain,adaptorSemaphores,prePresentFences),core::dont_grab);
+    return core::smart_refctd_ptr<CVulkanSwapchain>(new CVulkanSwapchain(std::move(device),std::move(params),imageCount,std::move(oldSwapchain),vk_swapchain,semaphores+imageCount,semaphores,semaphores+2*imageCount),core::dont_grab);
 }
 
 CVulkanSwapchain::CVulkanSwapchain(
@@ -160,8 +157,9 @@ CVulkanSwapchain::CVulkanSwapchain(
     const uint32_t imageCount,
     core::smart_refctd_ptr<CVulkanSwapchain>&& oldSwapchain,
     const VkSwapchainKHR swapchain,
-    const VkSemaphore* const _adaptorSemaphores,
-    const VkFence* const _prePresentFences
+    const VkSemaphore* const _acquireAdaptorSemaphores,
+    const VkSemaphore* const _prePresentSemaphores,
+    const VkSemaphore* const _presentAdaptorSemaphores
 ) : ISwapchain(std::move(logicalDevice),std::move(params),imageCount,std::move(oldSwapchain)),
     m_imgMemRequirements{.size=0,.memoryTypeBits=0x0u,.alignmentLog2=63,.prefersDedicatedAllocation=true,.requiresDedicatedAllocation=true}, m_vkSwapchainKHR(swapchain)
 {
@@ -176,9 +174,9 @@ CVulkanSwapchain::CVulkanSwapchain(
         assert(retval==VK_SUCCESS && dummy==getImageCount());
     }
 
-    std::copy_n(_adaptorSemaphores,imageCount,m_acquireAdaptorSemaphores);
-    std::copy_n(_prePresentFences,imageCount,m_prePresentFences);
-    std::copy_n(_adaptorSemaphores+imageCount,imageCount,m_presentAdaptorSemaphores);
+    std::copy_n(_acquireAdaptorSemaphores,imageCount,m_acquireAdaptorSemaphores);
+    std::copy_n(_prePresentSemaphores,imageCount,m_prePresentSemaphores);
+    std::copy_n(_presentAdaptorSemaphores,imageCount,m_presentAdaptorSemaphores);
 }
 
 CVulkanSwapchain::~CVulkanSwapchain()
@@ -192,15 +190,22 @@ CVulkanSwapchain::~CVulkanSwapchain()
     if (m_needToWaitIdle)
         vk.vkDeviceWaitIdle(vk_device);
 
+    const VkSemaphoreWaitInfo info = {
+        .sType = VK_STRUCTURE_TYPE_SEMAPHORE_WAIT_INFO,
+        .pNext = nullptr,
+        .flags = 0,
+        .semaphoreCount = getImageCount(),
+        .pSemaphores = m_prePresentSemaphores,
+        .pValues = m_perImageAcquireCount
+    };
     while (true) // if you find yourself spinning here forever, it might be because you didn't present an already acquired image
-    if (vk.vkWaitForFences(vk_device,getImageCount(),m_prePresentFences,true,~0ull)!=VK_TIMEOUT)
+    if (vk.vkWaitSemaphores(vk_device,&info,~0ull)!=VK_TIMEOUT)
         break;
-    for (auto i=0; i<getImageCount(); i++)
-        vk.vkDestroyFence(vk_device,m_prePresentFences[i],nullptr);
 
     for (auto i=0u; i<getImageCount(); i++)
     {
         vk.vkDestroySemaphore(vk_device,m_acquireAdaptorSemaphores[i],nullptr);
+        vk.vkDestroySemaphore(vk_device,m_prePresentSemaphores[i],nullptr);
         vk.vkDestroySemaphore(vk_device,m_presentAdaptorSemaphores[i],nullptr);
     }
 
@@ -210,8 +215,12 @@ CVulkanSwapchain::~CVulkanSwapchain()
 bool CVulkanSwapchain::unacquired(const uint8_t imageIndex) const
 {
     const CVulkanLogicalDevice* vulkanDevice = static_cast<const CVulkanLogicalDevice*>(getOriginDevice());
-    // Only lets us know if we're after a successful CPU call to acquire an image, and before the GPU finishes all work blocking the present!
-    return vulkanDevice->getFunctionTable()->vk.vkGetFenceStatus(vulkanDevice->getInternalObject(),m_prePresentFences[imageIndex])!=VK_NOT_READY;
+
+    uint64_t value=0;
+    // Only lets us know if we're not between after a successful CPU call to acquire an image, and before the GPU finishes all work blocking the present!
+    if (vulkanDevice->getFunctionTable()->vk.vkGetSemaphoreCounterValue(vulkanDevice->getInternalObject(),m_prePresentSemaphores[imageIndex],&value)!=VK_SUCCESS)
+        return true;
+    return value>=m_perImageAcquireCount[imageIndex];
 }
 
 auto CVulkanSwapchain::acquireNextImage_impl(const SAcquireInfo& info, uint32_t* const out_imgIx) -> ACQUIRE_IMAGE_RESULT
@@ -234,15 +243,22 @@ auto CVulkanSwapchain::acquireNextImage_impl(const SAcquireInfo& info, uint32_t*
         .deviceIndex = 0u // TODO: later obtain device index from swapchain
     };
 
-    bool suboptimal = false;
 
     auto& vk = vulkanDevice->getFunctionTable()->vk;
-    const VkDevice vk_device = vulkanDevice->getInternalObject();
+
+    // The presentation engine may not have finished reading from the image at the time it is acquired, so the application must use semaphore and/or fence
+    // to ensure that the image layout and contents are not modified until the presentation engine reads have completed.
+    // Once vkAcquireNextImageKHR successfully acquires an image, the semaphore signal operation referenced by semaphore is submitted for execution.
+    // If vkAcquireNextImageKHR does not successfully acquire an image, semaphore and fence are unaffected.
+    bool suboptimal = false;
     {
+        const VkDevice vk_device = vulkanDevice->getInternalObject();
+
         VkAcquireNextImageInfoKHR acquire = {VK_STRUCTURE_TYPE_ACQUIRE_NEXT_IMAGE_INFO_KHR,nullptr};
         acquire.swapchain = m_vkSwapchainKHR;
         acquire.timeout = info.timeout;
         acquire.semaphore = adaptorInfo.semaphore;
+        // Fence is redundant, we can Host-wait on the timeline semaphore latched by the binary adaptor semaphore instead
         acquire.fence = VK_NULL_HANDLE;
         acquire.deviceMask = 0x1u<<adaptorInfo.deviceIndex;
         switch (vk.vkAcquireNextImage2KHR(vk_device,&acquire,out_imgIx))
@@ -262,17 +278,15 @@ auto CVulkanSwapchain::acquireNextImage_impl(const SAcquireInfo& info, uint32_t*
                 return ACQUIRE_IMAGE_RESULT::_ERROR;
         }
     }
+
     const auto imgIx = *out_imgIx;
-    
-    // The previous present on the image MUST have had finished
-    // TODO: why do I crash here? maybe cause I don't wait on the acquired fence!
-    assert(unacquired(imgIx));
-    // Now put the fence into UNSIGNALLED state where it will stay until "just before" the present
-    {
-        const bool result = vk.vkResetFences(vk_device,1,m_prePresentFences+imgIx)==VK_SUCCESS;
-        // If this goes wrong, we are lost without KHR_swapchain_maintenance1 because there's no way to release acquired images without presenting them!
-        assert(result);
-    }
+    // There's a certain problem `assert(unacquired(imgIx))`, because:
+    // - we can't assert anything before the acquire, as:
+    //   + we don't know the `imgIx` before we issue the acquire call on the CPU
+    //   + it's okay for the present to not be done yet, because only when acquire signals the semaphore/fence it lets us know image is ready for use
+    // - after the acquire returns on the CPU the image may not be ready and present could still be reading from it, unknowable without an acquire fence
+    //   + we'd have to have a special fence just for acquire and `assert(unacquired || !acquireFenceReady)` before incrementing the acquire/TS counter
+    m_perImageAcquireCount[imgIx]++;
 
     core::vector<VkSemaphoreSubmitInfo> signalInfos(info.signalSemaphores.size(),{VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO,nullptr});
     for (auto i=0u; i<info.signalSemaphores.size(); i++)
@@ -304,19 +318,10 @@ auto CVulkanSwapchain::present_impl(const SPresentInfo& info) -> PRESENT_RESULT
     if (!vulkanQueue)
         return PRESENT_RESULT::FATAL_ERROR;
 
+    // We make the present wait on an empty submit so we can signal our image timeline semaphores that let us work out `unacquired()`
     auto& vk = vulkanDevice->getFunctionTable()->vk;
     const auto vk_device = vulkanDevice->getInternalObject();
 
-    // We make the present wait on an empty submit so we can signal our "not-acquired" fence
-    VkSemaphoreSubmitInfo adaptorInfo = {
-        .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO,
-        .pNext = nullptr,
-        .semaphore =  m_presentAdaptorSemaphores[info.imgIndex],
-        .value = 0,// value is ignored because the adaptors are binary
-        .stageMask = VK_PIPELINE_STAGE_2_NONE,
-        .deviceIndex = 0u // TODO: later
-    };
-
     // Optionally the submit ties timeline semaphore waits with itself
     core::vector<VkSemaphoreSubmitInfo> waitInfos(info.waitSemaphores.size(),{VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO,nullptr});
     for (auto i=0u; i<info.waitSemaphores.size(); i++)
@@ -329,29 +334,54 @@ auto CVulkanSwapchain::present_impl(const SPresentInfo& info) -> PRESENT_RESULT
         waitInfos[i].value = info.waitSemaphores[i].value;
         waitInfos[i].deviceIndex = 0u;
     }
+    
+    const auto prePresentSema = m_prePresentSemaphores[info.imgIndex];
+    auto& presentAdaptorSema = m_presentAdaptorSemaphores[info.imgIndex];
+    // First signal that we're done writing to the swapchain, then signal the binary sema for presentation
+    const VkSemaphoreSubmitInfo signalInfos[2] = {
+        {
+            .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO,
+            .pNext = nullptr,
+            .semaphore = prePresentSema,
+            .value = m_perImageAcquireCount[info.imgIndex],
+            .stageMask = VK_PIPELINE_STAGE_2_NONE,
+            .deviceIndex = 0u // TODO: later
+        },
+        {
+            .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SUBMIT_INFO,
+            .pNext = nullptr,
+            .semaphore = presentAdaptorSema,
+            .value = 0,// value is ignored because the adaptors are binary
+            .stageMask = VK_PIPELINE_STAGE_2_NONE,
+            .deviceIndex = 0u // TODO: later
+        }
+    };
 
     // Perform the empty submit
     {
         VkSubmitInfo2 submit = {VK_STRUCTURE_TYPE_SUBMIT_INFO_2,nullptr};
         submit.waitSemaphoreInfoCount = info.waitSemaphores.size();
         submit.pWaitSemaphoreInfos = waitInfos.data();
         submit.commandBufferInfoCount = 0u;
-        submit.signalSemaphoreInfoCount = 1u;
-        submit.pSignalSemaphoreInfos = &adaptorInfo;
-        if (vk.vkQueueSubmit2(vulkanQueue->getInternalObject(),1u,&submit,m_prePresentFences[info.imgIndex])!=VK_SUCCESS)
+        submit.signalSemaphoreInfoCount = 2u;
+        submit.pSignalSemaphoreInfos = signalInfos;
+        if (vk.vkQueueSubmit2(vulkanQueue->getInternalObject(),1u,&submit,VK_NULL_HANDLE)!=VK_SUCCESS)
         {
-            // need to recreate because can't signal a Fence from the Host
-            vk.vkDestroyFence(vk_device,m_prePresentFences[info.imgIndex],nullptr);
-            VkFenceCreateInfo createInfo = {VK_STRUCTURE_TYPE_FENCE_CREATE_INFO,nullptr};
-            createInfo.flags = VK_FENCE_CREATE_SIGNALED_BIT; // makes life easier on the very first acquire and in the destructor
-            vk.vkCreateFence(vk_device,&createInfo,nullptr,m_prePresentFences+info.imgIndex);
+            // Increment the semaphore on the Host to make sure we don't figure as "unacquired" forever
+            const VkSemaphoreSignalInfo signalInfo = {
+                .sType = VK_STRUCTURE_TYPE_SEMAPHORE_SIGNAL_INFO,
+                .pNext = nullptr,
+                .semaphore = prePresentSema,
+                .value = m_perImageAcquireCount[info.imgIndex]
+            };
+            vk.vkSignalSemaphore(vk_device,&signalInfo);
             return PRESENT_RESULT::FATAL_ERROR;
         }
     }
 
     VkPresentInfoKHR presentInfo = {VK_STRUCTURE_TYPE_PRESENT_INFO_KHR,nullptr};
     presentInfo.waitSemaphoreCount = 1u;
-    presentInfo.pWaitSemaphores = &adaptorInfo.semaphore;
+    presentInfo.pWaitSemaphores = &presentAdaptorSema;
     presentInfo.swapchainCount = 1u;
     presentInfo.pSwapchains = &m_vkSwapchainKHR;
     presentInfo.pImageIndices = &info.imgIndex;
@@ -367,11 +397,11 @@ auto CVulkanSwapchain::present_impl(const SPresentInfo& info) -> PRESENT_RESULT
         case VK_ERROR_OUT_OF_DEVICE_MEMORY:
             // as per the spec, only these "hard" errors don't result in the binary semaphore getting waited on
             vk.vkQueueWaitIdle(vulkanQueue->getInternalObject());
-            vk.vkDestroySemaphore(vk_device,adaptorInfo.semaphore,nullptr);
+            vk.vkDestroySemaphore(vk_device,presentAdaptorSema,nullptr);
             {
                 VkSemaphoreCreateInfo createInfo = {VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO,nullptr};
                 createInfo.flags = 0u;
-                vk.vkCreateSemaphore(vk_device,&createInfo,nullptr,m_presentAdaptorSemaphores+info.imgIndex);
+                vk.vkCreateSemaphore(vk_device,&createInfo,nullptr,&presentAdaptorSema);
             }
             [[fallthrough]];
         case VK_ERROR_DEVICE_LOST:
