From f6327194bc0ec35483e3bd6650346a017d3b7234 Mon Sep 17 00:00:00 2001 From: Cam Mannett Date: Fri, 6 Sep 2024 15:09:45 +0100 Subject: [PATCH] Switch to a ref-counted solution --- renderdoc/driver/vulkan/vk_core.cpp | 81 ++++++++++--------- renderdoc/driver/vulkan/vk_core.h | 11 +-- renderdoc/driver/vulkan/vk_resources.cpp | 23 ++++-- renderdoc/driver/vulkan/vk_resources.h | 34 ++++++-- .../driver/vulkan/wrappers/vk_cmd_funcs.cpp | 5 +- .../driver/vulkan/wrappers/vk_misc_funcs.cpp | 2 - .../driver/vulkan/wrappers/vk_queue_funcs.cpp | 2 + 7 files changed, 97 insertions(+), 61 deletions(-) diff --git a/renderdoc/driver/vulkan/vk_core.cpp b/renderdoc/driver/vulkan/vk_core.cpp index 8f467c7bc..ed21ff4f5 100644 --- a/renderdoc/driver/vulkan/vk_core.cpp +++ b/renderdoc/driver/vulkan/vk_core.cpp @@ -705,28 +705,17 @@ void WrappedVulkan::SetDebugMessageSink(WrappedVulkan::ScopedDebugMessageSink *s Threading::SetTLSValue(debugMessageSinkTLSSlot, (void *)sink); } -void WrappedVulkan::MarkPendingCommandBufferAsDeleted(VkCommandBuffer commandBuffer) +void WrappedVulkan::InsertPendingCommandBufferCallbacksEvent(VkCommandBuffer commandBuffer) { - SCOPED_LOCK(m_PendingCmdBufferCallbacksLock); - - for(PendingCommandBufferCallbacks &pending : m_PendingCmdBufferCallbacks) - { - if(pending.commandBuffer == commandBuffer) - { - pending.commandBuffer = VK_NULL_HANDLE; - return; - } - } -} - -void WrappedVulkan::AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuffer) -{ - SCOPED_LOCK(m_PendingCmdBufferCallbacksLock); + // This occurs pre-baking as the event needs to be in the command buffer before vkEndCommandBuffer + // is called VkResourceRecord *cmdRecord = GetRecord(commandBuffer); - rdcarray> &callbacks = cmdRecord->cmdInfo->pendingSubmissionCompleteCallbacks; + VkPendingSubmissionCompleteCallbacks *pending = + cmdRecord->cmdInfo->pendingSubmissionCompleteCallbacks; + RDCASSERT(pending->event == VK_NULL_HANDLE); - if(callbacks.empty()) + if(pending->callbacks.empty()) return; const VkEventCreateInfo info = {VK_STRUCTURE_TYPE_EVENT_CREATE_INFO}; @@ -736,41 +725,61 @@ void WrappedVulkan::AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuff ObjDisp(commandBuffer)->CmdSetEvent(Unwrap(commandBuffer), event, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT); - m_PendingCmdBufferCallbacks.push_back({event, commandBuffer, std::move(callbacks)}); + pending->device = cmdRecord->cmdInfo->device; + pending->event = event; +} + +void WrappedVulkan::AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuffer) +{ + VkResourceRecord *cmdRecord = GetRecord(commandBuffer); + VkPendingSubmissionCompleteCallbacks *pending = + cmdRecord->bakedCommands->cmdInfo->pendingSubmissionCompleteCallbacks; + + if(pending->callbacks.empty()) + return; + + RDCASSERT(pending->event != VK_NULL_HANDLE); + + pending->AddRef(); + + SCOPED_LOCK(m_PendingCmdBufferCallbacksLock); + m_PendingCmdBufferCallbacks.push_back(pending); } void WrappedVulkan::CheckPendingCommandBufferCallbacks() { + // This approach is bad for contention, so a future optimisation could be to: + // 1. Acquire the lock + // 2. Move m_PendingCmdBufferCallbacks into a local + // 3. Release the lock + // 4. Do the checks/execution + // 5. Acquire the lock + // 6. Merge any remaining entries to m_PendingCmdBufferCallbacks (which may have accumulated new + // entries from other threads) + // 7. Release the lock + SCOPED_LOCK(m_PendingCmdBufferCallbacksLock); - // Delete any events whose command buffers have been deleted and their callbacks called - m_PendingCmdBufferCallbacks.removeIf([d = m_Device](const PendingCommandBufferCallbacks &pending) { - const bool destroyEvent = pending.commandBuffer == VK_NULL_HANDLE && pending.callbacks.empty(); - if(destroyEvent) - ObjDisp(d)->DestroyEvent(Unwrap(d), pending.event, NULL); - - return destroyEvent; - }); - - for(PendingCommandBufferCallbacks &pending : m_PendingCmdBufferCallbacks) + for(size_t i = 0; i < m_PendingCmdBufferCallbacks.size();) { - // We've already executed the callbacks, we're now just waiting for the command buffer to be - // destroyed/reused so we can delete the event - if(pending.callbacks.empty()) - continue; + VkPendingSubmissionCompleteCallbacks *pending = m_PendingCmdBufferCallbacks[i]; - const VkResult vkr = ObjDisp(m_Device)->GetEventStatus(Unwrap(m_Device), pending.event); + const VkResult vkr = ObjDisp(m_Device)->GetEventStatus(Unwrap(m_Device), pending->event); if(vkr == VK_EVENT_SET) { - for(std::function &f : pending.callbacks) + for(std::function &f : pending->callbacks) f(); - pending.callbacks.clear(); + pending->Release(); + m_PendingCmdBufferCallbacks.erase(i); + continue; } else if(vkr != VK_EVENT_RESET) { CheckVkResult(vkr); } + + ++i; } } diff --git a/renderdoc/driver/vulkan/vk_core.h b/renderdoc/driver/vulkan/vk_core.h index 85ba40acb..fe083fb50 100644 --- a/renderdoc/driver/vulkan/vk_core.h +++ b/renderdoc/driver/vulkan/vk_core.h @@ -979,16 +979,9 @@ private: bytebuf m_MaskedMapData; - struct PendingCommandBufferCallbacks - { - VkEvent event; - VkCommandBuffer commandBuffer; - rdcarray> callbacks; - }; - Threading::CriticalSection m_PendingCmdBufferCallbacksLock; - rdcarray m_PendingCmdBufferCallbacks; - void MarkPendingCommandBufferAsDeleted(VkCommandBuffer commandBuffer); + rdcarray m_PendingCmdBufferCallbacks; + void InsertPendingCommandBufferCallbacksEvent(VkCommandBuffer commandBuffer); void AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuffer); void CheckPendingCommandBufferCallbacks(); diff --git a/renderdoc/driver/vulkan/vk_resources.cpp b/renderdoc/driver/vulkan/vk_resources.cpp index 335b414c5..243ce4862 100644 --- a/renderdoc/driver/vulkan/vk_resources.cpp +++ b/renderdoc/driver/vulkan/vk_resources.cpp @@ -3391,6 +3391,19 @@ VkImageAspectFlags FormatImageAspects(VkFormat fmt) return VK_IMAGE_ASPECT_COLOR_BIT; } +void VkPendingSubmissionCompleteCallbacks::Release() +{ + int32_t ref = Atomic::Dec32(&refCount); + RDCASSERT(ref >= 0); + if(ref <= 0) + { + if(event != VK_NULL_HANDLE) + ObjDisp(device)->DestroyEvent(Unwrap(device), event, NULL); + + delete this; + } +} + RenderPassInfo::RenderPassInfo(const VkRenderPassCreateInfo &ci) { // *2 in case we need separate barriers for depth and stencil, +1 for the terminating null @@ -3843,12 +3856,12 @@ InitReqType ImgRefs::SubresourceRangeMaxInitReq(VkImageSubresourceRange range, I return initReq; } -rdcarray > ImgRefs::SubresourceRangeInitReqs( +rdcarray> ImgRefs::SubresourceRangeInitReqs( VkImageSubresourceRange range, InitPolicy policy, bool initialized) const { VkImageSubresourceRange out(range); - rdcarray > res; - rdcarray > splitAspects; + rdcarray> res; + rdcarray> splitAspects; if(areAspectsSplit) { int aspectIndex = 0; @@ -4825,7 +4838,7 @@ TEST_CASE("Vulkan formats", "[format][vulkan]") { const uint32_t width = 24, height = 24; - rdcarray > > tests = { + rdcarray>> tests = { {VK_FORMAT_G8_B8_R8_3PLANE_420_UNORM, {576, 144, 144}}, {VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, {576, 288}}, {VK_FORMAT_G8_B8_R8_3PLANE_422_UNORM, {576, 288, 288}}, @@ -4852,7 +4865,7 @@ TEST_CASE("Vulkan formats", "[format][vulkan]") {VK_FORMAT_G16_B16R16_2PLANE_444_UNORM, {1152, 2304}}, }; - for(rdcpair > e : tests) + for(rdcpair> e : tests) { INFO("Format is " << ToStr(e.first)); for(uint32_t p = 0; p < e.second.size(); p++) diff --git a/renderdoc/driver/vulkan/vk_resources.h b/renderdoc/driver/vulkan/vk_resources.h index 81ede11ec..0ad5965b1 100644 --- a/renderdoc/driver/vulkan/vk_resources.h +++ b/renderdoc/driver/vulkan/vk_resources.h @@ -1088,6 +1088,24 @@ struct MemRefs struct ImgRefs; struct ImageState; +class VkPendingSubmissionCompleteCallbacks +{ +public: + VkPendingSubmissionCompleteCallbacks() = default; + VkPendingSubmissionCompleteCallbacks(const VkPendingSubmissionCompleteCallbacks &) = delete; + VkPendingSubmissionCompleteCallbacks(VkPendingSubmissionCompleteCallbacks &&) = delete; + + void AddRef() { Atomic::Inc32(&refCount); } + void Release(); + + VkDevice device = VK_NULL_HANDLE; + VkEvent event = VK_NULL_HANDLE; + rdcarray> callbacks; + +private: + int32_t refCount = 1; +}; + struct CmdPoolInfo { CmdPoolInfo() : pool(4 * 1024) {} @@ -1104,13 +1122,19 @@ struct CmdPoolInfo struct CmdBufferRecordingInfo { - CmdBufferRecordingInfo(CmdPoolInfo &pool) : alloc(pool.pool) {} + CmdBufferRecordingInfo(CmdPoolInfo &pool) + : alloc(pool.pool), + pendingSubmissionCompleteCallbacks(new VkPendingSubmissionCompleteCallbacks()) + { + } CmdBufferRecordingInfo(const CmdBufferRecordingInfo &) = delete; CmdBufferRecordingInfo(CmdBufferRecordingInfo &&) = delete; CmdBufferRecordingInfo &operator=(const CmdBufferRecordingInfo &) = delete; ~CmdBufferRecordingInfo() { // nothing to do explicitly, the alloc destructor will clean up any pages it holds + // pendingSubmissionCompleteCallbacks manages itself via ref-counting + pendingSubmissionCompleteCallbacks->Release(); } VkDevice device; @@ -1144,8 +1168,8 @@ struct CmdBufferRecordingInfo // A list of acceleration structures that this command buffer will build or copy rdcarray accelerationStructures; - // A list of callbacks to be executed once the command buffer execution has been completed - rdcarray> pendingSubmissionCompleteCallbacks; + // The VkEvent and the list of callbacks to be executed once it has been signalled + VkPendingSubmissionCompleteCallbacks *pendingSubmissionCompleteCallbacks = NULL; // AdvanceFrame/Present should be called after this buffer is submitted bool present; @@ -2250,8 +2274,8 @@ public: cmdInfo->imageStates.swap(bakedCommands->cmdInfo->imageStates); cmdInfo->memFrameRefs.swap(bakedCommands->cmdInfo->memFrameRefs); cmdInfo->accelerationStructures.swap(bakedCommands->cmdInfo->accelerationStructures); - cmdInfo->pendingSubmissionCompleteCallbacks.swap( - bakedCommands->cmdInfo->pendingSubmissionCompleteCallbacks); + std::swap(cmdInfo->pendingSubmissionCompleteCallbacks, + bakedCommands->cmdInfo->pendingSubmissionCompleteCallbacks); } // we have a lot of 'cold' data in the resource record, as it can be accessed diff --git a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp index 0c2c1b025..97087c015 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp @@ -1382,10 +1382,7 @@ VkResult WrappedVulkan::vkBeginCommandBuffer(VkCommandBuffer commandBuffer, // then begin is spec'd to implicitly reset. That means we need to tidy up // any existing baked commands before creating a new set. if(record->bakedCommands) - { - MarkPendingCommandBufferAsDeleted(commandBuffer); record->bakedCommands->Delete(GetResourceManager()); - } record->bakedCommands = GetResourceManager()->AddResourceRecord(ResourceIDGen::GetNewUniqueID()); record->bakedCommands->resType = eResCommandBuffer; @@ -1674,7 +1671,7 @@ VkResult WrappedVulkan::vkEndCommandBuffer(VkCommandBuffer commandBuffer) RDCASSERT(record); if(IsCaptureMode(m_State)) - AddPendingCommandBufferCallbacks(commandBuffer); + InsertPendingCommandBufferCallbacksEvent(commandBuffer); VkResult ret; SERIALISE_TIME_CALL(ret = ObjDisp(commandBuffer)->EndCommandBuffer(Unwrap(commandBuffer))); diff --git a/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp index 92929e54a..f7fd3d634 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp @@ -345,8 +345,6 @@ void WrappedVulkan::vkFreeCommandBuffers(VkDevice device, VkCommandPool commandP if(pCommandBuffers[c] == VK_NULL_HANDLE) continue; - MarkPendingCommandBufferAsDeleted(pCommandBuffers[c]); - WrappedVkDispRes *wrapped = (WrappedVkDispRes *)GetWrapped(pCommandBuffers[c]); #if ENABLED(VERBOSE_PARTIAL_REPLAY) diff --git a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp index 11f90d445..4e0c06451 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp @@ -1043,6 +1043,8 @@ void WrappedVulkan::CaptureQueueSubmit(VkQueue queue, record->bakedCommands->AddRef(); } + + AddPendingCommandBufferCallbacks(commandBuffers[i]); } if(backframe)