From 07e5ad5b232534a1da2cef9199ac4ca2225016dd Mon Sep 17 00:00:00 2001 From: Vladislav Belov Date: Mon, 8 Jun 2026 18:27:08 +0200 Subject: [PATCH] Fixes acquire and submit semaphores syncronization I forgot to finish CSubmitScheduler synchronization in 7c84c231146566be5916fd284e62fba367ea313c. Even ++m_FrameID was forgotten. And it worked at the time. A proper synchronization is described in renderer/backend/Vulkan/SwapChain.h. --- .../backend/vulkan/SubmitScheduler.cpp | 44 ++++-------- .../renderer/backend/vulkan/SubmitScheduler.h | 19 ++---- source/renderer/backend/vulkan/SwapChain.cpp | 68 +++++++++++++++++-- source/renderer/backend/vulkan/SwapChain.h | 34 +++++++++- 4 files changed, 113 insertions(+), 52 deletions(-) diff --git a/source/renderer/backend/vulkan/SubmitScheduler.cpp b/source/renderer/backend/vulkan/SubmitScheduler.cpp index 791b056c85..4cc8fb1ca0 100644 --- a/source/renderer/backend/vulkan/SubmitScheduler.cpp +++ b/source/renderer/backend/vulkan/SubmitScheduler.cpp @@ -1,4 +1,4 @@ -/* Copyright (C) 2025 Wildfire Games. +/* Copyright (C) 2026 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -58,17 +58,6 @@ std::unique_ptr CSubmitScheduler::Create( submitScheduler->m_Fences.push_back({fence, INVALID_SUBMIT_HANDLE}); } - for (FrameObject& frameObject : submitScheduler->m_FrameObjects) - { - VkSemaphoreCreateInfo semaphoreCreateInfo{}; - semaphoreCreateInfo.sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO; - RETURN_NULLPTR_IF_NOT_VK_SUCCESS(vkCreateSemaphore( - device->GetVkDevice(), &semaphoreCreateInfo, nullptr, &frameObject.acquireImageSemaphore)); - - RETURN_NULLPTR_IF_NOT_VK_SUCCESS(vkCreateSemaphore( - device->GetVkDevice(), &semaphoreCreateInfo, nullptr, &frameObject.submitDone)); - } - submitScheduler->m_AcquireCommandContext = CRingCommandContext::Create( device, NUMBER_OF_FRAMES_IN_FLIGHT, queueFamilyIndex, *submitScheduler); if (!submitScheduler->m_AcquireCommandContext) @@ -103,48 +92,39 @@ CSubmitScheduler::~CSubmitScheduler() for (Fence& fence : m_Fences) if (fence.value != VK_NULL_HANDLE) vkDestroyFence(device, fence.value, nullptr); - - for (FrameObject& frameObject : m_FrameObjects) - { - if (frameObject.acquireImageSemaphore != VK_NULL_HANDLE) - vkDestroySemaphore(device, frameObject.acquireImageSemaphore, nullptr); - - if (frameObject.submitDone != VK_NULL_HANDLE) - vkDestroySemaphore(device, frameObject.submitDone, nullptr); - } } -bool CSubmitScheduler::AcquireNextImage(CSwapChain& swapChain) +bool CSubmitScheduler::AcquireNextImage(CSwapChain& swapChain, VkSemaphore acquireImageSemaphore) { if (m_DebugWaitIdleBeforeAcquire) vkDeviceWaitIdle(m_Device->GetVkDevice()); - FrameObject& frameObject = m_FrameObjects[m_FrameID % m_FrameObjects.size()]; - if (!swapChain.AcquireNextImage(frameObject.acquireImageSemaphore)) + if (!swapChain.AcquireNextImage()) return false; swapChain.SubmitCommandsAfterAcquireNextImage(*m_AcquireCommandContext); - m_NextWaitSemaphore = frameObject.acquireImageSemaphore; + m_NextWaitSemaphore = acquireImageSemaphore; m_NextWaitDstStageMask = VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT; m_AcquireCommandContext->Flush(); return true; } -void CSubmitScheduler::Present(CSwapChain& swapChain) +CSubmitScheduler::SubmitHandle CSubmitScheduler::Present(CSwapChain& swapChain, VkSemaphore submitDone) { - FrameObject& frameObject = m_FrameObjects[m_FrameID % m_FrameObjects.size()]; swapChain.SubmitCommandsBeforePresent(*m_PresentCommandContext); - m_NextSubmitSignalSemaphore = frameObject.submitDone; + m_NextSubmitSignalSemaphore = submitDone; m_PresentCommandContext->Flush(); - Flush(); + const SubmitHandle submitHandle{Flush()}; if (m_DebugWaitIdleBeforePresent) vkDeviceWaitIdle(m_Device->GetVkDevice()); - swapChain.Present(frameObject.submitDone, m_Queue); + swapChain.Present(submitDone, m_Queue); if (m_DebugWaitIdleAfterPresent) vkDeviceWaitIdle(m_Device->GetVkDevice()); + + return submitHandle; } CSubmitScheduler::SubmitHandle CSubmitScheduler::Submit(VkCommandBuffer commandBuffer) @@ -172,7 +152,7 @@ void CSubmitScheduler::WaitUntilFree(const SubmitHandle handle) } } -void CSubmitScheduler::Flush() +CSubmitScheduler::SubmitHandle CSubmitScheduler::Flush() { ENSURE(!m_SubmittedCommandBuffers.empty()); @@ -208,6 +188,8 @@ void CSubmitScheduler::Flush() m_NextSubmitSignalSemaphore = VK_NULL_HANDLE; m_SubmittedCommandBuffers.clear(); + + return fence.lastUsedHandle; } } // namespace Vulkan diff --git a/source/renderer/backend/vulkan/SubmitScheduler.h b/source/renderer/backend/vulkan/SubmitScheduler.h index 592b9ace06..be9df34f50 100644 --- a/source/renderer/backend/vulkan/SubmitScheduler.h +++ b/source/renderer/backend/vulkan/SubmitScheduler.h @@ -1,4 +1,4 @@ -/* Copyright (C) 2025 Wildfire Games. +/* Copyright (C) 2026 Wildfire Games. * This file is part of 0 A.D. * * 0 A.D. is free software: you can redistribute it and/or modify @@ -53,9 +53,9 @@ public: CDevice* device, const uint32_t queueFamilyIndex, VkQueue queue); ~CSubmitScheduler(); - bool AcquireNextImage(CSwapChain& swapChain); + bool AcquireNextImage(CSwapChain& swapChain, VkSemaphore acquireImageSemaphore); - void Present(CSwapChain& swapChain); + SubmitHandle Present(CSwapChain& swapChain, VkSemaphore submitDone); SubmitHandle Submit(VkCommandBuffer commandBuffer); @@ -63,7 +63,7 @@ public: uint32_t GetFrameID() const { return m_FrameID; } - void Flush(); + SubmitHandle Flush(); private: CSubmitScheduler(CDevice* device, VkQueue queue); @@ -90,17 +90,6 @@ private: }; std::queue m_SubmittedHandles; - // We can't reuse frame data immediately after present because it might - // still be processing on GPU. - struct FrameObject - { - // We need to wait for the image on GPU to draw to it. - VkSemaphore acquireImageSemaphore = VK_NULL_HANDLE; - // We need to present only after all submit work is done. - VkSemaphore submitDone = VK_NULL_HANDLE; - }; - std::array m_FrameObjects; - VkSemaphore m_NextWaitSemaphore = VK_NULL_HANDLE; VkPipelineStageFlags m_NextWaitDstStageMask = 0; VkSemaphore m_NextSubmitSignalSemaphore = VK_NULL_HANDLE; diff --git a/source/renderer/backend/vulkan/SwapChain.cpp b/source/renderer/backend/vulkan/SwapChain.cpp index 555616880d..403f2e1589 100644 --- a/source/renderer/backend/vulkan/SwapChain.cpp +++ b/source/renderer/backend/vulkan/SwapChain.cpp @@ -248,6 +248,37 @@ std::unique_ptr CSwapChain::Create( swapChainCreateInfo.imageUsage, swapChainWidth, swapChainHeight); } + // +1 to minimize waiting on our side instead of vkAcquireNextImageKHR. + for (size_t index{0}; index < NUMBER_OF_FRAMES_IN_FLIGHT + 1; ++index) + { + VkSemaphore semaphore{VK_NULL_HANDLE}; + + VkSemaphoreCreateInfo semaphoreCreateInfo{}; + semaphoreCreateInfo.sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO; + + RETURN_NULLPTR_IF_NOT_VK_SUCCESS(vkCreateSemaphore( + device->GetVkDevice(), &semaphoreCreateInfo, nullptr, &semaphore)); + snprintf(nameBuffer, std::size(nameBuffer), "SwapChainAcquireSemaphore #%zu", index); + device->SetObjectName(VK_OBJECT_TYPE_SEMAPHORE, semaphore, nameBuffer); + + swapChain->m_FrameObjects.emplace_back(FrameObject{semaphore, CSubmitScheduler::INVALID_SUBMIT_HANDLE}); + } + + for (size_t index{0}; index < imageCount; ++index) + { + VkSemaphore semaphore{VK_NULL_HANDLE}; + + VkSemaphoreCreateInfo semaphoreCreateInfo{}; + semaphoreCreateInfo.sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO; + + RETURN_NULLPTR_IF_NOT_VK_SUCCESS(vkCreateSemaphore( + device->GetVkDevice(), &semaphoreCreateInfo, nullptr, &semaphore)); + snprintf(nameBuffer, std::size(nameBuffer), "SwapChainSubmitSemaphore #%zu", index); + device->SetObjectName(VK_OBJECT_TYPE_SEMAPHORE, semaphore, nameBuffer); + + swapChain->m_SubmitSemaphores.emplace_back(semaphore); + } + swapChain->m_IsValid = true; return swapChain; @@ -257,13 +288,28 @@ CSwapChain::CSwapChain() = default; CSwapChain::~CSwapChain() { + VkDevice device{m_Device->GetVkDevice()}; + m_Backbuffers.clear(); m_Textures.clear(); m_DepthTexture.reset(); if (m_SwapChain != VK_NULL_HANDLE) - vkDestroySwapchainKHR(m_Device->GetVkDevice(), m_SwapChain, nullptr); + vkDestroySwapchainKHR(device, m_SwapChain, nullptr); + + for (FrameObject& frameObject : m_FrameObjects) + { + if (frameObject.submitHandle != CSubmitScheduler::INVALID_SUBMIT_HANDLE) + m_SubmitScheduler->WaitUntilFree(frameObject.submitHandle); + + if (frameObject.acquireImageSemaphore != VK_NULL_HANDLE) + vkDestroySemaphore(device, frameObject.acquireImageSemaphore, nullptr); + } + + for (VkSemaphore& semaphore : m_SubmitSemaphores) + if (semaphore != VK_NULL_HANDLE) + vkDestroySemaphore(device, semaphore, nullptr); } IDevice* CSwapChain::GetDevice() @@ -276,16 +322,29 @@ bool CSwapChain::AcquireNextBackbuffer() ENSURE(m_IsValid); PROFILE3("AcquireNextBackbuffer"); - return m_SubmitScheduler->AcquireNextImage(*this); + + // We need to make sure we can reuse the semaphore. + FrameObject& frameObject{m_FrameObjects[m_FrameID % m_FrameObjects.size()]}; + if (frameObject.submitHandle != CSubmitScheduler::INVALID_SUBMIT_HANDLE) + { + PROFILE3("WaitAcquireSemaphore"); + m_SubmitScheduler->WaitUntilFree(frameObject.submitHandle); + } + + return m_SubmitScheduler->AcquireNextImage(*this, frameObject.acquireImageSemaphore); } void CSwapChain::Present() { ENSURE(m_IsValid); + ENSURE(m_CurrentImageIndex != std::numeric_limits::max()); + ENSURE(m_CurrentImageIndex < m_SubmitSemaphores.size()); PROFILE3("Present"); - m_SubmitScheduler->Present(*this); + FrameObject& frameObject{m_FrameObjects[m_FrameID % m_FrameObjects.size()]}; + frameObject.submitHandle = m_SubmitScheduler->Present(*this, m_SubmitSemaphores[m_CurrentImageIndex]); m_Device->OnPresent(); + ++m_FrameID; } size_t CSwapChain::SwapChainBackbuffer::BackbufferKeyHash::operator()(const BackbufferKey& key) const @@ -304,11 +363,12 @@ CSwapChain::SwapChainBackbuffer::SwapChainBackbuffer(SwapChainBackbuffer&& other CSwapChain::SwapChainBackbuffer& CSwapChain::SwapChainBackbuffer::operator=(SwapChainBackbuffer&& other) = default; -bool CSwapChain::AcquireNextImage(VkSemaphore acquireImageSemaphore) +bool CSwapChain::AcquireNextImage() { ENSURE(m_IsValid); ENSURE(m_CurrentImageIndex == std::numeric_limits::max()); + VkSemaphore acquireImageSemaphore{m_FrameObjects[m_FrameID % m_FrameObjects.size()].acquireImageSemaphore}; const VkResult acquireResult = vkAcquireNextImageKHR( m_Device->GetVkDevice(), m_SwapChain, std::numeric_limits::max(), acquireImageSemaphore, diff --git a/source/renderer/backend/vulkan/SwapChain.h b/source/renderer/backend/vulkan/SwapChain.h index 70b4c5695d..17d234beaa 100644 --- a/source/renderer/backend/vulkan/SwapChain.h +++ b/source/renderer/backend/vulkan/SwapChain.h @@ -20,6 +20,7 @@ #include "renderer/backend/IFramebuffer.h" #include "renderer/backend/ISwapChain.h" +#include "renderer/backend/vulkan/SubmitScheduler.h" #include #include @@ -33,7 +34,6 @@ namespace Renderer::Backend::Vulkan { class CDevice; } namespace Renderer::Backend::Vulkan { class CFramebuffer; } namespace Renderer::Backend::Vulkan { class CRingCommandContext; } -namespace Renderer::Backend::Vulkan { class CSubmitScheduler; } namespace Renderer::Backend::Vulkan { class CTexture; } namespace Renderer @@ -66,7 +66,7 @@ public: VkSwapchainKHR GetVkSwapchain() { return m_SwapChain; } - bool AcquireNextImage(VkSemaphore acquireImageSemaphore); + bool AcquireNextImage(); void SubmitCommandsAfterAcquireNextImage( CRingCommandContext& commandContext); void SubmitCommandsBeforePresent( @@ -103,6 +103,36 @@ private: std::unique_ptr m_DepthTexture; VkFormat m_ImageFormat = VK_FORMAT_UNDEFINED; + uint32_t m_FrameID{0}; + + // We can't reuse the same acquire semaphore immediately after present + // because it might still be processing on GPU as vkQueuePresentKHR doesn't + // have to be blocking. + // We need to wait for the image on GPU to draw to it. + struct FrameObject + { + VkSemaphore acquireImageSemaphore{VK_NULL_HANDLE}; + // We need to know when we can reuse the semaphore. + CSubmitScheduler::SubmitHandle submitHandle{CSubmitScheduler::INVALID_SUBMIT_HANDLE}; + }; + std::vector m_FrameObjects; + + // The number of submit semaphores should be equal to the number of images + // in the swapchain. We could use NUMBER_OF_FRAMES_IN_FLIGHT of objects but + // it might be not safe. Since vkQueuePresentKHR doesn't provide a way to + // tell that a semaphore was signaled. + // + // A possible situation, list of acquired indices: + // 0, 1, 2, 0, 1, 0, 1 + // ^ + // In theory in the end we might still have a semaphore in use for the + // 2nd swapchain image. + // + // See also: + // https://docs.vulkan.org/guide/latest/swapchain_semaphore_reuse.html + // We need to present only after all submit work is done. + std::vector m_SubmitSemaphores; + struct SwapChainBackbuffer { using BackbufferKey = std::tuple<