Merge branch 'frames_in_flight_fixes'

Author: Erfan-Ahmadi

include/nbl/video/CVulkanSwapchain.h

@@ -20,8 +20,14 @@ class CVulkanSwapchain final : public ISwapchain
 
         void setObjectDebugName(const char* label) const override;
 
-        inline const void* getNativeHandle() const {return &m_vkSwapchainKHR;}
+        inline const void* getNativeHandle() const override {return &m_vkSwapchainKHR;}
         inline VkSwapchainKHR getInternalObject() const {return m_vkSwapchainKHR;}
+        
+        // returns the maximum number of time acquire can be called without releasing the image index through present.
+        inline uint8_t getMaxBlockingAcquiresBeforePresent() const override { return m_maxBlockingAcquiresBeforePresent; }
+        
+        // returns the maximum number of acquires you can request without waiting for previous acquire semaphores to signal.
+        uint8_t getMaxAcquiresInFlight() const override { return getImageCount(); }
 
     private:
         CVulkanSwapchain(
@@ -32,7 +38,8 @@ class CVulkanSwapchain final : public ISwapchain
             const VkSwapchainKHR swapchain,
             const VkSemaphore* const _acquireAdaptorSemaphores,
             const VkSemaphore* const _prePresentSemaphores,
-            const VkSemaphore* const _presentAdaptorSemaphores
+            const VkSemaphore* const _presentAdaptorSemaphores,
+            const uint8_t maxAcquiresBeforePresent
         );
         ~CVulkanSwapchain();
 
@@ -51,6 +58,7 @@ class CVulkanSwapchain final : public ISwapchain
         uint64_t m_perImageAcquireCount[ISwapchain::MaxImages] = { 0 };
         // nasty way to fight UB of the Vulkan spec
         bool m_needToWaitIdle = true;
+        uint8_t m_maxBlockingAcquiresBeforePresent = 0u;
 };
 
 }

include/nbl/video/ISwapchain.h

@@ -464,15 +464,21 @@ class ISwapchain : public IBackendObject
         // Vulkan: const VkSwapchainKHR*
         virtual const void* getNativeHandle() const = 0;
 
-		// only public because MultiTimelineEventHandlerST needs to know about it
-		class DeferredFrameSemaphoreDrop final
-		{
+        // returns the maximum number of time acquires with infinite timeout which can be called before releasing the image index through present.
+        virtual uint8_t getMaxBlockingAcquiresBeforePresent() const = 0u;
+
+        // returns the maximum number of acquires you can request without waiting for previous acquire semaphores to signal.
+        virtual uint8_t getMaxAcquiresInFlight() const = 0u;
+
+        // only public because MultiTimelineEventHandlerST needs to know about it
+        class DeferredFrameSemaphoreDrop final
+        {
                 using sema_refctd_ptr = core::smart_refctd_ptr<ISemaphore>;
-				core::smart_refctd_dynamic_array<sema_refctd_ptr> m_otherSemaphores = nullptr;
+                core::smart_refctd_dynamic_array<sema_refctd_ptr> m_otherSemaphores = nullptr;
 
-			public:
-				inline DeferredFrameSemaphoreDrop(const std::span<const IQueue::SSubmitInfo::SSemaphoreInfo> _semaphores)
-				{
+            public:
+                inline DeferredFrameSemaphoreDrop(const std::span<const IQueue::SSubmitInfo::SSemaphoreInfo> _semaphores)
+                {
                     const auto otherCount = _semaphores.size()-1;
                     // first semaphore always serves as the timeline and will be refcounted in the event handler
                     if (otherCount==0)
@@ -481,34 +487,34 @@ class ISwapchain : public IBackendObject
 
                     for (auto i=0ull; i<otherCount; i++)
                         m_otherSemaphores->operator[](i) = sema_refctd_ptr(_semaphores[i].semaphore);
-				}
+                }
                 DeferredFrameSemaphoreDrop(const DeferredFrameSemaphoreDrop& other) = delete;
-				inline DeferredFrameSemaphoreDrop(DeferredFrameSemaphoreDrop&& other) : m_otherSemaphores(nullptr)
-				{
-					this->operator=(std::move(other));
-				}
+                inline DeferredFrameSemaphoreDrop(DeferredFrameSemaphoreDrop&& other) : m_otherSemaphores(nullptr)
+                {
+                    this->operator=(std::move(other));
+                }
 
                 DeferredFrameSemaphoreDrop& operator=(const DeferredFrameSemaphoreDrop& other) = delete;
-				inline DeferredFrameSemaphoreDrop& operator=(DeferredFrameSemaphoreDrop&& other)
-				{
+                inline DeferredFrameSemaphoreDrop& operator=(DeferredFrameSemaphoreDrop&& other)
+                {
                     m_otherSemaphores = std::move(other.m_otherSemaphores);
-					other.m_otherSemaphores = nullptr;
-					return *this;
-				}
-
-				struct single_poll_t {};
-				static inline single_poll_t single_poll;
-				inline bool operator()(single_poll_t _single_poll)
-				{
-					operator()();
-					return true;
-				}
+                    other.m_otherSemaphores = nullptr;
+                    return *this;
+                }
+
+                struct single_poll_t {};
+                static inline single_poll_t single_poll;
+                inline bool operator()(single_poll_t _single_poll)
+                {
+                    operator()();
+                    return true;
+                }
 
                 inline void operator()()
                 {
                     m_otherSemaphores = nullptr;
                 }
-		};
+        };
 
     protected:
         ISwapchain(core::smart_refctd_ptr<const ILogicalDevice>&& dev, SCreationParams&& params, const uint8_t imageCount, core::smart_refctd_ptr<ISwapchain>&& oldSwapchain);

include/nbl/video/utilities/CSmoothResizeSurface.h

@@ -336,7 +336,7 @@ class NBL_API2 ISmoothResizeSurface : public ISimpleManagedSurface
 			auto device = const_cast<ILogicalDevice*>(queue->getOriginDevice());
 
 			// need to wait before resetting a commandbuffer
-			const auto maxFramesInFlight = getMaxFramesInFlight();
+			const auto maxFramesInFlight = getMaxAcquiresInFlight();
 			if (acquireCount>maxFramesInFlight)
 			{
 				const ISemaphore::SWaitInfo cmdbufDonePending[1] = {
@@ -454,7 +454,7 @@ class CSmoothResizeSurface final : public ISmoothResizeSurface
 
 			// transient commandbuffer and pool to perform the blits or other copies to SC images
 			auto pool = device->createCommandPool(queue->getFamilyIndex(),IGPUCommandPool::CREATE_FLAGS::RESET_COMMAND_BUFFER_BIT);
-			if (!pool || !pool->createCommandBuffers(IGPUCommandPool::BUFFER_LEVEL::PRIMARY,{m_cmdbufs.data(),getMaxFramesInFlight()}))
+			if (!pool || !pool->createCommandBuffers(IGPUCommandPool::BUFFER_LEVEL::PRIMARY,{m_cmdbufs.data(),getMaxAcquiresInFlightUpperLimit()}))
 				return init_fail();
 
 			m_swapchainResources = std::move(scResources);

include/nbl/video/utilities/ISimpleManagedSurface.h

@@ -39,9 +39,22 @@ class NBL_API2 ISimpleManagedSurface : public core::IReferenceCounted
 		//
 		inline ISurface* getSurface() {return m_surface.get();}
 		inline const ISurface* getSurface() const {return m_surface.get();}
+		
 
-		//
-		inline uint8_t getMaxFramesInFlight() const {return m_maxFramesInFlight;}
+		// returns the maximum number of acquires you can request without waiting for previous acquire semaphores to signal.
+		// this value is dynamic, so don't think about caching this value, this might change on swapchain recreation.
+		inline uint8_t getMaxAcquiresInFlight()
+		{
+			auto swapchainResources = getSwapchainResources();
+			if (swapchainResources && swapchainResources->swapchain)
+				return core::min(swapchainResources->swapchain->getMaxAcquiresInFlight(), m_maxImageCount);
+			return m_maxImageCount;
+		}
+
+		// returns upper limit of maximum acquires in flight returned in `getMaxAcquiresInFlight()`.
+		// use this to allocate your command buffers or per frame in flight resources
+		// call this function after initilization.
+		inline uint8_t getMaxAcquiresInFlightUpperLimit() const { assert(m_maxImageCount > 0); return m_maxImageCount; }
 
 		// A small utility for the boilerplate
 		inline uint8_t pickQueueFamily(ILogicalDevice* device) const
@@ -166,7 +179,7 @@ class NBL_API2 ISimpleManagedSurface : public core::IReferenceCounted
 				m_queue->waitIdle();
 
 			m_queue = nullptr;
-			m_maxFramesInFlight = 0;
+			m_maxImageCount = 0;
 			m_acquireCount = 0;
 			std::fill(m_acquireSemaphores.begin(),m_acquireSemaphores.end(),nullptr);
 		}
@@ -179,7 +192,7 @@ class NBL_API2 ISimpleManagedSurface : public core::IReferenceCounted
 
 		// An interesting solution to the "Frames In Flight", our tiny wrapper class will have its own Timeline Semaphore incrementing with each acquire, and thats it.
 		inline uint64_t getAcquireCount() {return m_acquireCount;}
-		inline ISemaphore* getAcquireSemaphore(const uint8_t ix) {return ix<m_maxFramesInFlight ? m_acquireSemaphores[ix].get():nullptr;}
+		inline ISemaphore* getAcquireSemaphore(const uint8_t ix) {return ix<m_maxImageCount ? m_acquireSemaphores[ix].get():nullptr;}
 
 		// What `acquireNextImage` returns
 		struct SAcquireResult
@@ -213,11 +226,10 @@ class NBL_API2 ISimpleManagedSurface : public core::IReferenceCounted
 					{
 						ISurface::SCapabilities caps = {};
 						m_surface->getSurfaceCapabilitiesForPhysicalDevice(physDev,caps);
-						// vkAcquireNextImageKHR should not be called if the number of images that the application has currently acquired is greater than SwapchainImages-MinimumImages
-						m_maxFramesInFlight = core::min(caps.maxImageCount+1-caps.minImageCount,ISwapchain::MaxImages);
+						m_maxImageCount = core::min(caps.maxImageCount,ISwapchain::MaxImages);
 					}
 
-					for (uint8_t i=0u; i<m_maxFramesInFlight; i++)
+					for (uint8_t i=0u; i<m_maxImageCount; i++)
 					{
 						m_acquireSemaphores[i] = device->createSemaphore(0u);
 						if (!m_acquireSemaphores[i])
@@ -251,7 +263,7 @@ class NBL_API2 ISimpleManagedSurface : public core::IReferenceCounted
 
 				const auto nextAcquireSignal = m_acquireCount+1;
 				SAcquireResult retval = {
-					.semaphore = m_acquireSemaphores[nextAcquireSignal%m_maxFramesInFlight].get(),
+					.semaphore = m_acquireSemaphores[nextAcquireSignal%m_maxImageCount].get(),
 					.acquireCount = nextAcquireSignal
 				};
 				const IQueue::SSubmitInfo::SSemaphoreInfo signalInfos[1] = {
@@ -344,12 +356,13 @@ class NBL_API2 ISimpleManagedSurface : public core::IReferenceCounted
 
 		// Use a Threadsafe queue to make sure we can do smooth resize in derived class, might get re-assigned
 		CThreadSafeQueueAdapter* m_queue = nullptr;
-		//
-		uint8_t m_maxFramesInFlight = 0;
+		
+		uint8_t m_maxImageCount = 0;
+
 		// Created and persistent after first initialization, Note that we need one swapchain per Frame In Fligth because Acquires can't wait or synchronize with anything
-		// The only rule is that you can only have `m_maxFramesInFlight` pending acquires to wait with an infinte timeout, so thats about as far as they synchronize.
 		std::array<core::smart_refctd_ptr<ISemaphore>,ISwapchain::MaxImages> m_acquireSemaphores;
-		// You don't want to use `m_swapchainResources.swapchain->getAcquireCount()` because it resets when swapchain gets recreated
+
+		// We shouldn't use `m_swapchainResources.swapchain->getAcquireCount()` because it resets when swapchain gets recreated
 		uint64_t m_acquireCount = 0;
 };
 

src/nbl/video/CVulkanSwapchain.cpp

@@ -13,11 +13,11 @@ core::smart_refctd_ptr<CVulkanSwapchain> CVulkanSwapchain::create(core::smart_re
         return nullptr;
     if (!params.surface)
         return nullptr;
+    
+    auto physDev = logicalDevice->getPhysicalDevice();
 
     // now check if any queue family of the physical device supports this surface
     {
-        auto physDev = logicalDevice->getPhysicalDevice();
-
         bool noSupport = true;
         for (auto familyIx=0u; familyIx<ILogicalDevice::MaxQueueFamilies; familyIx++)
         if (logicalDevice->getQueueCount(familyIx) && params.surface->isSupportedForPhysicalDevice(physDev,familyIx))
@@ -148,7 +148,12 @@ core::smart_refctd_ptr<CVulkanSwapchain> CVulkanSwapchain::create(core::smart_re
         return nullptr;
     }
 
-    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);
+    ISurface::SCapabilities caps = {};
+    params.surface->getSurfaceCapabilitiesForPhysicalDevice(physDev, caps);
+    // read https://registry.khronos.org/vulkan/specs/1.3-extensions/html/chap34.html#swapchain-acquire-forward-progress
+    const uint8_t maxAcquiresWithoutPresent = core::min(imageCount,ISwapchain::MaxImages) - caps.minImageCount + 1u;
+
+    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, maxAcquiresWithoutPresent),core::dont_grab);
 }
 
 CVulkanSwapchain::CVulkanSwapchain(
@@ -159,9 +164,10 @@ CVulkanSwapchain::CVulkanSwapchain(
     const VkSwapchainKHR swapchain,
     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)
+    const VkSemaphore* const _presentAdaptorSemaphores,
+    const uint8_t maxAcquiresBeforePresent) 
+    : 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), m_maxBlockingAcquiresBeforePresent(maxAcquiresBeforePresent)
 {
     // we've got it from here!
     if (m_oldSwapchain)
@@ -173,10 +179,13 @@ CVulkanSwapchain::CVulkanSwapchain(
         auto retval = vulkanDevice->getFunctionTable()->vk.vkGetSwapchainImagesKHR(vulkanDevice->getInternalObject(),m_vkSwapchainKHR,&dummy,m_images);
         assert(retval==VK_SUCCESS && dummy==getImageCount());
     }
+    const uint8_t maxAcquiresInFlight = getMaxAcquiresInFlight();
+    assert(maxAcquiresInFlight == imageCount);
+    assert(getMaxBlockingAcquiresBeforePresent() <= imageCount);
 
-    std::copy_n(_acquireAdaptorSemaphores,imageCount,m_acquireAdaptorSemaphores);
-    std::copy_n(_prePresentSemaphores,imageCount,m_prePresentSemaphores);
-    std::copy_n(_presentAdaptorSemaphores,imageCount,m_presentAdaptorSemaphores);
+    std::copy_n(_acquireAdaptorSemaphores,maxAcquiresInFlight,m_acquireAdaptorSemaphores);
+    std::copy_n(_prePresentSemaphores,maxAcquiresInFlight,m_prePresentSemaphores);
+    std::copy_n(_presentAdaptorSemaphores,maxAcquiresInFlight,m_presentAdaptorSemaphores);
 }
 
 CVulkanSwapchain::~CVulkanSwapchain()