Merge pull request 'fix(window): silence per-frame and setup-path Vulkan validation errors (#153)' (#154) from claude/issue-153 into master

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
+    // Transition into VK_IMAGE_LAYOUT_GENERAL before recording passes. image,
-    // and oldLayout are patched per frame (oldLayout depends on first use).
+    // 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{};

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 —

project.cpp

@@ -296,6 +296,41 @@ extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> 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<fs::path> 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

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 <cstdlib>
+
+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;
+}

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;