From 24d62ffd720b5bec5d07b07b8d2c9dda7635f3c0 Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Wed, 15 Jan 2025 15:29:32 +0100 Subject: [PATCH] rhi: vulkan: Simplify frame sync imageFence seems to have no purpose. And some of the comments seem to be out of date / misleading. More importantly, do the command buffer completion wait before the acquireNextImage. Otherwise frame.imageSem may have wait operations pending if the queueSubmit did not get to wait on it yet. The expectation is that this potential issue is what triggers the recent validation layer releases' enhanced synchronization checkers. Pick-to: 6.9 6.8 Fixes: QTBUG-132808 Change-Id: Ib8e3884ccea343f5a839aa2ee17cd358fafeac69 Reviewed-by: Andy Nichols --- src/gui/rhi/qrhivulkan.cpp | 44 +++++++++++--------------------------- src/gui/rhi/qrhivulkan_p.h | 2 -- 2 files changed, 12 insertions(+), 34 deletions(-) diff --git a/src/gui/rhi/qrhivulkan.cpp b/src/gui/rhi/qrhivulkan.cpp index 7ff3f335d82..9f7e0fc9d06 100644 --- a/src/gui/rhi/qrhivulkan.cpp +++ b/src/gui/rhi/qrhivulkan.cpp @@ -2413,9 +2413,6 @@ bool QRhiVulkan::recreateSwapChain(QRhiSwapChain *swapChain) frame.imageAcquired = false; frame.imageSemWaitable = false; - df->vkCreateFence(dev, &fenceInfo, nullptr, &frame.imageFence); - frame.imageFenceWaitable = true; // fence was created in signaled state - df->vkCreateSemaphore(dev, &semInfo, nullptr, &frame.imageSem); df->vkCreateSemaphore(dev, &semInfo, nullptr, &frame.drawSem); @@ -2451,13 +2448,6 @@ void QRhiVulkan::releaseSwapChainResources(QRhiSwapChain *swapChain) frame.cmdFence = VK_NULL_HANDLE; frame.cmdFenceWaitable = false; } - if (frame.imageFence) { - if (frame.imageFenceWaitable) - df->vkWaitForFences(dev, 1, &frame.imageFence, VK_TRUE, UINT64_MAX); - df->vkDestroyFence(dev, frame.imageFence, nullptr); - frame.imageFence = VK_NULL_HANDLE; - frame.imageFenceWaitable = false; - } if (frame.imageSem) { df->vkDestroySemaphore(dev, frame.imageSem, nullptr); frame.imageSem = VK_NULL_HANDLE; @@ -2544,24 +2534,26 @@ QRhi::FrameOpResult QRhiVulkan::beginFrame(QRhiSwapChain *swapChain, QRhi::Begin inst->handle()->beginFrame(swapChainD->window); - if (!frame.imageAcquired) { - // Wait if we are too far ahead, i.e. the thread gets throttled based on the presentation rate - // (note that we are using FIFO mode -> vsync) - if (frame.imageFenceWaitable) { - df->vkWaitForFences(dev, 1, &frame.imageFence, VK_TRUE, UINT64_MAX); - df->vkResetFences(dev, 1, &frame.imageFence); - frame.imageFenceWaitable = false; - } + // Make sure the previous commands for the same frame slot have finished. + // + // Do this also for any other swapchain's commands with the same frame slot + // While this reduces concurrency, it keeps resource usage safe: swapchain + // A starting its frame 0, followed by swapchain B starting its own frame 0 + // will make B wait for A's frame 0 commands, so if a resource is written + // in B's frame or when B checks for pending resource releases, that won't + // mess up A's in-flight commands (as they are not in flight anymore). + waitCommandCompletion(frameResIndex); + if (!frame.imageAcquired) { // move on to next swapchain image uint32_t imageIndex = 0; VkResult err = vkAcquireNextImageKHR(dev, swapChainD->sc, UINT64_MAX, - frame.imageSem, frame.imageFence, &imageIndex); + frame.imageSem, VK_NULL_HANDLE, &imageIndex); + if (err == VK_SUCCESS || err == VK_SUBOPTIMAL_KHR) { swapChainD->currentImageIndex = imageIndex; frame.imageSemWaitable = true; frame.imageAcquired = true; - frame.imageFenceWaitable = true; } else if (err == VK_ERROR_OUT_OF_DATE_KHR) { return QRhi::FrameOpSwapChainOutOfDate; } else { @@ -2575,18 +2567,6 @@ QRhi::FrameOpResult QRhiVulkan::beginFrame(QRhiSwapChain *swapChain, QRhi::Begin } } - // Make sure the previous commands for the same image have finished. (note - // that this is based on the fence from the command buffer submit, nothing - // to do with the Present) - // - // Do this also for any other swapchain's commands with the same frame slot - // While this reduces concurrency, it keeps resource usage safe: swapchain - // A starting its frame 0, followed by swapchain B starting its own frame 0 - // will make B wait for A's frame 0 commands, so if a resource is written - // in B's frame or when B checks for pending resource releases, that won't - // mess up A's in-flight commands (as they are not in flight anymore). - waitCommandCompletion(frameResIndex); - currentFrameSlot = int(swapChainD->currentFrameSlot); currentSwapChain = swapChainD; if (swapChainD->ds) diff --git a/src/gui/rhi/qrhivulkan_p.h b/src/gui/rhi/qrhivulkan_p.h index 13bcf741fbf..77100f91a5b 100644 --- a/src/gui/rhi/qrhivulkan_p.h +++ b/src/gui/rhi/qrhivulkan_p.h @@ -653,8 +653,6 @@ struct QVkSwapChain : public QRhiSwapChain QVarLengthArray imageRes; struct FrameResources { - VkFence imageFence = VK_NULL_HANDLE; - bool imageFenceWaitable = false; VkSemaphore imageSem = VK_NULL_HANDLE; VkSemaphore drawSem = VK_NULL_HANDLE; bool imageAcquired = false;