From 24834a6478198f9f15fc5d8dccd46fe64517ed6f Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 28 Jul 2026 10:47:52 +0200 Subject: [PATCH] =?UTF-8?q?fix(render):=20Campaign=20V=20slice=20V6g=20?= =?UTF-8?q?=E2=80=94=20the=20Vulkan=20frame=20stops=20lying=20to=20the=20d?= =?UTF-8?q?river?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V6f ran the bring-up host once under VK_LAYER_KHRONOS_validation and found seven VUIDs, every one of them on the path any world frame takes (plan §5.5.7). This closes all of them, plus a fourth defect in the same log that §5.5.7 did not call out. The host now runs validation-clean: zero errors and zero warnings over 39,855 frames. Nothing outside Gpu/Vk/ is touched, so the GL backend executes not one changed statement. The offline pixel gate says so too — 4.08e-05 differing fraction against f8dbe2ee, which is exactly the value the campaign recorded as its own same-commit control (§5.1's 15–23 pixel band). The dynamic-descriptor limit was a decision, not a patch. V6b declared all ten of set 0's bindings STORAGE_BUFFER_DYNAMIC on the reasoning that the contract lets a renderer bind any range per draw. That is true and still cost nothing to honour for four of them: a dynamic descriptor buys exactly one thing, the ability to address the SAME buffer at a DIFFERENT offset without a descriptor write, which is the shape of a ring allocation and of nothing else. So the ring-fed bindings — instances, batches, clip slots, instance light sets — stay dynamic, and the ones pointing at a long-lived buffer written whole and bound once per pass carry their offset in the descriptor instead. Binding 9 is the clearest of those: it is the GL-only uvec2 handle table, which the Vulkan backend never binds at all. That lands on four dynamic storage descriptors. The RX 9070 XT allows eight, so eight would have worked here — but four is Vulkan's GUARANTEED minimum, which means no conformant device can fail this layout, and V9's lavapipe row and the deferred physical Linux row both depend on that. The count is asserted against maxDescriptorSetStorageBuffersDynamic in the capability record, so a device that cannot serve it is rejected at startup in the report under the same exit-code-4 contract as every other requirement, rather than failing silently at vkCreatePipelineLayout the way this one did. Depth-off pipelines were malformed in any pass that has depth. Dynamic rendering bakes the depth/stencil attachment format into the pipeline and requires it to equal the pass's; V6c set it only when the pipeline itself tested or wrote depth. Debug lines, the retained UI and the sky are all depth-off and all composite over the main pass, so this was not an edge case. The same GpuPipelineDescription is legitimately used both ways — ui-text opens its own depth-less pass — so the description cannot answer the question and the backend builds both variants, binding whichever matches what vkCmdBeginRendering was actually handed rather than what the pass asked for. Both are built at startup against the persisted cache, so no frame compiles one. A slice entitled to change the contract should add a depth-format field the way V6d added ColorFormat; this is the honest expression of the gap until then. vk-backbuffer-depth and vk-backbuffer-msaa-color were created UNDEFINED and never moved. Both now barrier on every backbuffer pass — from UNDEFINED on the first use after Configure, from attachment-optimal with a write-after-write dependency thereafter. The dependency matters on its own account, not just the layout: two passes in one frame write both images and so does the next frame, and Vulkan orders nothing between render-pass instances. The fourth defect is the one worth reading twice. CaptureBackbuffer transitioned the LAST PRESENTED swapchain image to TRANSFER_SRC and copied out of it. After vkQueuePresentKHR that image belongs to the presentation engine and its contents are not ours to read — and the pixels were usually right, which is precisely the problem. This campaign spent three sections of its own plan (§5.5.1–§5.5.3) discovering how much a capture instrument that is "usually right" can cost, and shipping that shape on the new backend would have made every Vulkan PNG, and the V7 differential built on them, formally undefined. The frame now copies its own output into a host-readable buffer while it still owns the image, and the capture reads that. Retention is opt-in, armed when an artifact directory exists: one full-resolution copy per frame is worth nothing to a player and is the entire instrument to a gate. The old one-shot command pool, device-idle wait and per-capture readback buffer go with it. Two gaps found and recorded in §5.5.8 rather than fixed, both outside this slice's brief. UniformSkyParams (set 1, binding 4) is not in the uniform set layout, so whoever first draws sky on Vulkan must add it. And a binding pointed at two different buffers within one frame silently corrupts the earlier draws, on dynamic and plain descriptors alike, because descriptor contents are read at execution time — no consumer does that today, but WbDrawDispatcher and EnvCellRenderer each own their own instance and batch buffers and both bind bindings 0, 1, 3, 4 and 5 in one frame, so the Vulkan world arm has to know before it is written. Gates: Release build; App tests 4,075 passed / 3 skipped (baseline 4,073 + the two new capability cases); GL offline pixel gate PASS at 4.08e-05; one validation-layer Vulkan run, clean, with the captured PNG inspected and correct in orientation, colour and glyph coverage. Co-Authored-By: Claude Fable 5 --- docs/plans/2026-07-27-vulkan-campaign.md | 79 +++++ .../Rendering/Gpu/Vk/VulkanBringUpHost.cs | 8 +- .../Gpu/Vk/VulkanCapabilityRecord.cs | 32 ++ .../Rendering/Gpu/Vk/VulkanFrameBindings.cs | 118 +++++-- .../Gpu/Vk/VulkanGpuDevice.Resources.cs | 298 +++++++++++------- .../Rendering/Gpu/Vk/VulkanGpuDevice.cs | 28 +- .../Rendering/Gpu/Vk/VulkanGpuPassEncoder.cs | 14 +- .../Rendering/Gpu/Vk/VulkanGpuPipeline.cs | 73 ++++- .../Rendering/Gpu/Vk/VulkanGpuRenderTarget.cs | 33 ++ .../Rendering/Gpu/Vk/VulkanInterop.cs | 2 + .../Rendering/Gpu/Vk/VulkanPipelineLayouts.cs | 85 ++++- .../Gpu/Vk/VulkanCapabilityGateTests.cs | 58 ++++ 12 files changed, 653 insertions(+), 175 deletions(-) diff --git a/docs/plans/2026-07-27-vulkan-campaign.md b/docs/plans/2026-07-27-vulkan-campaign.md index 9216b9f8..fe1fb7f7 100644 --- a/docs/plans/2026-07-27-vulkan-campaign.md +++ b/docs/plans/2026-07-27-vulkan-campaign.md @@ -572,6 +572,7 @@ dialect slice. **V6f closed all three** (the third was the frag's GL-only `sampler2DArray(handle)` construction), so 8/9 pairs now compile. `mesh` is a tenth pair with no consumer at all; see the V6e report. +| **V6g** | The four Vulkan validation defects §5.5.7 and its log left open: the dynamic-descriptor split (an architect decision, §5.5.8 item 1), per-pass depth-format pipeline variants, first-use backbuffer attachment layout transitions, and a backbuffer capture that no longer reads a presented swapchain image. Confined to `Gpu/Vk/`; the GL backend executes not one changed statement. | validation-clean bring-up run (0 errors / 0 warnings over 39,855 frames, against 7 VUIDs + 1 UNASSIGNED at the parent), App tests, GL offline pixel gate 4.08e-05 — its own same-commit control value | | **V7** | GL-versus-Vulkan differential: `tools/run-backend-differential-gate.ps1`, strict paired-PNG compare, divergences fixed in the Vulkan backend only, then lifecycle + R6 soak natively on Vulkan, one validation-layer-clean run, one RenderDoc capture. **Milestone: parity.** | every differential checkpoint passes; both connected routes green on VK | | **V8** | Perf gate on the RX 9070 XT, uncapped, both backends, same route. | §2 acceptance table; parity is the floor | | **V9** | Linux + CI: X11/Wayland surfaces; a `linux-vulkan` job on lavapipe (probe accepts on a real 1.3 software device, a short real render under xvfb, forced-unsupported → exit 4, `.spv` freshness). Physical Linux GPU row deferred post-cutover, as for Slice L. | CI green including the new job | @@ -1116,6 +1117,84 @@ and sky with no scenery or statics, and would be the first real evidence the Vulkan world path works. V6f's shader work is the whole of that path's shader prerequisite. +#### 5.5.8 V6g (2026-07-28): the validation defects are closed, and a fourth was found + +§5.5.7's step 2 — "the three validation defects, since every one of them is on +the path any world frame takes" — is done, and the Vulkan bring-up host now runs +**validation-clean**: zero errors and zero warnings across a 39,855-frame run +with `VK_LAYER_KHRONOS_validation` loaded, against the same run that produced +seven of them at `f8dbe2ee`. + +**1. The dynamic-descriptor limit (`VUID-VkPipelineLayoutCreateInfo-descriptorType-03032` +/ `-pSetLayouts-03040`).** Resolved by decision rather than patch, as §5.5.7 asked. +V6b declared all ten of set 0's bindings `STORAGE_BUFFER_DYNAMIC`; the rule now is +that **a dynamic descriptor is for ring-fed data whose offset moves, and nothing +else**. Instances (0), batches (1), clip slots (3) and instance light sets (5) stay +dynamic; global lights (4), clip regions (2), instance indoor (6), alpha (7), +selection lighting (8) and the GL-only texture table (9) become plain +`STORAGE_BUFFER` carrying their offset in the descriptor. That is **four** dynamic +storage descriptors — not merely under the RX 9070 XT's 8 but exactly Vulkan's +guaranteed minimum, so no conformant device can fail the layout, which is what V9's +lavapipe row and the deferred physical Linux row depend on. The count is asserted +against `maxDescriptorSetStorageBuffersDynamic` in the capability record, so a +device that cannot serve it is rejected at startup under the exit-code-4 contract +instead of failing at `vkCreatePipelineLayout`. Bindings 6–8 are per-instance +arrays grouped with the frame-global tables because their owner writes them whole +once per frame; if the Vulkan world path needs one re-pointed per draw, promoting +it back is one line, with four unused dynamic slots to promote into. + +**2. Depth-off pipelines in depth-carrying passes +(`VUID-vkCmdDraw-dynamicRenderingUnusedAttachments-08914` / `-08917`).** The +backend now builds **two variants of every pipeline** — one declaring the pass's +depth/stencil format, one declaring `UNDEFINED` — and binds whichever matches what +`vkCmdBeginRendering` was actually handed. The same `GpuPipelineDescription` is +legitimately used in both kinds of pass (`ui-text` opens its own depth-less pass; +the world pass it composites over has depth), so the description genuinely cannot +answer the question. **A later slice entitled to change the contract should add a +depth-format field the way V6d added `ColorFormat`**; until then, materialising +both at startup against the persisted cache is the honest expression of the gap and +no frame ever compiles one. + +**3. Missing first-use layout transitions +(`VUID-vkCmdBeginRendering-pRenderingInfo-09588` / `-09590` / `-09592`).** +`vk-backbuffer-depth` and `vk-backbuffer-msaa-color` are created UNDEFINED and were +never moved. Both now get a barrier on every backbuffer pass: from UNDEFINED on the +first use after `Configure`, and from the attachment-optimal layout with a +write-after-write dependency thereafter — the same shape the swapchain image +already had. The dependency matters independently of the layout: two passes in one +frame write both images, and so does the next frame, with no implicit ordering +between render-pass instances. + +**4. The capture path read an image it did not own +(`UNASSIGNED-non-acquired-swapchain-image-used`).** Present in V6f's log and not +called out there. `CaptureBackbuffer` transitioned the **last presented** swapchain +image to `TRANSFER_SRC` and copied out of it; after `vkQueuePresentKHR` that image +belongs to the presentation engine and its contents are not the application's to +read. The pixels were usually right — which is exactly what makes it +unacceptable. **This campaign spent §5.5.1–§5.5.3 discovering what a capture +instrument that is "usually right" costs**, and shipping the same shape on the new +backend would have made every Vulkan PNG, and the V7 differential built on them, +formally undefined. The frame now copies its own output into a host-readable buffer +while it still owns the image, and `CaptureBackbuffer` reads that. Retention is +opt-in (armed when an artifact directory exists) because it costs one full-res +image-to-buffer copy per frame: worth nothing to a player, and the entire +instrument to a gate. + +**Two gaps recorded, not fixed** — both outside this slice's brief, both real: + +- **`UniformSkyParams` (set 1, binding 4) is not in the uniform set layout**, which + declares only bindings 1 and 3, and `VulkanFrameBindings.UniformBindingCount` is + 4, so `SetUniform(4, …)` throws before it can be wrong. The `sky` pair compiles to + SPIR-V declaring that binding, so whoever first draws sky on Vulkan must add it. +- **A binding pointed at two different buffers within one frame silently corrupts + the earlier draws**, on dynamic and plain descriptors alike: `SetStorage` rewrites + the descriptor when the buffer changes, and descriptor contents are read at + execution time, not record time. No consumer does this today. The world path + will: `WbDrawDispatcher` and `EnvCellRenderer` each own their own instance and + batch buffers and both bind bindings 0, 1, 3, 4 and 5 in one frame. **The Vulkan + world arm needs one descriptor set per renderer, or per-renderer sub-ranges of one + buffer, and it needs to know that before it is written.** + ### 5.4 The null-target `BeginPass` divergence (V4c) — must be undone at V6 V4c had to stop GL's `BeginPass` from binding framebuffer 0 when a pass declares diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanBringUpHost.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanBringUpHost.cs index 2969fccb..632745f5 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanBringUpHost.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanBringUpHost.cs @@ -375,7 +375,13 @@ internal sealed unsafe class VulkanBringUpHost : IDisposable _debugNames, new SwapchainBackbuffer(_swapchain!, _presentQueue), ShaderSpirvDirectory(), - _platform.Paths.CacheDirectory); + _platform.Paths.CacheDirectory, + // Slice V6g: a frame can only be read back while it still owns its + // swapchain image, so retention has to be armed before the first + // frame rather than at the moment a screenshot is asked for. Armed + // exactly when an artifact directory exists, which is what a gate + // run has and a player run does not. + retainBackbufferCapture: !string.IsNullOrWhiteSpace(_options.AutomationArtifactDirectory)); // Four samples where the device allows it, so the backbuffer pass really // resolves rather than rendering straight into the swapchain image. diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanCapabilityRecord.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanCapabilityRecord.cs index 6374f8a6..d5e218f6 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanCapabilityRecord.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanCapabilityRecord.cs @@ -157,6 +157,22 @@ internal sealed record VulkanDeviceLimitSupport /// Sets 0, 1 and 2 are all bound simultaneously, so at least 3. public required uint MaxBoundDescriptorSets { get; init; } + /// + /// Must reach . + /// + /// Added at slice V6g. The V6b layout declared all ten of set 0's + /// bindings dynamic and was rejected by this limit on the RX 9070 XT, which + /// allows 8 — silently, at vkCreatePipelineLayout, and only visible + /// with the validation layer loaded (plan §5.5.7 defect 1). Reading the limit + /// into the record means the gate rejects such a device at startup, in the + /// report, under the same exit-code-4 contract as every other requirement, + /// rather than the layout failing at the first pipeline. + /// + public required uint MaxDescriptorSetStorageBuffersDynamic { get; init; } + + /// Must reach the number of dynamic uniform bindings set 1 declares. + public required uint MaxDescriptorSetUniformBuffersDynamic { get; init; } + /// Must reach . public required uint MaxDescriptorSetUpdateAfterBindSampledImages { get; init; } @@ -189,6 +205,10 @@ internal sealed record VulkanDeviceLimitSupport MaxPushConstantsSize = GpuBindingModel.MaxPushConstantBytes, MaxClipDistances = GpuBindingModel.ClipPlanesPerSlot, MaxBoundDescriptorSets = 4, + // Vulkan's guaranteed minimums. That the layout fits inside them is the + // point of slice V6g's split — see VulkanPipelineLayouts. + MaxDescriptorSetStorageBuffersDynamic = 4, + MaxDescriptorSetUniformBuffersDynamic = 8, MaxDescriptorSetUpdateAfterBindSampledImages = GpuBindingModel.TextureTableCapacity, MaxPerStageDescriptorUpdateAfterBindSampledImages = GpuBindingModel.TextureTableCapacity, TimestampComputeAndGraphics = true, @@ -450,6 +470,18 @@ internal static class VulkanCapabilityRequirements $"{VulkanDescriptorSetCount} simultaneously bound descriptor sets are required " + $"(storage, uniform, texture table); this device provides {limits.MaxBoundDescriptorSets}."); } + if (limits.MaxDescriptorSetStorageBuffersDynamic < VulkanPipelineLayouts.DynamicStorageBindingCount) + { + failures.Add( + $"set 0 declares {VulkanPipelineLayouts.DynamicStorageBindingCount} dynamic storage bindings " + + $"(Vulkan guarantees 4); this device provides {limits.MaxDescriptorSetStorageBuffersDynamic}."); + } + if (limits.MaxDescriptorSetUniformBuffersDynamic < VulkanFrameBindings.DynamicUniformBindingCount) + { + failures.Add( + $"set 1 declares {VulkanFrameBindings.DynamicUniformBindingCount} dynamic uniform bindings " + + $"(Vulkan guarantees 8); this device provides {limits.MaxDescriptorSetUniformBuffersDynamic}."); + } if (limits.MaxDescriptorSetUpdateAfterBindSampledImages < GpuBindingModel.TextureTableCapacity) { failures.Add( diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs index 2b1a0ddb..17708b0a 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs @@ -10,11 +10,20 @@ namespace AcDream.App.Rendering.Gpu.Vk; /// and ring allocations mean that range moves every frame. The obvious /// implementation — write a descriptor per bind — would put a /// vkUpdateDescriptorSets in the hot path and reintroduce the exact cost -/// the texture table was designed to remove. So each binding is a +/// the texture table was designed to remove. So each ring-fed binding is a /// *_BUFFER_DYNAMIC descriptor pointing at the whole ring, and the /// per-draw offset travels in vkCmdBindDescriptorSets's dynamic-offset /// array, which is free. /// +/// Not every binding is dynamic. Slice V6g split set 0 by +/// , because ten +/// dynamic storage descriptors exceeded the device limit (plan §5.5.7 defect 1). +/// A plain binding carries its offset in the descriptor itself, so it is +/// rewritten when the range moves rather than when only the buffer changes — and +/// its slot in the dynamic-offset array does not exist. Getting that array's +/// length or ordering wrong is a validation error, so both are derived from the +/// same predicate the layout is built from rather than restated. +/// /// Every binding is always bound, whether a renderer uses it or /// not. Bindings a shader does not declare still need a live descriptor, so /// unused ones point at a shared dummy range. That is what lets there be ONE @@ -39,9 +48,34 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable private bool _disposed; + /// + /// Set 0's dynamic-offset slots, in binding order — the order + /// vkCmdBindDescriptorSets requires. A plain binding has no slot. + /// + private static readonly uint[] DynamicStorageBindings = BuildDynamicStorageBindings(); + + private static uint[] BuildDynamicStorageBindings() + { + var bindings = new List((int)GpuBindingModel.StorageBindingCount); + for (uint binding = 0; binding < GpuBindingModel.StorageBindingCount; binding++) + { + if (VulkanPipelineLayouts.IsDynamicStorageBinding(binding)) + bindings.Add(binding); + } + + return [.. bindings]; + } + /// Bindings 0..3 of set 1; only 1 (SceneLighting) and 3 (terrain tiling) are used. internal const int UniformBindingCount = 4; + /// + /// How many of set 1's bindings the layout actually declares, all dynamic. + /// Asserted against maxDescriptorSetUniformBuffersDynamic by the + /// capability gate; Vulkan guarantees 8, so this is comfortable. + /// + internal const uint DynamicUniformBindingCount = 2; + /// /// Widest range any single binding may address. Dynamic descriptors take a /// static range at write time and slide it with an offset, so this bounds @@ -62,13 +96,19 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable ArgumentNullException.ThrowIfNull(ring); ArgumentNullException.ThrowIfNull(dummy); - DescriptorPoolSize* sizes = stackalloc DescriptorPoolSize[2]; + DescriptorPoolSize* sizes = stackalloc DescriptorPoolSize[3]; sizes[0] = new DescriptorPoolSize { Type = DescriptorType.StorageBufferDynamic, - DescriptorCount = GpuBindingModel.StorageBindingCount, + DescriptorCount = VulkanPipelineLayouts.DynamicStorageBindingCount, }; sizes[1] = new DescriptorPoolSize + { + Type = DescriptorType.StorageBuffer, + DescriptorCount = + GpuBindingModel.StorageBindingCount - VulkanPipelineLayouts.DynamicStorageBindingCount, + }; + sizes[2] = new DescriptorPoolSize { Type = DescriptorType.UniformBufferDynamic, DescriptorCount = UniformBindingCount, @@ -77,7 +117,7 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable { SType = StructureType.DescriptorPoolCreateInfo, MaxSets = 2, - PoolSizeCount = 2, + PoolSizeCount = 3, PPoolSizes = sizes, }; VulkanInterop.Check( @@ -90,7 +130,12 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable for (uint binding = 0; binding < GpuBindingModel.StorageBindingCount; binding++) { _storageBuffers[binding] = dummy.Handle; - WriteStorage(binding, dummy.Handle, (uint)Math.Min(dummy.SizeBytes, MaxBindingRangeBytes)); + WriteStorage( + binding, + dummy.Handle, + offsetBytes: 0, + (uint)Math.Min(dummy.SizeBytes, MaxBindingRangeBytes), + VulkanPipelineLayouts.IsDynamicStorageBinding(binding)); } // Only the two bindings the layout declares exist; the rest of the @@ -108,14 +153,29 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable internal VulkanGpuBuffer Dummy { get; } - /// Points a storage binding at a range, re-writing the descriptor only when the BUFFER changes. + /// + /// Points a storage binding at a range. + /// + /// A DYNAMIC binding re-writes its descriptor only when the BUFFER + /// changes; the offset rides the bind call. A PLAIN binding has no such + /// channel, so the descriptor itself carries the offset and is re-written + /// when either moves. + /// internal void SetStorage(uint binding, VulkanGpuBuffer buffer, uint offsetBytes, uint sizeBytes) { ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(binding, GpuBindingModel.StorageBindingCount); - if (_storageBuffers[binding].Handle != buffer.Handle.Handle) + bool dynamic = VulkanPipelineLayouts.IsDynamicStorageBinding(binding); + bool bufferChanged = _storageBuffers[binding].Handle != buffer.Handle.Handle; + bool offsetChanged = _storageOffsets[binding] != offsetBytes; + if (bufferChanged || (!dynamic && offsetChanged)) { _storageBuffers[binding] = buffer.Handle; - WriteStorage(binding, buffer.Handle, ClampRange(buffer, sizeBytes)); + WriteStorage( + binding, + buffer.Handle, + dynamic ? 0 : offsetBytes, + ClampRange(buffer, sizeBytes, dynamic ? 0 : offsetBytes), + dynamic); } _storageOffsets[binding] = offsetBytes; @@ -127,7 +187,7 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable if (_uniformBuffers[binding].Handle != buffer.Handle.Handle) { _uniformBuffers[binding] = buffer.Handle; - WriteUniform(binding, buffer.Handle, Math.Min(ClampRange(buffer, sizeBytes), 65536)); + WriteUniform(binding, buffer.Handle, Math.Min(ClampRange(buffer, sizeBytes, offsetBytes: 0), 65536)); } _uniformOffsets[binding] = offsetBytes; @@ -141,13 +201,14 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable sets[1] = _uniformSet; sets[2] = device.TextureTable.Set; - int dynamicCount = _storageOffsets.Length + 2; + int dynamicCount = DynamicStorageBindings.Length + 2; uint* offsets = stackalloc uint[dynamicCount]; - for (int i = 0; i < _storageOffsets.Length; i++) - offsets[i] = _storageOffsets[i]; - // Dynamic offsets are ordered by set, then by binding number. - offsets[_storageOffsets.Length + 0] = _uniformOffsets[GpuBindingModel.UniformSceneLighting]; - offsets[_storageOffsets.Length + 1] = _uniformOffsets[GpuBindingModel.UniformTerrainTiling]; + // Dynamic offsets are ordered by set, then by binding number, and only + // the DYNAMIC descriptors have a slot at all. + for (int i = 0; i < DynamicStorageBindings.Length; i++) + offsets[i] = _storageOffsets[DynamicStorageBindings[i]]; + offsets[DynamicStorageBindings.Length + 0] = _uniformOffsets[GpuBindingModel.UniformSceneLighting]; + offsets[DynamicStorageBindings.Length + 1] = _uniformOffsets[GpuBindingModel.UniformTerrainTiling]; _vk.CmdBindDescriptorSets( commands, @@ -160,9 +221,19 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable offsets); } - private static uint ClampRange(VulkanGpuBuffer buffer, uint requested) + private static uint ClampRange(VulkanGpuBuffer buffer, uint requested, uint offsetBytes) { - uint available = (uint)Math.Min(buffer.SizeBytes, MaxBindingRangeBytes); + long remaining = buffer.SizeBytes - offsetBytes; + if (remaining <= 0) + { + throw new ArgumentOutOfRangeException( + nameof(offsetBytes), + offsetBytes, + $"A storage binding was pointed past the end of its {buffer.SizeBytes}-byte buffer. " + + "A descriptor range of zero is not representable in Vulkan."); + } + + uint available = (uint)Math.Min(remaining, MaxBindingRangeBytes); return requested == 0 ? available : Math.Min(Math.Max(requested, 16), available); } @@ -182,12 +253,17 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable return set; } - private void WriteStorage(uint binding, Silk.NET.Vulkan.Buffer buffer, uint rangeBytes) + private void WriteStorage( + uint binding, + Silk.NET.Vulkan.Buffer buffer, + uint offsetBytes, + uint rangeBytes, + bool dynamic) { var info = new DescriptorBufferInfo { Buffer = buffer, - Offset = 0, + Offset = offsetBytes, Range = rangeBytes, }; var write = new WriteDescriptorSet @@ -196,7 +272,9 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable DstSet = _storageSet, DstBinding = binding, DescriptorCount = 1, - DescriptorType = DescriptorType.StorageBufferDynamic, + DescriptorType = dynamic + ? DescriptorType.StorageBufferDynamic + : DescriptorType.StorageBuffer, PBufferInfo = &info, }; _vk.UpdateDescriptorSets(_device, 1, &write, 0, null); diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs index 3ef4cbb9..753fd2ea 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs @@ -148,6 +148,8 @@ internal sealed unsafe partial class VulkanGpuDevice _bindingDummy?.Dispose(); _bindingDummy = null; + _captureBuffer?.Dispose(); + _captureBuffer = null; _defaultTexture?.Dispose(); _defaultTexture = null; @@ -192,8 +194,11 @@ internal sealed unsafe partial class VulkanGpuDevice /// extent and the requested sample count. Called by the host after a /// swapchain create or recreate, behind a device-idle wait. /// - internal void ConfigureBackbufferAttachments(uint width, uint height, Format colorFormat, int sampleCount) => + internal void ConfigureBackbufferAttachments(uint width, uint height, Format colorFormat, int sampleCount) + { BackbufferAttachments.Configure(width, height, colorFormat, sampleCount); + ConfigureBackbufferCapture(width, height); + } public IGpuTexture CreateTexture(in GpuTextureDescription description) { @@ -406,6 +411,7 @@ internal sealed unsafe partial class VulkanGpuDevice { colorView = attachments.ColorView; resolveView = _backbuffer.ViewAt(imageIndex); + TransitionBackbufferScratchColor(commands, attachments); } else { @@ -413,7 +419,10 @@ internal sealed unsafe partial class VulkanGpuDevice } if (description.Depth is not null && attachments.HasDepth) + { depthView = attachments.DepthView; + TransitionBackbufferDepth(commands, attachments); + } } else { @@ -498,7 +507,8 @@ internal sealed unsafe partial class VulkanGpuDevice _frameBindings[frame.SlotIndex], description, width, - height); + height, + hasDepthAttachment: depthView.Handle != 0); _openPass = encoder; return encoder; } @@ -583,6 +593,60 @@ internal sealed unsafe partial class VulkanGpuDevice _vk.CmdPipelineBarrier2(commands, &dependency); } + /// + /// Campaign V slice V6g: moves the multisampled colour scratch into + /// COLOR_ATTACHMENT_OPTIMAL before the pass that names it there. + /// + /// The first use after (re)creation starts from UNDEFINED — the image + /// genuinely has no contents, and saying so lets the driver skip a + /// decompress. Every later use starts from the layout the previous pass left + /// and needs the barrier for its write-after-write dependency instead: two + /// passes in one frame both write this image, and so does the next frame, + /// with no implicit ordering between render-pass instances. + /// + private void TransitionBackbufferScratchColor( + CommandBuffer commands, + VulkanBackbufferAttachments attachments) + { + bool first = !attachments.ColorLayoutInitialized; + attachments.MarkColorLayoutInitialized(); + TransitionImage( + commands, + attachments.ColorImage, + ImageAspectFlags.ColorBit, + first ? ImageLayout.Undefined : ImageLayout.ColorAttachmentOptimal, + ImageLayout.ColorAttachmentOptimal, + first ? PipelineStageFlags2.TopOfPipeBit : PipelineStageFlags2.ColorAttachmentOutputBit, + first ? AccessFlags2.None : AccessFlags2.ColorAttachmentWriteBit, + PipelineStageFlags2.ColorAttachmentOutputBit, + AccessFlags2.ColorAttachmentWriteBit | AccessFlags2.ColorAttachmentReadBit); + } + + /// + /// The same, for the transient depth/stencil buffer. Both aspects move + /// together because the image carries both and the pass names it as both a + /// depth and a stencil attachment. + /// + private void TransitionBackbufferDepth( + CommandBuffer commands, + VulkanBackbufferAttachments attachments) + { + bool first = !attachments.DepthLayoutInitialized; + attachments.MarkDepthLayoutInitialized(); + const PipelineStageFlags2 DepthStages = + PipelineStageFlags2.EarlyFragmentTestsBit | PipelineStageFlags2.LateFragmentTestsBit; + TransitionImage( + commands, + attachments.DepthImage, + ImageAspectFlags.DepthBit | ImageAspectFlags.StencilBit, + first ? ImageLayout.Undefined : ImageLayout.DepthStencilAttachmentOptimal, + ImageLayout.DepthStencilAttachmentOptimal, + first ? PipelineStageFlags2.TopOfPipeBit : DepthStages, + first ? AccessFlags2.None : AccessFlags2.DepthStencilAttachmentWriteBit, + DepthStages, + AccessFlags2.DepthStencilAttachmentWriteBit | AccessFlags2.DepthStencilAttachmentReadBit); + } + private void TransitionRenderTargetForRendering(CommandBuffer commands, VulkanGpuRenderTarget target) { TransitionImage( @@ -670,12 +734,29 @@ internal sealed unsafe partial class VulkanGpuDevice } /// - /// Reads the presented image back as tightly packed top-left-origin RGBA8. + /// Reads the last presented frame back as tightly packed top-left-origin + /// RGBA8. /// /// The swapchain is B8G8R8A8_UNORM (plan §4.9), so the channels /// are swizzled on the CPU to preserve FrameScreenshotController's /// RGBA byte contract — the same seam every automated screenshot gate already /// uses, so the comparison tooling is unaffected by the backend swap. + /// + /// Campaign V slice V6g: it reads a device-owned copy, not the + /// swapchain image. V6c transitioned the LAST PRESENTED swapchain image + /// to TRANSFER_SRC and copied out of it, which the validation layer + /// rejects as UNASSIGNED-non-acquired-swapchain-image-used: once + /// vkQueuePresentKHR has taken an image, the presentation engine owns + /// it and its contents are not the application's to read. The pixels were + /// usually right, which is precisely what makes it dangerous — this campaign + /// spent three sections (§5.5.1–§5.5.3) discovering how much a capture + /// instrument that is "usually right" can cost. So the frame copies its own + /// output into a host-readable buffer while it still owns the image, and this + /// method reads that. + /// + /// Retention is opt-in and off in production: it costs one full-res + /// image-to-buffer copy per frame, which is worth nothing to a player and is + /// the entire instrument to a gate. /// public byte[] CaptureBackbuffer(int width, int height) { @@ -684,19 +765,98 @@ internal sealed unsafe partial class VulkanGpuDevice ArgumentOutOfRangeException.ThrowIfNegativeOrZero(height); if (_backbuffer is null) throw new InvalidOperationException("This device has no backbuffer to capture."); + if (_captureBuffer is null) + { + throw new InvalidOperationException( + "Backbuffer capture was not retained by this device. Construct it with " + + "retainBackbufferCapture: true — reading the presented swapchain image " + + "instead is a Vulkan usage error (see this method's remarks)."); + } + if (width != _captureWidth || height != _captureHeight) + { + throw new ArgumentException( + $"The retained capture is {_captureWidth}x{_captureHeight}; {width}x{height} was requested. " + + "The capture buffer is sized with the swapchain, so a mismatch means the caller " + + "and the backbuffer disagree about the frame that was just presented."); + } - return CaptureImage(_backbuffer.ImageAt(_lastPresentedImageIndex), (uint)width, (uint)height); + // Everything that could still be writing the buffer is a submitted frame. + VulkanInterop.Check(_vk.DeviceWaitIdle(_device), "vkDeviceWaitIdle (capture)"); + var pixels = new byte[(long)_captureWidth * _captureHeight * 4]; + _captureBuffer.Read(0, pixels); + // ToRgba, NOT ToGlOriginRgba: IGpuDevice.CaptureBackbuffer is documented + // as top-left-origin, and a Vulkan image already is. + return VulkanBackbufferSwizzle.ToRgba(pixels, width, height, width * 4); } - private uint _lastPresentedImageIndex; - private bool _backbufferRenderingReady; - - private byte[] CaptureImage(Image image, uint width, uint height) + /// + /// Records the acquired image's contents into the retained capture buffer, + /// while the frame still owns the image. Returns the layout the image is + /// left in, which the present barrier has to start from. + /// + internal ImageLayout RecordBackbufferCapture(CommandBuffer commands, Image image) { - uint byteCount = width * height * 4; - VulkanInterop.Check(_vk.DeviceWaitIdle(_device), "vkDeviceWaitIdle (capture)"); + if (_captureBuffer is null || _backbuffer is null) + return ImageLayout.ColorAttachmentOptimal; + if (_captureWidth != _backbuffer.Width || _captureHeight != _backbuffer.Height) + return ImageLayout.ColorAttachmentOptimal; - var readback = new VulkanGpuBuffer( + TransitionImage( + commands, + image, + ImageAspectFlags.ColorBit, + ImageLayout.ColorAttachmentOptimal, + ImageLayout.TransferSrcOptimal, + PipelineStageFlags2.ColorAttachmentOutputBit, + AccessFlags2.ColorAttachmentWriteBit, + PipelineStageFlags2.CopyBit, + AccessFlags2.TransferReadBit); + + var region = new BufferImageCopy + { + BufferOffset = 0, + BufferRowLength = 0, + BufferImageHeight = 0, + ImageSubresource = new ImageSubresourceLayers + { + AspectMask = ImageAspectFlags.ColorBit, + MipLevel = 0, + BaseArrayLayer = 0, + LayerCount = 1, + }, + ImageOffset = new Offset3D(0, 0, 0), + ImageExtent = new Extent3D(_captureWidth, _captureHeight, 1), + }; + _vk.CmdCopyImageToBuffer( + commands, + image, + ImageLayout.TransferSrcOptimal, + _captureBuffer.Handle, + 1, + ®ion); + return ImageLayout.TransferSrcOptimal; + } + + /// + /// Sizes the retained capture buffer with the swapchain. Called from + /// , which the host already + /// drives behind a vkDeviceWaitIdle on resize. + /// + private void ConfigureBackbufferCapture(uint width, uint height) + { + if (!_retainBackbufferCapture) + return; + if (_captureBuffer is not null && _captureWidth == width && _captureHeight == height) + return; + + _captureBuffer?.Dispose(); + _captureBuffer = null; + _captureWidth = width; + _captureHeight = height; + if (width == 0 || height == 0) + return; + + _captureBuffer = new VulkanGpuBuffer( _vk, _device, _allocator, @@ -705,116 +865,14 @@ internal sealed unsafe partial class VulkanGpuDevice _debugNames, new GpuBufferDescription( "vk-backbuffer-capture", - byteCount, + width * height * 4, GpuBufferUsage.TransferDestination, GpuMemoryResidency.HostReadable)); - CommandPool pool = default; - try - { - var poolCreate = new CommandPoolCreateInfo - { - SType = StructureType.CommandPoolCreateInfo, - QueueFamilyIndex = _graphicsFamily, - Flags = CommandPoolCreateFlags.TransientBit, - }; - VulkanInterop.Check( - _vk.CreateCommandPool(_device, &poolCreate, null, out pool), - "vkCreateCommandPool (capture)"); - var allocate = new CommandBufferAllocateInfo - { - SType = StructureType.CommandBufferAllocateInfo, - CommandPool = pool, - Level = CommandBufferLevel.Primary, - CommandBufferCount = 1, - }; - VulkanInterop.Check( - _vk.AllocateCommandBuffers(_device, &allocate, out CommandBuffer commands), - "vkAllocateCommandBuffers (capture)"); - - var begin = new CommandBufferBeginInfo - { - SType = StructureType.CommandBufferBeginInfo, - Flags = CommandBufferUsageFlags.OneTimeSubmitBit, - }; - VulkanInterop.Check(_vk.BeginCommandBuffer(commands, &begin), "vkBeginCommandBuffer (capture)"); - - TransitionImage( - commands, - image, - ImageAspectFlags.ColorBit, - ImageLayout.PresentSrcKhr, - ImageLayout.TransferSrcOptimal, - PipelineStageFlags2.AllCommandsBit, - AccessFlags2.None, - PipelineStageFlags2.CopyBit, - AccessFlags2.TransferReadBit); - - var region = new BufferImageCopy - { - BufferOffset = 0, - BufferRowLength = 0, - BufferImageHeight = 0, - ImageSubresource = new ImageSubresourceLayers - { - AspectMask = ImageAspectFlags.ColorBit, - MipLevel = 0, - BaseArrayLayer = 0, - LayerCount = 1, - }, - ImageOffset = new Offset3D(0, 0, 0), - ImageExtent = new Extent3D(width, height, 1), - }; - _vk.CmdCopyImageToBuffer( - commands, - image, - ImageLayout.TransferSrcOptimal, - readback.Handle, - 1, - ®ion); - - TransitionImage( - commands, - image, - ImageAspectFlags.ColorBit, - ImageLayout.TransferSrcOptimal, - ImageLayout.PresentSrcKhr, - PipelineStageFlags2.CopyBit, - AccessFlags2.TransferReadBit, - PipelineStageFlags2.AllCommandsBit, - AccessFlags2.None); - - VulkanInterop.Check(_vk.EndCommandBuffer(commands), "vkEndCommandBuffer (capture)"); - - var commandSubmit = new CommandBufferSubmitInfo - { - SType = StructureType.CommandBufferSubmitInfo, - CommandBuffer = commands, - }; - var submit = new SubmitInfo2 - { - SType = StructureType.SubmitInfo2, - CommandBufferInfoCount = 1, - PCommandBufferInfos = &commandSubmit, - }; - VulkanInterop.Check(_vk.QueueSubmit2(_graphicsQueue, 1, &submit, default), "vkQueueSubmit2 (capture)"); - VulkanInterop.Check(_vk.QueueWaitIdle(_graphicsQueue), "vkQueueWaitIdle (capture)"); - - var pixels = new byte[byteCount]; - readback.Read(0, pixels); - // ToRgba, NOT ToGlOriginRgba: IGpuDevice.CaptureBackbuffer is - // documented as top-left-origin, and a Vulkan image already is. - // (VulkanSwapchain.CaptureImage feeds FrameScreenshotController - // instead, which flips again on the way to the PNG, so THAT path - // flips here to cancel. Two consumers, two conventions, one - // difference — worth stating because a single wrong choice produces - // a perfectly plausible upside-down screenshot.) - return VulkanBackbufferSwizzle.ToRgba(pixels, (int)width, (int)height, (int)width * 4); - } - finally - { - if (pool.Handle != 0) - _vk.DestroyCommandPool(_device, pool, null); - readback.Dispose(); - } } + + private readonly bool _retainBackbufferCapture; + private VulkanGpuBuffer? _captureBuffer; + private uint _captureWidth; + private uint _captureHeight; + private bool _backbufferRenderingReady; } diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs index 5d392341..bb6b732f 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs @@ -117,8 +117,13 @@ internal sealed unsafe partial class VulkanGpuDevice : IGpuDevice string? shaderSpirvDirectory = null, string? pipelineCacheDirectory = null, int ringCapacityBytesPerSlot = DefaultRingCapacityBytesPerSlot, - int framesInFlight = VulkanFrameFlightController.DefaultFramesInFlight) + int framesInFlight = VulkanFrameFlightController.DefaultFramesInFlight, + bool retainBackbufferCapture = false) { + // Slice V6g. Off by default because it costs a full-resolution + // image-to-buffer copy per frame; on for the automated gates, which is + // the only legal way to read a frame back — see CaptureBackbuffer. + _retainBackbufferCapture = retainBackbufferCapture; _vk = vk ?? throw new ArgumentNullException(nameof(vk)); ArgumentNullException.ThrowIfNull(features); ArgumentNullException.ThrowIfNull(limits); @@ -406,7 +411,12 @@ internal sealed unsafe partial class VulkanGpuDevice : IGpuDevice if (_acquiredImageIndex is { } imageIndex && _backbuffer is not null) { - TransitionBackbufferForPresent(commands, _backbuffer.ImageAt(imageIndex)); + Image presentable = _backbuffer.ImageAt(imageIndex); + // Slice V6g: the ONLY point at which this frame's output may legally + // be read is here, while the image is still acquired. After the + // present below it belongs to the presentation engine. + ImageLayout current = RecordBackbufferCapture(commands, presentable); + TransitionBackbufferForPresent(commands, presentable, current); } VulkanInterop.Check(_vk.EndCommandBuffer(commands), "vkEndCommandBuffer (frame)"); @@ -461,7 +471,6 @@ internal sealed unsafe partial class VulkanGpuDevice : IGpuDevice if (_acquiredImageIndex is { } toPresent && _backbuffer is not null) { - _lastPresentedImageIndex = toPresent; PresentSucceeded = _backbuffer.Present(toPresent); _acquiredImageIndex = null; } @@ -470,16 +479,21 @@ internal sealed unsafe partial class VulkanGpuDevice : IGpuDevice /// False after a present that reported the swapchain should be rebuilt. internal bool PresentSucceeded { get; private set; } = true; - private void TransitionBackbufferForPresent(CommandBuffer commands, Image image) + private void TransitionBackbufferForPresent(CommandBuffer commands, Image image, ImageLayout currentLayout) { + bool captured = currentLayout == ImageLayout.TransferSrcOptimal; var barrier = new ImageMemoryBarrier2 { SType = StructureType.ImageMemoryBarrier2, - SrcStageMask = PipelineStageFlags2.ColorAttachmentOutputBit, - SrcAccessMask = AccessFlags2.ColorAttachmentWriteBit, + SrcStageMask = captured + ? PipelineStageFlags2.CopyBit + : PipelineStageFlags2.ColorAttachmentOutputBit, + SrcAccessMask = captured + ? AccessFlags2.TransferReadBit + : AccessFlags2.ColorAttachmentWriteBit, DstStageMask = PipelineStageFlags2.BottomOfPipeBit, DstAccessMask = AccessFlags2.None, - OldLayout = ImageLayout.ColorAttachmentOptimal, + OldLayout = currentLayout, NewLayout = ImageLayout.PresentSrcKhr, SrcQueueFamilyIndex = Silk.NET.Vulkan.Vk.QueueFamilyIgnored, DstQueueFamilyIndex = Silk.NET.Vulkan.Vk.QueueFamilyIgnored, diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPassEncoder.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPassEncoder.cs index 8a041beb..48cec39e 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPassEncoder.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPassEncoder.cs @@ -29,6 +29,7 @@ internal sealed unsafe class VulkanGpuPassEncoder : IGpuPassEncoder private readonly CommandBuffer _commands; private readonly VulkanFrameBindings _bindings; private readonly uint _attachmentHeight; + private readonly bool _hasDepthAttachment; private VulkanGpuPipeline? _pipeline; private bool _closed; @@ -40,13 +41,19 @@ internal sealed unsafe class VulkanGpuPassEncoder : IGpuPassEncoder VulkanFrameBindings bindings, GpuPassDescription pass, uint attachmentWidth, - uint attachmentHeight) + uint attachmentHeight, + bool hasDepthAttachment) { _device = device; _frame = frame; _commands = commands; _bindings = bindings; _attachmentHeight = attachmentHeight; + // Slice V6g: what vkCmdBeginRendering was actually handed, not what the + // description asked for. A backbuffer pass that requests depth before + // the attachments exist gets none, and the pipeline variant has to agree + // with the command buffer rather than with the intent. + _hasDepthAttachment = hasDepthAttachment; Pass = pass; // A pass always starts with the whole attachment drawable. GL's @@ -68,7 +75,10 @@ internal sealed unsafe class VulkanGpuPassEncoder : IGpuPassEncoder throw new ArgumentException("The Vulkan backend can only bind a Vulkan pipeline.", nameof(pipeline)); _pipeline = vulkanPipeline; - _device.Api.CmdBindPipeline(_commands, PipelineBindPoint.Graphics, vulkanPipeline.Handle); + _device.Api.CmdBindPipeline( + _commands, + PipelineBindPoint.Graphics, + vulkanPipeline.HandleFor(_hasDepthAttachment)); // Every pipeline shares one layout, so the descriptor sets and push // constants bound earlier in the pass survive this call. That is the diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPipeline.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPipeline.cs index f2ed1bf3..7eee3dce 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPipeline.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuPipeline.cs @@ -17,12 +17,34 @@ namespace AcDream.App.Rendering.Gpu.Vk; /// core in 1.3. That is what lets a pass be described by /// alone rather than by an object that has to be /// created, cached and matched. +/// +/// Two variants, selected at bind time (slice V6g). Dynamic +/// rendering bakes the depth/stencil attachment FORMAT into the pipeline, and it +/// must equal the format of the pass the pipeline draws in — UNDEFINED +/// when the pass has no depth attachment, the real format when it has one. V6c +/// declared the format only when the pipeline itself tested or wrote depth, +/// which made every depth-off pipeline malformed the moment it drew inside a +/// depth-carrying pass. That is not an edge case: debug lines, the retained UI +/// and the sky are all depth-off and all draw inside the main pass, and plan +/// §5.5.7 recorded it firing as +/// VUID-vkCmdDraw-dynamicRenderingUnusedAttachments-08914/-08917. +/// +/// The same is legitimately used in +/// both kinds of pass — ui-text opens a depth-less pass of its own, and +/// the world pass it composites over has depth — so the description cannot +/// answer the question and the backend builds both. The contract could grow a +/// depth-format field the way it grew +/// at V6d; until a slice is entitled to change the contract, materialising both +/// is the honest expression of the gap. Both are built at startup against the +/// persisted cache, so no frame ever compiles one. /// internal sealed unsafe class VulkanGpuPipeline : IGpuPipeline { private readonly Silk.NET.Vulkan.Vk _vk; private readonly Device _device; private readonly IGpuResourceRetirementQueue _retirement; + private readonly Pipeline _withDepthAttachment; + private readonly Pipeline _withoutDepthAttachment; private bool _disposed; internal VulkanGpuPipeline( @@ -181,12 +203,8 @@ internal sealed unsafe class VulkanGpuPipeline : IGpuPipeline SType = StructureType.PipelineRenderingCreateInfo, ColorAttachmentCount = 1, PColorAttachmentFormats = &color, - DepthAttachmentFormat = description.Depth.Test || description.Depth.Write - ? depthStencilFormat - : Format.Undefined, - StencilAttachmentFormat = description.Depth.Test || description.Depth.Write - ? depthStencilFormat - : Format.Undefined, + DepthAttachmentFormat = depthStencilFormat, + StencilAttachmentFormat = depthStencilFormat, }; var create = new GraphicsPipelineCreateInfo @@ -210,10 +228,26 @@ internal sealed unsafe class VulkanGpuPipeline : IGpuPipeline }; VulkanInterop.Check( - _vk.CreateGraphicsPipelines(_device, cache, 1, &create, null, out Pipeline pipeline), - $"vkCreateGraphicsPipelines ('{description.Name}')"); - Handle = pipeline; - debugNames.NamePipeline(pipeline, description.Name); + _vk.CreateGraphicsPipelines(_device, cache, 1, &create, null, out Pipeline withDepth), + $"vkCreateGraphicsPipelines ('{description.Name}', depth attachment)"); + _withDepthAttachment = withDepth; + debugNames.NamePipeline(withDepth, description.Name); + + rendering.DepthAttachmentFormat = Format.Undefined; + rendering.StencilAttachmentFormat = Format.Undefined; + try + { + VulkanInterop.Check( + _vk.CreateGraphicsPipelines(_device, cache, 1, &create, null, out Pipeline withoutDepth), + $"vkCreateGraphicsPipelines ('{description.Name}', no depth attachment)"); + _withoutDepthAttachment = withoutDepth; + debugNames.NamePipeline(withoutDepth, $"{description.Name}-nodepth"); + } + catch + { + _vk.DestroyPipeline(_device, withDepth, null); + throw; + } } finally { @@ -223,15 +257,28 @@ internal sealed unsafe class VulkanGpuPipeline : IGpuPipeline public GpuPipelineDescription Description { get; } - internal Pipeline Handle { get; } + /// + /// The variant whose declared depth/stencil format matches the open pass. + /// Binding the wrong one is undefined behaviour that only a validation layer + /// reports, which is why the caller is never allowed to guess: the value + /// comes from whether vkCmdBeginRendering was handed a depth image + /// view, not from what the pass description asked for. + /// + internal Pipeline HandleFor(bool passHasDepthAttachment) => + passHasDepthAttachment ? _withDepthAttachment : _withoutDepthAttachment; public void Dispose() { if (_disposed) return; _disposed = true; - Pipeline handle = Handle; - _retirement.Retire(() => _vk.DestroyPipeline(_device, handle, null)); + Pipeline withDepth = _withDepthAttachment; + Pipeline withoutDepth = _withoutDepthAttachment; + _retirement.Retire(() => + { + _vk.DestroyPipeline(_device, withDepth, null); + _vk.DestroyPipeline(_device, withoutDepth, null); + }); } } diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuRenderTarget.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuRenderTarget.cs index 68842377..60f22c15 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuRenderTarget.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuRenderTarget.cs @@ -164,6 +164,35 @@ internal sealed unsafe class VulkanBackbufferAttachments : IDisposable internal bool HasDepth => _depthView.Handle != 0; + /// + /// Whether each attachment has been moved out of VK_IMAGE_LAYOUT_UNDEFINED + /// since it was last (re)created. + /// + /// Campaign V slice V6g. Both images are created with an UNDEFINED + /// initial layout and both are named in vkCmdBeginRendering as + /// COLOR_ATTACHMENT_OPTIMAL / DEPTH_STENCIL_ATTACHMENT_OPTIMAL, + /// and nothing ever moved them — plan §5.5.7 defect 3, reported as + /// VUID-vkCmdBeginRendering-pRenderingInfo-09588/-09590/-09592. The + /// swapchain image had a transition from the start; these two were simply + /// missed, because the swapchain's is acquired per frame and these are not. + /// + /// + /// The flag is per (re)creation rather than per frame: after a pass + /// ends, dynamic rendering leaves the image in the layout the pass declared, + /// so only the first use after starts from UNDEFINED. + /// Every later use still needs a barrier — for the write-after-write + /// dependency between frames, not for the layout — which is why the caller + /// issues one either way and only the old layout and source masks differ. + /// + /// + internal bool ColorLayoutInitialized { get; private set; } + + internal bool DepthLayoutInitialized { get; private set; } + + internal void MarkColorLayoutInitialized() => ColorLayoutInitialized = true; + + internal void MarkDepthLayoutInitialized() => DepthLayoutInitialized = true; + /// /// Rebuilds both attachments for a new extent, format or sample count. /// A no-op when nothing changed, so the host may call it every frame. @@ -280,6 +309,10 @@ internal sealed unsafe class VulkanBackbufferAttachments : IDisposable _depthView = default; _depthImage = default; _depthAllocation = default; + // A recreated image is a new image in UNDEFINED layout, whatever the old + // one had reached. + ColorLayoutInitialized = false; + DepthLayoutInitialized = false; } public void Dispose() diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanInterop.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanInterop.cs index df359f5d..1f74bf7b 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanInterop.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanInterop.cs @@ -362,6 +362,8 @@ internal static unsafe class VulkanPhysicalDeviceInspector MaxPushConstantsSize = limits.MaxPushConstantsSize, MaxClipDistances = limits.MaxClipDistances, MaxBoundDescriptorSets = limits.MaxBoundDescriptorSets, + MaxDescriptorSetStorageBuffersDynamic = limits.MaxDescriptorSetStorageBuffersDynamic, + MaxDescriptorSetUniformBuffersDynamic = limits.MaxDescriptorSetUniformBuffersDynamic, MaxDescriptorSetUpdateAfterBindSampledImages = indexing.MaxDescriptorSetUpdateAfterBindSampledImages, MaxPerStageDescriptorUpdateAfterBindSampledImages = diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs index 8bc862c8..a4ac876f 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanPipelineLayouts.cs @@ -82,19 +82,78 @@ internal static unsafe class VulkanPipelineLayouts } /// - /// Set 0 — the ten storage bindings pins. + /// Campaign V slice V6g: which of the ten storage bindings gets a DYNAMIC + /// descriptor, and why not all of them. /// - /// DYNAMIC storage buffers, because the RHI contract lets a renderer - /// bind an arbitrary range per draw and ring allocations move that range - /// every frame. A non-dynamic descriptor would have to be rewritten each - /// time, putting a vkUpdateDescriptorSets in the hot path — exactly the cost - /// the texture table was designed to remove. The dynamic offset travels in - /// vkCmdBindDescriptorSets instead, which is free. + /// V6b declared all ten STORAGE_BUFFER_DYNAMIC, on the reasoning + /// that the contract lets a renderer bind an arbitrary range per draw. That + /// met a real device limit the first time a validation layer looked at it: + /// maxDescriptorSetStorageBuffersDynamic is 8 on the RX 9070 XT and + /// only 4 at Vulkan's guaranteed minimum, so ten was never portable — + /// see plan §5.5.7 defect 1. /// - /// Ten dynamic storage descriptors is above Vulkan`s guaranteed - /// minimum of four, so this is a real requirement rather than a free choice. - /// It is asserted at layout creation, which fails loudly at startup on a - /// device that cannot serve it rather than at the first draw. + /// The rule. A dynamic descriptor buys exactly one thing: the + /// ability to address the SAME buffer at a DIFFERENT offset without a + /// descriptor write. That is the shape of a per-frame ring allocation, so + /// the bindings a renderer feeds from the ring stay dynamic and the offset + /// travels in vkCmdBindDescriptorSets for free. Bindings that point at + /// a long-lived, renderer-owned buffer written whole and bound once per pass + /// buy nothing from it, and each one costs a scarce device resource. + /// + /// Four dynamic descriptors is not merely under the RX 9070 XT's 8 — it + /// is exactly Vulkan's guaranteed minimum, so no device that can run acdream + /// at all can fail this layout. That matters for slice V9's lavapipe row and + /// for whatever Linux driver the deferred physical row eventually uses. + /// + /// Binding 9 is the clearest case. The texture table is the + /// GL-only uvec2 handle-buffer emulation; the Vulkan backend binds set + /// 2 instead and never touches binding 9 at all, so a dynamic descriptor for + /// it would be a device resource spent on a binding that is provably never + /// bound. + /// + /// What to do if V4c disagrees. Bindings 6, 7 and 8 are + /// per-instance arrays grouped here with the frame-global tables because + /// their owner writes them whole once per frame. If the Vulkan world path + /// turns out to re-point one of them at a moving ring offset per draw, + /// promoting it back is one line here plus one in + /// — and there are four unused dynamic + /// slots to promote into before the guaranteed minimum is exceeded. + /// + internal static bool IsDynamicStorageBinding(uint binding) => binding switch + { + // Per-frame ring uploads: the instance transform array, the per-draw + // batch table, and the two arrays the world dispatcher chunks alongside + // instances. + GpuBindingModel.StorageInstances => true, + GpuBindingModel.StorageBatches => true, + GpuBindingModel.StorageClipSlots => true, + GpuBindingModel.StorageInstanceLightSets => true, + _ => false, + }; + + /// + /// How many of set 0's bindings are dynamic. Asserted against + /// maxDescriptorSetStorageBuffersDynamic by the capability gate, so a + /// device that cannot serve the layout is rejected at startup with the + /// exit-code-4 contract rather than at vkCreatePipelineLayout. + /// + internal static uint DynamicStorageBindingCount { get; } = CountDynamicStorageBindings(); + + private static uint CountDynamicStorageBindings() + { + uint count = 0; + for (uint binding = 0; binding < GpuBindingModel.StorageBindingCount; binding++) + { + if (IsDynamicStorageBinding(binding)) + count++; + } + + return count; + } + + /// + /// Set 0 — the ten storage bindings pins, split + /// between dynamic and plain by . /// internal static DescriptorSetLayout CreateStorageSetLayout(Silk.NET.Vulkan.Vk vk, Device device) { @@ -105,7 +164,9 @@ internal static unsafe class VulkanPipelineLayouts bindings[i] = new DescriptorSetLayoutBinding { Binding = (uint)i, - DescriptorType = DescriptorType.StorageBufferDynamic, + DescriptorType = IsDynamicStorageBinding((uint)i) + ? DescriptorType.StorageBufferDynamic + : DescriptorType.StorageBuffer, DescriptorCount = 1, StageFlags = ShaderStageFlags.VertexBit | ShaderStageFlags.FragmentBit, }; diff --git a/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanCapabilityGateTests.cs b/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanCapabilityGateTests.cs index e209563c..e6ca461b 100644 --- a/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanCapabilityGateTests.cs +++ b/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanCapabilityGateTests.cs @@ -193,6 +193,64 @@ public sealed class VulkanCapabilityGateTests StringComparison.Ordinal)); } + /// + /// Campaign V slice V6g, plan §5.5.7 defect 1. The V6b layout declared all + /// ten of set 0's bindings STORAGE_BUFFER_DYNAMIC and was rejected by + /// the RX 9070 XT's limit of 8 — silently, and only visible under the + /// validation layer. The split has to stay inside Vulkan's GUARANTEED + /// minimum, not merely inside one device's: at four, no conformant + /// implementation can fail the layout, which is what slice V9's lavapipe row + /// and the deferred physical Linux row depend on. + /// + [Fact] + public void TheDynamicStorageBindingSplitFitsVulkansGuaranteedMinimum() + { + const uint VulkanGuaranteedMinimum = 4; + Assert.True( + VulkanPipelineLayouts.DynamicStorageBindingCount <= VulkanGuaranteedMinimum, + $"set 0 declares {VulkanPipelineLayouts.DynamicStorageBindingCount} dynamic storage " + + $"bindings; Vulkan only guarantees {VulkanGuaranteedMinimum}."); + + // The bindings that stay dynamic are the ones a renderer feeds from the + // per-frame ring; the rest point at long-lived buffers bound once per + // pass and buy nothing from a dynamic offset. + Assert.True(VulkanPipelineLayouts.IsDynamicStorageBinding(GpuBindingModel.StorageInstances)); + Assert.True(VulkanPipelineLayouts.IsDynamicStorageBinding(GpuBindingModel.StorageBatches)); + Assert.False(VulkanPipelineLayouts.IsDynamicStorageBinding(GpuBindingModel.StorageGlobalLights)); + Assert.False(VulkanPipelineLayouts.IsDynamicStorageBinding(GpuBindingModel.StorageClipRegions)); + + // Binding 9 is the GL-only uvec2 handle-table emulation. The Vulkan + // backend binds set 2 instead and never touches it, so spending a scarce + // dynamic descriptor on it would be spending one on a binding that is + // provably never bound. + Assert.False(VulkanPipelineLayouts.IsDynamicStorageBinding(GpuBindingModel.StorageTextureTable)); + } + + [Fact] + public void ADeviceWithTooFewDynamicBufferDescriptorsIsRejected() + { + VulkanCapabilityRecord storage = SupportedRecord( + limits: VulkanDeviceLimitSupport.Complete with + { + MaxDescriptorSetStorageBuffersDynamic = + VulkanPipelineLayouts.DynamicStorageBindingCount - 1, + }); + VulkanCapabilityRecord uniform = SupportedRecord( + limits: VulkanDeviceLimitSupport.Complete with + { + MaxDescriptorSetUniformBuffersDynamic = + VulkanFrameBindings.DynamicUniformBindingCount - 1, + }); + + Assert.Contains( + storage.SupportFailures, + failure => failure.Contains("dynamic storage bindings", StringComparison.Ordinal)); + Assert.Contains( + uniform.SupportFailures, + failure => failure.Contains("dynamic uniform bindings", StringComparison.Ordinal)); + Assert.Empty(SupportedRecord().SupportFailures); + } + [Fact] public void ATextureTableSmallerThanTheCapacityIsRejectedPerSetAndPerStage() {