diff --git a/implementations/Crafter.Graphics-Window.cpp b/implementations/Crafter.Graphics-Window.cpp index 052d66c..5138839 100644 --- a/implementations/Crafter.Graphics-Window.cpp +++ b/implementations/Crafter.Graphics-Window.cpp @@ -434,27 +434,41 @@ Window::Window(std::uint32_t width, std::uint32_t height) : width(width), height cmdBufAllocateInfo.commandBufferCount = numFrames; Device::CheckVkResult(vkAllocateCommandBuffers(Device::device, &cmdBufAllocateInfo, drawCmdBuffers)); + // Per-frame-in-flight synchronisation objects (issue #40). The fences are + // created signaled so the first wait on each image's fence — before that + // image has ever been submitted — returns immediately instead of + // deadlocking. The submitInfo is rebuilt per frame in Render() now that + // the wait/signal semaphores vary by frame. VkSemaphoreCreateInfo semaphoreCreateInfo {}; semaphoreCreateInfo.sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO; - Device::CheckVkResult(vkCreateSemaphore(Device::device, &semaphoreCreateInfo, nullptr, &semaphores.presentComplete)); - Device::CheckVkResult(vkCreateSemaphore(Device::device, &semaphoreCreateInfo, nullptr, &semaphores.renderComplete)); - - // Set up submit info structure - // Semaphores will stay the same during application lifetime - // Command buffer submission info is set by each example - submitInfo.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; - submitInfo.pWaitDstStageMask = &submitPipelineStages; - submitInfo.waitSemaphoreCount = 1; - submitInfo.pWaitSemaphores = &semaphores.presentComplete; - submitInfo.signalSemaphoreCount = 1; - submitInfo.pSignalSemaphores = &semaphores.renderComplete; - submitInfo.pNext = VK_NULL_HANDLE; + VkFenceCreateInfo fenceCreateInfo {}; + fenceCreateInfo.sType = VK_STRUCTURE_TYPE_FENCE_CREATE_INFO; + fenceCreateInfo.flags = VK_FENCE_CREATE_SIGNALED_BIT; + for (std::uint8_t i = 0; i < numFrames; i++) { + Device::CheckVkResult(vkCreateSemaphore(Device::device, &semaphoreCreateInfo, nullptr, &renderFinishedSemaphores[i])); + Device::CheckVkResult(vkCreateFence(Device::device, &fenceCreateInfo, nullptr, &waitFences[i])); + } + for (std::uint8_t i = 0; i < numAcquireSemaphores; i++) { + Device::CheckVkResult(vkCreateSemaphore(Device::device, &semaphoreCreateInfo, nullptr, &imageAcquiredSemaphores[i])); + } // Per-frame info structs: everything that never varies between frames is - // set here once. Render() only patches the barriers' image/oldLayout. + // set here once. Render() patches the barriers' image/oldLayout and the + // sync handles that follow currentBuffer/frameCounter (submitInfo's + // wait/signal semaphores + command buffer, presentInfo's wait semaphore). cmdBufInfo = {}; cmdBufInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; + // Submit: stage mask + the 1/1/1 counts are invariant; the wait/signal + // semaphore handles and the command buffer are patched per frame. + submitInfo = {}; + submitInfo.sType = VK_STRUCTURE_TYPE_SUBMIT_INFO; + submitInfo.pNext = VK_NULL_HANDLE; + submitInfo.pWaitDstStageMask = &submitPipelineStages; + submitInfo.waitSemaphoreCount = 1; + submitInfo.signalSemaphoreCount = 1; + submitInfo.commandBufferCount = 1; + const VkImageSubresourceRange range { .aspectMask = VK_IMAGE_ASPECT_COLOR_BIT, .baseMipLevel = 0, @@ -491,8 +505,9 @@ Window::Window(std::uint32_t width, std::uint32_t height) : width(width), height }; // pImageIndices/pSwapchains point at members, so they follow value changes - // (currentBuffer each frame, swapChain across recreation) on their own. - // The render-complete wait semaphore is fixed for the window's lifetime. + // (currentBuffer each frame, swapChain across recreation) on their own. The + // wait semaphore is the per-image render-finished semaphore, patched per + // frame in Render() (it follows currentBuffer). presentInfo = {}; presentInfo.sType = VK_STRUCTURE_TYPE_PRESENT_INFO_KHR; presentInfo.pNext = NULL; @@ -500,7 +515,6 @@ Window::Window(std::uint32_t width, std::uint32_t height) : width(width), height presentInfo.pSwapchains = &swapChain; presentInfo.pImageIndices = ¤tBuffer; presentInfo.waitSemaphoreCount = 1; - presentInfo.pWaitSemaphores = &semaphores.renderComplete; lastMousePos = {0,0}; mouseDelta = {0,0}; @@ -744,22 +758,33 @@ void Window::Render() { // Acquire the next image from the swap chain. If the surface has // changed size out from under us (compositor/Win32 resize delivered // between Render calls), recreate and retry once. + // Pick this frame's acquire semaphore from a free-running CPU counter: the + // image index (currentBuffer) isn't known until acquire returns, so the + // acquire's signal semaphore can't be keyed by the image. + const std::uint32_t acquireSlot = static_cast(frameCounter % numAcquireSemaphores); + VkSemaphore imageAcquired = imageAcquiredSemaphores[acquireSlot]; { VkResult acquire = vkAcquireNextImageKHR(Device::device, swapChain, UINT64_MAX, - semaphores.presentComplete, (VkFence)nullptr, ¤tBuffer); + imageAcquired, (VkFence)nullptr, ¤tBuffer); if (acquire == VK_ERROR_OUT_OF_DATE_KHR) { Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); RecreateSwapchainAndImages(); onResize.Invoke(); acquire = vkAcquireNextImageKHR(Device::device, swapChain, UINT64_MAX, - semaphores.presentComplete, (VkFence)nullptr, ¤tBuffer); + imageAcquired, (VkFence)nullptr, ¤tBuffer); } if (acquire != VK_SUBOPTIMAL_KHR) { Device::CheckVkResult(acquire); } } - submitInfo.commandBufferCount = 1; - submitInfo.pCommandBuffers = &drawCmdBuffers[currentBuffer]; + + // drawCmdBuffers[currentBuffer] and this image's descriptor-heap slot are + // about to be re-recorded. Wait on the fence guarding this image's + // previous submission (keyed by image index, since acquire returns an + // arbitrary index), then reset it for this submit. This — not a full-queue + // wait-idle — is what bounds frames-in-flight and gives the CPU/GPU overlap. + Device::CheckVkResult(vkWaitForFences(Device::device, 1, &waitFences[currentBuffer], VK_TRUE, UINT64_MAX)); + Device::CheckVkResult(vkResetFences(Device::device, 1, &waitFences[currentBuffer])); Device::CheckVkResult(vkBeginCommandBuffer(drawCmdBuffers[currentBuffer], &cmdBufInfo)); @@ -854,20 +879,38 @@ void Window::Render() { Device::CheckVkResult(vkEndCommandBuffer(drawCmdBuffers[currentBuffer])); - Device::CheckVkResult(vkQueueSubmit(Device::queue, 1, &submitInfo, VK_NULL_HANDLE)); + // Patch the per-frame fields of the cached submitInfo/presentInfo (the + // invariant fields were set once in the constructor): submit waits on this + // frame's acquire semaphore and signals this image's render-finished + // semaphore, present waits on that same render-finished semaphore. The + // fence (keyed by image index) lets a future Render() reuse this image's + // command buffer + heap slot safely. The semaphore handles live in member + // arrays, so taking their addresses here is stable for the call. + submitInfo.pWaitSemaphores = &imageAcquiredSemaphores[acquireSlot]; + submitInfo.pSignalSemaphores = &renderFinishedSemaphores[currentBuffer]; + submitInfo.pCommandBuffers = &drawCmdBuffers[currentBuffer]; + Device::CheckVkResult(vkQueueSubmit(Device::queue, 1, &submitInfo, waitFences[currentBuffer])); + + presentInfo.pWaitSemaphores = &renderFinishedSemaphores[currentBuffer]; VkResult result = vkQueuePresentKHR(Device::queue, &presentInfo); if (result == VK_SUBOPTIMAL_KHR || result == VK_ERROR_OUT_OF_DATE_KHR) { - // Surface size changed mid-present. Drain the queue, rebuild the - // swapchain, and let dependents (descriptors holding old image - // handles) re-bind via onResize before the next frame. + // Surface size changed mid-present. Drain the queue (the only place we + // still wait-idle, alongside Resize() and the acquire OUT_OF_DATE + // path), rebuild the swapchain, and let dependents (descriptors + // holding old image handles) re-bind via onResize before the next + // frame. The wait-idle here also re-settles every per-frame fence to a + // signaled state, so the next Render()'s fence wait passes through. Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); RecreateSwapchainAndImages(); onResize.Invoke(); } else { Device::CheckVkResult(result); } - Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + // No vkQueueWaitIdle in the steady-state path: per-frame fences now gate + // command-buffer reuse, so the CPU is free to acquire/record frame N+1 + // while the GPU is still executing frame N. + frameCounter++; } #ifdef CRAFTER_TIMING diff --git a/interfaces/Crafter.Graphics-Window.cppm b/interfaces/Crafter.Graphics-Window.cppm index 94796cd..0676b96 100644 --- a/interfaces/Crafter.Graphics-Window.cppm +++ b/interfaces/Crafter.Graphics-Window.cppm @@ -270,18 +270,59 @@ export namespace Crafter { std::array imageInitialised{}; std::thread thread; VkCommandBuffer drawCmdBuffers[numFrames]; - VkSubmitInfo submitInfo; // Per-frame Vulkan info structs whose contents are almost entirely // invariant. Initialised once in the constructor; Render() patches only // the few fields that change each frame (the two barriers' image and - // the acquire barrier's oldLayout). presentInfo.pImageIndices and - // pSwapchains point at the currentBuffer/swapChain members, so they - // track value changes (including swapchain recreation) automatically. + // the acquire barrier's oldLayout, and — for the multi-frame-in-flight + // sync below — submitInfo/presentInfo's wait/signal semaphores plus + // submitInfo's command buffer, which follow currentBuffer/frameCounter). + // presentInfo.pImageIndices and pSwapchains point at the + // currentBuffer/swapChain members, so they track value changes + // (including swapchain recreation) automatically. + VkSubmitInfo submitInfo; VkCommandBufferBeginInfo cmdBufInfo; VkImageMemoryBarrier acquireBarrier; VkImageMemoryBarrier presentBarrier; VkPresentInfoKHR presentInfo; - Semaphores semaphores; + + // ── Multi-frame-in-flight synchronisation (issue #40) ────────────── + // The renderer keeps up to numFrames frames in flight, so a single + // semaphore pair + a per-frame wait-idle is no longer enough. Note + // that drawCmdBuffers, the per-frame descriptor heap slots, and the + // swapchain images are ALL keyed by the acquired image index + // (currentBuffer) — WriteSwapchainDescriptors bakes heap slot i to + // write imageViews[i], so the index that selects a command buffer / + // heap slot must equal the acquired image index. Everything below + // follows from that. + + // Signaled by the queue submit, waited by vkQueuePresentKHR. Keyed by + // the acquired IMAGE index: the presentation engine keeps this + // semaphore in use until the image is re-acquired, so a per-image + // semaphore is the only safe key. Keying it per-CPU-frame trips + // VUID-vkQueueSubmit-pSignalSemaphores-00067. + // https://docs.vulkan.org/guide/latest/swapchain_semaphore_reuse.html + std::array renderFinishedSemaphores{}; + + // Signaled by the queue submit, waited + reset before + // drawCmdBuffers[image] and that image's descriptor-heap slot are + // re-recorded. Keyed by image index. Created signaled so the first + // wait on each image passes through instead of deadlocking. + std::array waitFences{}; + + // Signaled by vkAcquireNextImageKHR, waited by the queue submit. Keyed + // by a free-running CPU frame counter, because the image index isn't + // known until acquire returns. Sized numFrames+1: at most numFrames + // frames are ever in flight (each image's command buffer is gated by + // its own fence, and there are numFrames images), so numFrames+1 + // distinct acquire semaphores guarantees the one being reused has no + // pending operation — required by + // VUID-vkAcquireNextImageKHR-semaphore-01779. + static constexpr std::uint8_t numAcquireSemaphores = numFrames + 1; + std::array imageAcquiredSemaphores{}; + + // Free-running count of Render() calls; drives the acquire-semaphore + // slot (frameCounter % numAcquireSemaphores). + std::uint64_t frameCounter = 0; std::uint32_t currentBuffer = 0; VkPipelineStageFlags submitPipelineStages = VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT; std::vector passes; diff --git a/project.cpp b/project.cpp index 1915b59..dabf62b 100644 --- a/project.cpp +++ b/project.cpp @@ -264,6 +264,37 @@ extern "C" Configuration CrafterBuildProject(std::span a scrollImpls.emplace_back("tests/MouseScroll/main"); sc.GetInterfacesAndImplementations(ifaces, scrollImpls); cfg.tests.push_back(std::move(scrollTest)); + + // Issue #40: multi-frame-in-flight frame pacing (per-image fences + // + per-frame semaphores, no steady-state wait-idle). Drives the + // real frame loop against a live Wayland compositor for many more + // frames than there are in-flight slots and asserts the validation + // layer stays silent — the old singleton-semaphore design only + // avoided being an active race because the wait-idle masked it, so + // a clean multi-frame run is the regression guard. Needs the + // Wayland backend + a real compositor, hence inside the !windows + // block alongside MouseScroll. + Test frameLoopTest; + Configuration& fl = frameLoopTest.config; + fl.path = cfg.path; + fl.name = "FrameLoopSync"; + fl.outputName = "FrameLoopSync"; + fl.type = ConfigurationType::Executable; + fl.target = cfg.target; + fl.march = cfg.march; + fl.mtune = cfg.mtune; + fl.debug = cfg.debug; + fl.sysroot = cfg.sysroot; + fl.dependencies = cfg.dependencies; + fl.externalDependencies = cfg.externalDependencies; + fl.compileFlags = cfg.compileFlags; + fl.linkFlags = cfg.linkFlags; + fl.defines = cfg.defines; + fl.cFiles = cfg.cFiles; + std::vector frameLoopImpls(impls.begin(), impls.end()); + frameLoopImpls.emplace_back("tests/FrameLoopSync/main"); + fl.GetInterfacesAndImplementations(ifaces, frameLoopImpls); + cfg.tests.push_back(std::move(frameLoopTest)); } // Issue #36: BLAS build options. Drives the real hardware AS-build diff --git a/tests/FrameLoopSync/main.cpp b/tests/FrameLoopSync/main.cpp new file mode 100644 index 0000000..6a1a78c --- /dev/null +++ b/tests/FrameLoopSync/main.cpp @@ -0,0 +1,126 @@ +/* +Crafter®.Graphics +Copyright (C) 2026 Catcrafts® +catcrafts.net + +This library is free software; you can redistribute it and/or +modify it under the terms of the GNU Lesser General Public +License version 3.0 as published by the Free Software Foundation; + +This library is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public +License along with this library; if not, write to the Free Software +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +// Issue #40: multi-frame-in-flight frame pacing. The renderer used to be +// effectively single-buffered — Render() ended with an unconditional +// vkQueueWaitIdle and shared a single (presentComplete, renderComplete) +// semaphore pair with zero fences for the Window's whole lifetime. This +// reworks the pacing model: +// +// - one VkFence per swapchain slot, keyed by the ACQUIRED IMAGE INDEX, +// passed to vkQueueSubmit and waited on (then reset) before that image's +// command buffer is re-recorded; +// - one semaphore pair per in-flight frame, keyed by a free-running CPU +// frame counter % numFrames (the image index isn't known until acquire +// returns), so frame N's render-complete and frame N+1's acquire can be +// pending at once; +// - the steady-state vkQueueWaitIdle is gone (kept only on resize / +// OUT_OF_DATE / teardown). +// +// The fence and semaphore changes are mutually load-bearing: dropping the +// wait-idle while reusing one binary semaphore pair across in-flight frames is +// a textbook Vulkan race, and per-frame semaphores without per-frame fences is +// a command-buffer use-after-free. So this test drives the *real* frame loop — +// a real Wayland surface, a real swapchain, real acquire/submit/present — for +// many more frames than there are in-flight slots, then asserts: +// +// - every Render() completed and the CPU frame counter advanced by the +// expected amount (no deadlock on the per-image fence wait); +// - the acquired image index rotated across more than one slot (proof the +// swapchain is genuinely multi-buffered, not pinned to image 0); +// - the Vulkan validation layer reported ZERO errors over the whole run. +// This is the load-bearing check: the old singleton-pair design only +// avoided being an active race because the wait-idle masked it. Reusing a +// binary semaphore across overlapping frames, or recording into a command +// buffer still in flight, both light up the validation layer immediately. +// +// Needs a live Wayland compositor + a Vulkan device at runtime (same as the +// windowed examples), so it shares the native build settings and is Linux-only. + +#include "vulkan/vulkan.h" +#include + +import Crafter.Graphics; +import std; + +using namespace Crafter; + +namespace { + +int failures = 0; + +void Check(bool ok, std::string_view what) { + std::println("{} {}", ok ? "PASS" : "FAIL", what); + if (!ok) ++failures; +} + +} // namespace + +int main() { + Device::Initialize(); + + // A real, configured window against the running compositor. Construction + // performs the xdg configure handshake and creates the swapchain + the + // per-frame fences/semaphores under test. + Window window(640, 480, "FrameLoopSync test"); + + // Render() with no passes still exercises the full pacing path: acquire + // (signals frameSemaphores[frame].presentComplete) → per-image fence + // wait+reset → barriers → submit (signals renderComplete, fences the + // command buffer) → present. Run well past numFrames so every semaphore + // slot and every per-image fence is reused several times — exactly the + // condition the old single-pair / wait-idle design could not survive + // without draining the queue every frame. + constexpr int kFrames = 60; + static_assert(kFrames > Window::numFrames * 3, + "must loop enough to reuse each in-flight slot multiple times"); + + std::array imageSeen{}; + for (int i = 0; i < kFrames; ++i) { + window.Render(); + if (window.currentBuffer < Window::numFrames) { + imageSeen[window.currentBuffer] = true; + } + } + + // Drain before inspecting — no steady-state wait-idle means GPU work may + // still be in flight when the loop exits. + Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + + Check(window.frameCounter == static_cast(kFrames), + std::format("CPU frame counter advanced once per Render() ({} of {})", + window.frameCounter, kFrames)); + + int distinctImages = 0; + for (bool seen : imageSeen) if (seen) ++distinctImages; + Check(distinctImages > 1, + std::format("swapchain rotated across multiple images ({} of {} slots used)", + distinctImages, Window::numFrames)); + + Check(Device::validationErrorCount == 0, + std::format("no Vulkan validation errors across {} frames ({} seen)", + kFrames, Device::validationErrorCount)); + + if (failures != 0) { + std::println("{} check(s) failed", failures); + return EXIT_FAILURE; + } + std::println("all checks passed"); + return EXIT_SUCCESS; +}