diff --git a/implementations/Crafter.Graphics-Window.cpp b/implementations/Crafter.Graphics-Window.cpp index 3891db7..bee7f82 100644 --- a/implementations/Crafter.Graphics-Window.cpp +++ b/implementations/Crafter.Graphics-Window.cpp @@ -482,12 +482,14 @@ Window::Window(std::uint32_t width, std::uint32_t height) : width(width), height .layerCount = VK_REMAINING_ARRAY_LAYERS, }; - // Transition into VK_IMAGE_LAYOUT_GENERAL before recording passes. image - // and oldLayout are patched per frame (oldLayout depends on first use). + // Transition into VK_IMAGE_LAYOUT_GENERAL before recording passes. image, + // oldLayout (depends on first use) and dstAccessMask (derived from the + // per-pass stage union) are patched per frame in Render(); the dstAccessMask + // here is just a well-formed default. acquireBarrier = { .sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER, .srcAccessMask = 0, - .dstAccessMask = VK_ACCESS_SHADER_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT, + .dstAccessMask = VK_ACCESS_SHADER_WRITE_BIT, .oldLayout = VK_IMAGE_LAYOUT_UNDEFINED, .newLayout = VK_IMAGE_LAYOUT_GENERAL, .srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED, @@ -496,7 +498,9 @@ Window::Window(std::uint32_t width, std::uint32_t height) : width(width), height .subresourceRange = range, }; - // Transition back to PRESENT_SRC_KHR after the passes. Only image varies. + // Transition back to PRESENT_SRC_KHR after the passes. image and + // srcAccessMask (derived from the per-pass stage union) are patched per + // frame in Render(); the srcAccessMask here is just a well-formed default. presentBarrier = { .sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER, .srcAccessMask = VK_ACCESS_SHADER_WRITE_BIT, @@ -835,6 +839,13 @@ void Window::Render() { // barrier below (passes don't change within a Render()). const VkPipelineStageFlags swapWriterStages = SwapchainStageUnion(passes); + // dstAccessMask must only contain access flags supported by the dst stage + // mask (VUID-vkCmdPipelineBarrier-pImageMemoryBarriers-02820). Derive it + // from the same per-pass stage union the barrier uses as its dst stage mask + // instead of hardcoding SHADER_WRITE|TRANSFER_WRITE, which would carry + // TRANSFER_WRITE into a compute-only frame's COMPUTE_SHADER dst stage. + acquireBarrier.dstAccessMask = SwapchainWriterAccess(swapWriterStages); + // The acquire->GENERAL transition only needs to be visible before the first // pass writes the image, i.e. at the writer stages — not ALL_COMMANDS. vkCmdPipelineBarrier(drawCmdBuffers[currentBuffer], VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT, swapWriterStages, 0, 0, nullptr, 0, nullptr, 1, &acquireBarrier); @@ -919,6 +930,12 @@ void Window::Render() { presentBarrier.image = images[currentBuffer]; + // Mirror of the acquire barrier: srcAccessMask must be supported by the src + // stage mask (swapWriterStages). SHADER_WRITE alone is valid for COMPUTE/RT + // and so never fires today, but a transfer-stage writer would need + // TRANSFER_WRITE here too — derive both from the same union for symmetry. + presentBarrier.srcAccessMask = SwapchainWriterAccess(swapWriterStages); + // The ->PRESENT_SRC transition only needs to wait on the stages that // actually wrote the image (the per-pass union), not ALL_COMMANDS; present // itself is gated by the render-finished semaphore, so dst stays BOTTOM. @@ -1107,6 +1124,20 @@ void Window::CreateSwapchain() VkCommandBuffer Window::StartInit() { VkCommandBufferBeginInfo cmdBufInfo {}; cmdBufInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; + // StartInit reuses drawCmdBuffers[currentBuffer] — the same buffer the + // steady-state Render() loop submits, guarded by waitFences[currentBuffer]. + // When setup runs mid-session (e.g. building map meshes / acceleration + // structures on a scene transition) the loop's last submission of this + // buffer may still be in flight: re-beginning it would trip + // VUID-vkBeginCommandBuffer-commandBuffer-00049, re-submitting it + // VUID-vkQueueSubmit-pCommandBuffers-00071, and any resource the caller + // destroys between here and FinishInit could still be referenced by that + // pending submission (vkDestroyBuffer-in-use). Drain the queue first so the + // buffer is free to re-record and no in-flight frame still references the + // resources this setup is about to replace (issue #153). Setup is rare + // (not the per-frame path), so a full wait-idle here is fine — FinishInit + // already wait-idles at the end. + Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); Device::CheckVkResult(vkBeginCommandBuffer(drawCmdBuffers[currentBuffer], &cmdBufInfo)); // The swapchain images are deliberately NOT transitioned here. They are @@ -1132,6 +1163,11 @@ void Window::FinishInit() { VkCommandBuffer Window::GetCmd() { VkCommandBufferBeginInfo cmdBufInfo {}; cmdBufInfo.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO; + // Same lifecycle hazard as StartInit: GetCmd re-begins and EndCmd + // re-submits the shared drawCmdBuffers[currentBuffer], so drain any + // in-flight submission of it (and free the resources it references) before + // re-recording (issue #153). + Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); Device::CheckVkResult(vkBeginCommandBuffer(drawCmdBuffers[currentBuffer], &cmdBufInfo)); VkImageSubresourceRange range{}; diff --git a/interfaces/Crafter.Graphics-RenderPass.cppm b/interfaces/Crafter.Graphics-RenderPass.cppm index c354336..3cae95d 100644 --- a/interfaces/Crafter.Graphics-RenderPass.cppm +++ b/interfaces/Crafter.Graphics-RenderPass.cppm @@ -69,6 +69,26 @@ export namespace Crafter { return stages; } + // The access mask matching a swapchain-writer stage union, for the + // frame-edge barriers (acquire dst / present src). A barrier's access mask + // must only contain access flags supported by its accompanying stage mask + // (VUID-vkCmdPipelineBarrier-pImageMemoryBarriers-02820): TRANSFER_WRITE is + // not a valid access for COMPUTE/RAY_TRACING stages, so hardcoding both + // SHADER_WRITE and TRANSFER_WRITE fires the VUID every frame on an + // all-compute frame. The swapchain image is written as a storage image by + // compute/RT passes (SHADER_WRITE) and only via TRANSFER_WRITE when a + // transfer-stage pass is present, so derive the access bits from the same + // per-pass stage union the barrier's stage mask uses instead of hardcoding. + inline VkAccessFlags SwapchainWriterAccess(VkPipelineStageFlags stages) { + VkAccessFlags access = 0; + if (stages & (VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT + | VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR)) + access |= VK_ACCESS_SHADER_WRITE_BIT; + if (stages & VK_PIPELINE_STAGE_TRANSFER_BIT) + access |= VK_ACCESS_TRANSFER_WRITE_BIT; + return access; + } + // The inter-pass swapchain barrier (replacing the old queue-wide // VkMemoryBarrier at the #115 location). Scoped to the swapchain image's // single colour subresource so only that image's shader caches round-trip — diff --git a/project.cpp b/project.cpp index 82fc66b..97fb4de 100644 --- a/project.cpp +++ b/project.cpp @@ -296,6 +296,41 @@ extern "C" Configuration CrafterBuildProject(std::span a frameLoopImpls.emplace_back("tests/FrameLoopSync/main"); fl.GetInterfacesAndImplementations(ifaces, frameLoopImpls); cfg.tests.push_back(std::move(frameLoopTest)); + + // Issue #153: two frame-loop validation errors. (1) The acquire + // barrier hardcoded dstAccessMask = SHADER_WRITE|TRANSFER_WRITE but + // used the per-pass stage union (COMPUTE_SHADER for an all-compute + // frame) as its dst stage, so TRANSFER_WRITE was unsupported and + // VUID-02820 fired every frame — FrameLoopSync can't see it because + // it runs with no passes (the union falls back to the conservative + // writer set, which includes TRANSFER). (2) Mid-session + // StartInit/FinishInit reuse the shared draw command buffer while + // the loop's last submission of it is still in flight, re-beginning + // (00049) and re-submitting (00071) a pending buffer. Drives a real + // compute pass through the loop plus interleaved StartInit rounds + // and asserts the layer stays silent. Needs the Wayland backend + a + // real compositor, so it lives in the !windows block. + Test setupReuseTest; + Configuration& sr = setupReuseTest.config; + sr.path = cfg.path; + sr.name = "SetupCmdBufferReuse"; + sr.outputName = "SetupCmdBufferReuse"; + sr.type = ConfigurationType::Executable; + sr.target = cfg.target; + sr.march = cfg.march; + sr.mtune = cfg.mtune; + sr.debug = cfg.debug; + sr.sysroot = cfg.sysroot; + sr.dependencies = cfg.dependencies; + sr.externalDependencies = cfg.externalDependencies; + sr.compileFlags = cfg.compileFlags; + sr.linkFlags = cfg.linkFlags; + sr.defines = cfg.defines; + sr.cFiles = cfg.cFiles; + std::vector setupReuseImpls(impls.begin(), impls.end()); + setupReuseImpls.emplace_back("tests/SetupCmdBufferReuse/main"); + sr.GetInterfacesAndImplementations(ifaces, setupReuseImpls); + cfg.tests.push_back(std::move(setupReuseTest)); } // Issue #36: BLAS build options. Drives the real hardware AS-build diff --git a/tests/SetupCmdBufferReuse/main.cpp b/tests/SetupCmdBufferReuse/main.cpp new file mode 100644 index 0000000..6e634a7 --- /dev/null +++ b/tests/SetupCmdBufferReuse/main.cpp @@ -0,0 +1,131 @@ +/* +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 #153: two distinct validation errors the frame loop emitted, both +// reproduced here by driving the *real* loop against a live compositor and +// asserting the Vulkan validation layer stays silent — same harness idea as +// FrameLoopSync (the silent layer is the load-bearing check). +// +// Problem 1 — per-frame acquire-barrier access/stage mismatch. The +// acquire->GENERAL barrier hardcoded dstAccessMask = +// SHADER_WRITE | TRANSFER_WRITE but used the per-pass stage union as its dst +// stage mask. For an all-compute frame the union narrows to COMPUTE_SHADER, +// which does NOT support TRANSFER_WRITE, so VUID-02820 fired every frame. +// FrameLoopSync runs with NO passes, where the union falls back to the +// conservative writer union (which DOES include TRANSFER) and the VUID never +// fires — so it cannot catch this. The key here is a real compute pass that +// narrows SwapchainStage() to COMPUTE_SHADER, exactly like UIRenderer. +// +// Problem 2 — mid-session StartInit/FinishInit reuse the shared +// drawCmdBuffers[currentBuffer]. With no steady-state wait-idle the loop's +// last submission of that buffer is still in flight when setup runs, so the +// old code re-began it (VUID-vkBeginCommandBuffer-commandBuffer-00049) and +// re-submitted it (VUID-vkQueueSubmit-pCommandBuffers-00071) while pending, +// and any resource freed in the StartInit..FinishInit bracket could still be +// referenced by that submission. StartInit/GetCmd now drain the queue first. +// +// Needs a live Wayland compositor + a Vulkan device at runtime (same as the +// windowed examples / FrameLoopSync), 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; +} + +// A minimal compute pass: records nothing, but narrows its swapchain write +// stage to COMPUTE_SHADER like every real menu/game pass. That narrowing is +// what shrinks the frame's stage union to COMPUTE_SHADER and so exposes the +// problem-1 acquire-barrier access/stage mismatch. Record() needs no work — +// the bug is in the frame-edge barriers the loop records around the passes, +// not in the pass body. +struct ComputePass : RenderPass { + void Record(GraphicsCommandBuffer, std::uint32_t, Window&) override {} + VkPipelineStageFlags SwapchainStage() const override { + return VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT; + } +}; + +} // namespace + +int main() { + Device::Initialize(); + + Window window(640, 480, "SetupCmdBufferReuse test"); + + ComputePass pass; + window.passes.push_back(&pass); + + // ── Problem 1: all-compute frames ─────────────────────────────────────── + // Run enough frames that the acquire barrier is recorded many times with + // swapWriterStages == COMPUTE_SHADER. Pre-fix this logged VUID-02820 once + // per frame. + constexpr int kWarmupFrames = 8; + for (int i = 0; i < kWarmupFrames; ++i) window.Render(); + + Check(Device::validationErrorCount == 0, + std::format("no validation errors over {} all-compute frames " + "(acquire-barrier access/stage match) ({} seen)", + kWarmupFrames, Device::validationErrorCount)); + + // ── Problem 2: mid-session setup while a frame is in flight ────────────── + // After Render() returns there is no steady-state wait-idle, so the last + // submission of drawCmdBuffers[currentBuffer] is still pending. Calling + // StartInit()/FinishInit() now (as a scene transition would, to build map + // meshes / acceleration structures) reuses that exact command buffer. The + // setup buffer is left empty on purpose: it's the begin/submit lifecycle + // around the in-flight buffer that the old code got wrong, not the + // contents. Interleave more frames so each StartInit again races a fresh + // in-flight submission. + constexpr int kSetupRounds = 3; + for (int r = 0; r < kSetupRounds; ++r) { + VkCommandBuffer cmd = window.StartInit(); + Check(cmd != VK_NULL_HANDLE, "StartInit returns a usable command buffer"); + window.FinishInit(); + for (int f = 0; f < 4; ++f) window.Render(); + } + + Check(Device::validationErrorCount == 0, + std::format("no validation errors across {} mid-session StartInit/" + "FinishInit rounds interleaved with rendering ({} seen)", + kSetupRounds, Device::validationErrorCount)); + + // Drain before teardown — no steady-state wait-idle means GPU work may + // still be in flight when the loop exits. + Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + + if (failures != 0) { + std::println("{} check(s) failed", failures); + return EXIT_FAILURE; + } + std::println("all checks passed"); + return EXIT_SUCCESS; +} diff --git a/tests/SwapchainBarrierScope/main.cpp b/tests/SwapchainBarrierScope/main.cpp index eb5e4e3..5c3357e 100644 --- a/tests/SwapchainBarrierScope/main.cpp +++ b/tests/SwapchainBarrierScope/main.cpp @@ -165,6 +165,45 @@ int main() { "inter-pass barrier is scoped to the single swapchain subresource"); } + // ─── SwapchainWriterAccess (issue #153) ───────────────────────────────── + // The frame-edge (acquire dst / present src) barriers must set an access + // mask supported by their accompanying stage mask + // (VUID-vkCmdPipelineBarrier-pImageMemoryBarriers-02820). The old code + // hardcoded SHADER_WRITE|TRANSFER_WRITE on a barrier whose stage mask is the + // per-pass union, so an all-compute frame carried TRANSFER_WRITE into a + // COMPUTE_SHADER dst stage and fired the VUID every frame. The fix derives + // the access from the same stage union, so each stage only pulls in the + // access flags it actually supports. + { + // All-compute frame: COMPUTE_SHADER supports SHADER_WRITE only — and + // crucially NOT TRANSFER_WRITE (the bit that fired the VUID). + VkAccessFlags compute = SwapchainWriterAccess(VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT); + Check(compute == VK_ACCESS_SHADER_WRITE_BIT, + "compute-only writer access is SHADER_WRITE only"); + Check((compute & VK_ACCESS_TRANSFER_WRITE_BIT) == 0, + "compute-only writer access does NOT include TRANSFER_WRITE (the VUID-02820 trap)"); + + // RT frame: RAY_TRACING_SHADER also writes via SHADER_WRITE, no transfer. + Check(SwapchainWriterAccess(VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR) == VK_ACCESS_SHADER_WRITE_BIT, + "RT-only writer access is SHADER_WRITE only"); + + // A transfer-stage writer pulls in TRANSFER_WRITE. + Check(SwapchainWriterAccess(VK_PIPELINE_STAGE_TRANSFER_BIT) == VK_ACCESS_TRANSFER_WRITE_BIT, + "transfer-only writer access is TRANSFER_WRITE only"); + + // The conservative writer union (un-overridden / empty frame) folds in + // both — and every bit it sets is supported by some stage in the union. + Check(SwapchainWriterAccess(kSwapchainWriterStages) + == (VK_ACCESS_SHADER_WRITE_BIT | VK_ACCESS_TRANSFER_WRITE_BIT), + "conservative writer union access is SHADER_WRITE | TRANSFER_WRITE"); + + // Mixed compute+RT (no transfer): still SHADER_WRITE only, no transfer. + VkAccessFlags mixed = SwapchainWriterAccess( + VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT | VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR); + Check(mixed == VK_ACCESS_SHADER_WRITE_BIT, + "compute+RT writer access is SHADER_WRITE only (no spurious TRANSFER_WRITE)"); + } + if (failures != 0) { std::println("{} check(s) failed", failures); return EXIT_FAILURE;