diff --git a/docs/plans/2026-07-27-vulkan-campaign.md b/docs/plans/2026-07-27-vulkan-campaign.md index 40897fc1..393be07a 100644 --- a/docs/plans/2026-07-27-vulkan-campaign.md +++ b/docs/plans/2026-07-27-vulkan-campaign.md @@ -1494,6 +1494,109 @@ instance and batch buffers and both bind bindings 0, 1, 3, 4 and 5 in one frame; and the world path's own texture creation still has to reach `IGpuTexture`, because a Vulkan draw cannot sample a GL handle. +#### 5.5.12 V6i-1 (2026-07-28): the descriptor-set hazard is closed before the world arm fires it + +§5.5.8 recorded, and deliberately did not fix, that **a binding pointed at two +different buffers within one frame silently corrupts the earlier draws**: the +backend rewrote the descriptor in place, and a descriptor set's contents are read +when the command buffer EXECUTES, not when it was recorded. Nothing fired it +while the Vulkan frame held only the retained UI. §5.5.11 handed it forward as +the first thing the world arm would hit, because `WbDrawDispatcher`, +`EnvCellRenderer` and `TerrainModernRenderer` each own their own instance, batch +and indirect buffers and all three bind set 0 in one frame. + +It is now closed, and it was closed *first* rather than alongside the world arm, +so that a blank or corrupt Vulkan world frame cannot be this defect wearing +another face. + +**One set pair per renderer scope, derived rather than declared.** There is no +longer a single (set 0, set 1) pair per flight slot; there is an arena of them. +`VulkanBindingScopeArena` — pure bookkeeping, no Vulkan handles, unit-tested — +decides which pair a bind belongs to and whether its descriptors must be written. +The scope key is the descriptor state itself: the ten storage buffer identities +and ranges, the plain (non-dynamic) bindings' offsets, and the two uniform buffer +identities and ranges. + +Deriving the scope is a decision, not an economy. **The pinned contract has no +place to name one** — `BindStorageBuffer` takes a buffer, an offset and a size, +and §3.3 is frozen. Deriving it from state also gives two properties a declared +scope would not: a renderer cannot forget to declare one, and two renderers that +genuinely share every buffer correctly share one pair instead of being told to +differ. A renderer's buffers are stable for its lifetime, so "distinct descriptor +state" *is* "renderer scope". + +**Dynamic offsets stay free.** A ring allocation moving between draws rides +`vkCmdBindDescriptorSets`'s dynamic-offset array, so it costs neither a new pair +nor a descriptor write — §4.4's "zero descriptor writes per frame" property +survives a frame now having more than one binding state in it. A steady frame +rewrites nothing at all: entries are not invalidated at `BeginFrame`, because the +slot's previous submission has retired and its descriptors still say exactly what +this frame is about to say. An entry matched from the previous frame is swapped +below the live cursor, so the rest of the frame cannot take it for a different +state — the property nine tests pin, including the ordering case where two +renderers swap their submission order between frames. + +**Two things the world arm still needs, unchanged from §5.5.11's handoff**, plus +one this slice measured: + +1. **World texture CREATION must reach `IGpuTexture`.** V4t moved the table + ENTRY to the device and left creation with `TerrainAtlas`, + `CompositeTextureArrayCache`, `ManagedGLTextureArray` and `TextureCache`'s + array path. `TerrainAtlas` is the smallest of these and the only one terrain + needs; `TextureAtlasManager`/`ManagedGLTextureArray` is the largest, and it is + what statics, scenery and EnvCell shells sample through. + `ICompositeTextureArrayBackend` is already a seam and takes an RHI backend + directly; `TextureAtlasManager` is not, and reaches `OpenGLGraphicsDevice` + through `CreateTextureArrayInternal`. `BlockCompressionCodec` and + `BlockCompressionMipChain` (V6b) already supply the BC mip chains that path + needs, so the missing piece is an `ITextureArray` implementation over + `IGpuTexture`, not a codec. + +2. **`terrain_modern.vert`'s `TerrainClip` block is in the wrong set for Vulkan, + and it is the same class of gap §5.5.8 recorded for `UniformSkyParams`.** It + is declared `layout(std140, binding = 2) uniform TerrainClip` with **no + `ACDREAM_UBO_SET`**, so under the Vulkan dialect it lands at set 0 binding 2 — + which set 0's layout declares as a STORAGE buffer. GL is unaffected (the macro + expands to nothing and the UBO namespace is separate), and nothing has caught + it because no terrain pipeline has ever been created on Vulkan. Whoever draws + terrain must add the macro, declare binding 2 in the uniform set layout, and + raise `DynamicUniformBindingCount` to 3 — still far under the guaranteed 8. + Worth auditing every remaining shader for the same omission in the same pass. + +3. **The flat-versus-PView question decides how much of the world arm is one + slice.** `WorldSceneRenderer` is already backend-neutral — it takes + `IWorldScenePassExecutor` and `IWorldScenePViewRenderer` as interfaces — so + the Vulkan arm reuses it whole. But `WorldRenderFrame.ClipRoot` is + `Roots.ViewerRoot ?? Buildings.OutdoorNode`, and the offline gate's scene has + buildings, so the offline capture takes the **PView** path, not the flat + safety path. A Vulkan world arm that implements only `WorldScenePassExecutor` + therefore renders nothing in the very scene the pixel gate captures. + `RetailPViewPassExecutor` (685 lines) is on the critical path to the first + Vulkan Dereth PNG, and the "terrain only" intermediate §5.5.7 suggested is + consequently NOT cheaper than it looks. + +**Gate results.** Strict GL offline pixel gate against `b9ab5890`: **1.60e-05** +(9 differing pixels of 563,200), at the low end of the documented 9–31 px band +and 62x under the 0.001 threshold — expected, since no GL file is touched. +GL connected `tools/run-repeat-connected-gate.ps1 -Runs 3`: **3/3 RENDERED** on +the desktop witness and 3/3 on the client capture. One offline Vulkan run with +`VK_LAYER_KHRONOS_validation` **proven inserted by the loader** (`Insert instance +layer "VK_LAYER_KHRONOS_validation"` from `VK_LOADER_DEBUG=layer`): **zero +validation errors and zero warnings**, a captured retained-UI frame, and no +`[shutdown]` diagnostic on either stream. App tests 4,086 / 3 skips (the 4,077 +baseline plus nine); complete Release suite 9,149 / 5. `#250`'s +`SurfaceOverrideFingerprint_DictionaryHotPathAllocatesNothing` failed once in a +whole-suite run and passed run alone, exactly as that issue documents. + +**What this slice deliberately does NOT do**: draw a world. The captured Vulkan +frame is V6h's — the retained UI over the atmosphere fog clear — because no world +renderer exists on the Vulkan arm yet. The arena's multi-scope path is therefore +exercised only by its unit tests; the live Vulkan frame binds one scope and +re-matches it every frame, which is the "rewrites nothing" case. That is the +honest state and it is why the hazard was closed as its own commit: when the +world arm lands and a frame first holds three renderers' buffers, a blank or +corrupt result cannot be this defect. + ### 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/VulkanBindingScopeArena.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanBindingScopeArena.cs new file mode 100644 index 00000000..5ec48d48 --- /dev/null +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanBindingScopeArena.cs @@ -0,0 +1,257 @@ +namespace AcDream.App.Rendering.Gpu.Vk; + +/// +/// Campaign V slice V6i: which descriptor set a bind belongs to, and whether it +/// has to be written. +/// +/// The defect this closes. Plan §5.5.8 recorded, and did not fix, +/// that pointing one binding at a second buffer within a frame silently corrupts +/// the draws recorded before it: vkUpdateDescriptorSets rewrites the set +/// in place, and a set's contents are read when the command buffer EXECUTES, not +/// when it was recorded. Nothing on the Vulkan arm did that while the frame held +/// only the retained UI. The world arm is what fires it — +/// WbDrawDispatcher, EnvCellRenderer and TerrainModernRenderer +/// each own their own instance, batch and indirect buffers and all three bind +/// set 0 in one frame. +/// +/// Why the policy lives here rather than in the renderers. The +/// pinned contract deliberately has no place to name a scope: +/// takes a buffer, an offset and +/// a size. So the backend DERIVES the scope from the descriptor state itself — +/// the buffer identities, plus offset and range for the plain (non-dynamic) +/// bindings whose descriptor carries them. A renderer's buffers are stable for +/// its lifetime, so "distinct descriptor state" is exactly "renderer scope", +/// with two properties a declared scope would not have: a renderer cannot forget +/// to declare one, and two renderers that genuinely share every buffer correctly +/// share one set instead of being told to differ. +/// +/// Dynamic offsets are not part of the state. A ring allocation +/// moving between draws rides vkCmdBindDescriptorSets's dynamic-offset +/// array, so it costs neither a new entry nor a descriptor write. That is what +/// keeps the campaign's "zero descriptor writes per frame" property true now +/// that a frame has more than one binding state in it. +/// +/// Lifetime. Entries below were claimed +/// earlier in the current frame and may have a recorded draw bound to them, so +/// they are never rewritten. Entries above it belong to the previous frame on +/// this flight slot, which has retired, so they are free to reclaim — and are +/// re-matched rather than rewritten when their state still holds, which is the +/// steady-state case. A match above the cursor is swapped down so the remainder +/// of the frame cannot take it for a different state. +/// +/// This type is pure bookkeeping: no Vulkan handles, no allocation of +/// descriptor sets. owns those and asks this +/// class two questions — which index, and does it need writing. +/// +internal sealed class VulkanBindingScopeArena +{ + private readonly int _storageBindingCount; + private readonly int _uniformBindingCount; + private readonly Func _isDynamicStorage; + + private readonly ulong[] _storageBuffers; + private readonly uint[] _storageOffsets; + private readonly uint[] _storageRanges; + private readonly ulong[] _uniformBuffers; + private readonly uint[] _uniformOffsets; + private readonly uint[] _uniformRanges; + + private readonly List _entries = []; + private int _liveCount; + private int _active = -1; + private bool _dirty = true; + + private sealed class Entry(int storageBindingCount, int uniformBindingCount) + { + public ulong[] StorageBuffers { get; } = new ulong[storageBindingCount]; + public uint[] StorageOffsets { get; } = new uint[storageBindingCount]; + public uint[] StorageRanges { get; } = new uint[storageBindingCount]; + public ulong[] UniformBuffers { get; } = new ulong[uniformBindingCount]; + public uint[] UniformRanges { get; } = new uint[uniformBindingCount]; + public int Slot { get; set; } = -1; + } + + internal VulkanBindingScopeArena( + int storageBindingCount, + int uniformBindingCount, + Func isDynamicStorage) + { + ArgumentOutOfRangeException.ThrowIfNegativeOrZero(storageBindingCount); + ArgumentOutOfRangeException.ThrowIfNegativeOrZero(uniformBindingCount); + _storageBindingCount = storageBindingCount; + _uniformBindingCount = uniformBindingCount; + _isDynamicStorage = isDynamicStorage + ?? throw new ArgumentNullException(nameof(isDynamicStorage)); + _storageBuffers = new ulong[storageBindingCount]; + _storageOffsets = new uint[storageBindingCount]; + _storageRanges = new uint[storageBindingCount]; + _uniformBuffers = new ulong[uniformBindingCount]; + _uniformOffsets = new uint[uniformBindingCount]; + _uniformRanges = new uint[uniformBindingCount]; + } + + /// Distinct descriptor states this slot has ever materialised. + internal int Count => _entries.Count; + + /// Entries claimed so far in the current frame; none of these may be rewritten. + internal int LiveCount => _liveCount; + + /// + /// Seeds every binding's initial descriptor state — the shared dummy range + /// each unused binding points at. Called once, before the first frame. + /// + internal void SeedStorage(uint binding, ulong buffer, uint offsetBytes, uint rangeBytes) + { + _storageBuffers[binding] = buffer; + _storageOffsets[binding] = offsetBytes; + _storageRanges[binding] = rangeBytes; + } + + /// + internal void SeedUniform(uint binding, ulong buffer, uint rangeBytes) + { + _uniformBuffers[binding] = buffer; + _uniformRanges[binding] = rangeBytes; + } + + internal void BeginFrame() + { + _liveCount = 0; + _active = -1; + _dirty = true; + } + + /// + /// Records a storage bind. A DYNAMIC binding's descriptor depends on the + /// buffer and range alone; a PLAIN one also carries the offset, so moving it + /// is a descriptor-visible change. + /// + internal void SetStorage(uint binding, ulong buffer, uint offsetBytes, uint rangeBytes) + { + ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(binding, (uint)_storageBindingCount); + bool dynamic = _isDynamicStorage(binding); + if (_storageBuffers[binding] != buffer + || _storageRanges[binding] != rangeBytes + || (!dynamic && _storageOffsets[binding] != offsetBytes)) + { + _storageBuffers[binding] = buffer; + _storageRanges[binding] = rangeBytes; + _dirty = true; + } + + _storageOffsets[binding] = offsetBytes; + } + + internal void SetUniform(uint binding, ulong buffer, uint offsetBytes, uint rangeBytes) + { + ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(binding, (uint)_uniformBindingCount); + if (_uniformBuffers[binding] != buffer || _uniformRanges[binding] != rangeBytes) + { + _uniformBuffers[binding] = buffer; + _uniformRanges[binding] = rangeBytes; + _dirty = true; + } + + _uniformOffsets[binding] = offsetBytes; + } + + internal uint StorageOffset(uint binding) => _storageOffsets[binding]; + + internal uint UniformOffset(uint binding) => _uniformOffsets[binding]; + + internal ulong StorageBuffer(uint binding) => _storageBuffers[binding]; + + internal uint StorageRange(uint binding) => _storageRanges[binding]; + + /// + /// The descriptor offset written INTO a binding's descriptor: zero for a + /// dynamic binding, which slides with the bind call, and the real offset for + /// a plain one. + /// + internal uint StorageDescriptorOffset(uint binding) => + _isDynamicStorage(binding) ? 0u : _storageOffsets[binding]; + + internal ulong UniformBuffer(uint binding) => _uniformBuffers[binding]; + + internal uint UniformRange(uint binding) => _uniformRanges[binding]; + + /// + /// The arena entry the current state belongs to. + /// is the caller's opaque handle for the entry — the + /// index of its (set 0, set 1) pair — or -1 when the entry is new and the + /// caller must allocate one and report it through . + /// NeedsWrite is true only when the entry's descriptors do not already + /// hold this state. + /// + internal (int Index, int Slot, bool NeedsWrite) Resolve() + { + if (!_dirty && _active >= 0) + return (_active, _entries[_active].Slot, false); + + for (int i = 0; i < _entries.Count; i++) + { + if (!Matches(_entries[i])) + continue; + if (i >= _liveCount) + { + (_entries[i], _entries[_liveCount]) = (_entries[_liveCount], _entries[i]); + _active = _liveCount; + _liveCount++; + } + else + { + _active = i; + } + + _dirty = false; + return (_active, _entries[_active].Slot, false); + } + + while (_entries.Count <= _liveCount) + _entries.Add(new Entry(_storageBindingCount, _uniformBindingCount)); + + Entry target = _entries[_liveCount]; + Adopt(target); + _active = _liveCount; + _liveCount++; + _dirty = false; + return (_active, target.Slot, true); + } + + /// Records the caller's descriptor-set-pair handle for a newly materialised entry. + internal void AssignSlot(int index, int slot) => _entries[index].Slot = slot; + + private bool Matches(Entry entry) + { + if (entry.Slot < 0) + return false; + for (uint binding = 0; binding < _storageBindingCount; binding++) + { + if (entry.StorageBuffers[binding] != _storageBuffers[binding]) + return false; + if (entry.StorageRanges[binding] != _storageRanges[binding]) + return false; + if (!_isDynamicStorage(binding) && entry.StorageOffsets[binding] != _storageOffsets[binding]) + return false; + } + + for (uint binding = 0; binding < _uniformBindingCount; binding++) + { + if (entry.UniformBuffers[binding] != _uniformBuffers[binding]) + return false; + if (entry.UniformRanges[binding] != _uniformRanges[binding]) + return false; + } + + return true; + } + + private void Adopt(Entry entry) + { + Array.Copy(_storageBuffers, entry.StorageBuffers, _storageBindingCount); + Array.Copy(_storageOffsets, entry.StorageOffsets, _storageBindingCount); + Array.Copy(_storageRanges, entry.StorageRanges, _storageBindingCount); + Array.Copy(_uniformBuffers, entry.UniformBuffers, _uniformBindingCount); + Array.Copy(_uniformRanges, entry.UniformRanges, _uniformBindingCount); + } +} diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs index 17708b0a..12638d35 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanFrameBindings.cs @@ -30,24 +30,33 @@ namespace AcDream.App.Rendering.Gpu.Vk; /// descriptor set layout and one pipeline layout rather than a permutation per /// renderer — plan §4.4's requirement, and the thing that makes switching /// pipelines mid-pass free. +/// +/// Slice V6i: one set pair per renderer scope. There is no longer a +/// single (set 0, set 1) pair per flight slot; there is an arena of them, and +/// decides which pair a bind belongs to and +/// whether its descriptors have to be written. That is what closes plan §5.5.8's +/// recorded one-binding-two-buffers hazard before the world arm fires it — see +/// the arena's own documentation for why the scope is derived from the +/// descriptor state rather than declared by the renderer. /// internal sealed unsafe class VulkanFrameBindings : IDisposable { private readonly Silk.NET.Vulkan.Vk _vk; private readonly Device _device; - private readonly DescriptorPool _pool; - private readonly DescriptorSet _storageSet; - private readonly DescriptorSet _uniformSet; - - private readonly uint[] _storageOffsets = new uint[GpuBindingModel.StorageBindingCount]; - private readonly uint[] _uniformOffsets = new uint[UniformBindingCount]; - private readonly Silk.NET.Vulkan.Buffer[] _storageBuffers = - new Silk.NET.Vulkan.Buffer[GpuBindingModel.StorageBindingCount]; - private readonly Silk.NET.Vulkan.Buffer[] _uniformBuffers = - new Silk.NET.Vulkan.Buffer[UniformBindingCount]; + private readonly VulkanPipelineLayouts.Created _layouts; + private readonly VulkanBindingScopeArena _arena; + private readonly List _pools = []; + private readonly List<(DescriptorSet Storage, DescriptorSet Uniform)> _sets = []; private bool _disposed; + /// + /// Set pairs one descriptor pool serves. Distinct descriptor states in a + /// frame are the world renderers plus the retained UI, so this is generous; + /// exceeding it allocates another pool rather than failing. + /// + private const int PairsPerPool = 16; + /// /// Set 0's dynamic-offset slots, in binding order — the order /// vkCmdBindDescriptorSets requires. A plain binding has no slot. @@ -92,61 +101,24 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable { _vk = vk ?? throw new ArgumentNullException(nameof(vk)); _device = device; - ArgumentNullException.ThrowIfNull(layouts); + _layouts = layouts ?? throw new ArgumentNullException(nameof(layouts)); ArgumentNullException.ThrowIfNull(ring); ArgumentNullException.ThrowIfNull(dummy); - DescriptorPoolSize* sizes = stackalloc DescriptorPoolSize[3]; - sizes[0] = new DescriptorPoolSize - { - Type = DescriptorType.StorageBufferDynamic, - DescriptorCount = VulkanPipelineLayouts.DynamicStorageBindingCount, - }; - sizes[1] = new DescriptorPoolSize - { - Type = DescriptorType.StorageBuffer, - DescriptorCount = - GpuBindingModel.StorageBindingCount - VulkanPipelineLayouts.DynamicStorageBindingCount, - }; - sizes[2] = new DescriptorPoolSize - { - Type = DescriptorType.UniformBufferDynamic, - DescriptorCount = UniformBindingCount, - }; - var poolCreate = new DescriptorPoolCreateInfo - { - SType = StructureType.DescriptorPoolCreateInfo, - MaxSets = 2, - PoolSizeCount = 3, - PPoolSizes = sizes, - }; - VulkanInterop.Check( - _vk.CreateDescriptorPool(_device, &poolCreate, null, out _pool), - "vkCreateDescriptorPool (frame bindings)"); - - _storageSet = Allocate(layouts.Storage); - _uniformSet = Allocate(layouts.Uniform); - - for (uint binding = 0; binding < GpuBindingModel.StorageBindingCount; binding++) - { - _storageBuffers[binding] = dummy.Handle; - 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 - // array is bookkeeping so the offsets stay index-aligned. - WriteUniform(GpuBindingModel.UniformSceneLighting, dummy.Handle, (uint)Math.Min(dummy.SizeBytes, 65536)); - WriteUniform(GpuBindingModel.UniformTerrainTiling, dummy.Handle, (uint)Math.Min(dummy.SizeBytes, 65536)); - _uniformBuffers[GpuBindingModel.UniformSceneLighting] = dummy.Handle; - _uniformBuffers[GpuBindingModel.UniformTerrainTiling] = dummy.Handle; - Ring = ring; Dummy = dummy; + _arena = new VulkanBindingScopeArena( + (int)GpuBindingModel.StorageBindingCount, + UniformBindingCount, + VulkanPipelineLayouts.IsDynamicStorageBinding); + + uint dummyStorageRange = (uint)Math.Min(dummy.SizeBytes, MaxBindingRangeBytes); + for (uint binding = 0; binding < GpuBindingModel.StorageBindingCount; binding++) + _arena.SeedStorage(binding, dummy.Handle.Handle, offsetBytes: 0, dummyStorageRange); + + uint dummyUniformRange = (uint)Math.Min(dummy.SizeBytes, 65536); + for (uint binding = 0; binding < UniformBindingCount; binding++) + _arena.SeedUniform(binding, dummy.Handle.Handle, dummyUniformRange); } internal VulkanGpuBuffer Ring { get; } @@ -154,51 +126,57 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable internal VulkanGpuBuffer Dummy { get; } /// - /// 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. + /// Diagnostic: distinct descriptor states this flight slot materialised. One + /// per renderer scope in a steady frame, so a number that keeps climbing is + /// a renderer pointing a binding at a fresh buffer every draw. /// + internal int ScopeCount => _arena.Count; + + /// + /// Recycles the arena for a new frame on this slot. Safe because the slot's + /// previous submission has retired before BeginFrame returns, which is + /// the same guarantee that lets the ring rewind. + /// + internal void BeginFrame() => _arena.BeginFrame(); + internal void SetStorage(uint binding, VulkanGpuBuffer buffer, uint offsetBytes, uint sizeBytes) { ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(binding, GpuBindingModel.StorageBindingCount); - 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, - dynamic ? 0 : offsetBytes, - ClampRange(buffer, sizeBytes, dynamic ? 0 : offsetBytes), - dynamic); - } - - _storageOffsets[binding] = offsetBytes; + uint descriptorOffset = VulkanPipelineLayouts.IsDynamicStorageBinding(binding) ? 0 : offsetBytes; + _arena.SetStorage( + binding, + buffer.Handle.Handle, + offsetBytes, + ClampRange(buffer, sizeBytes, descriptorOffset)); } internal void SetUniform(uint binding, VulkanGpuBuffer buffer, uint offsetBytes, uint sizeBytes) { ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(binding, (uint)UniformBindingCount); - if (_uniformBuffers[binding].Handle != buffer.Handle.Handle) - { - _uniformBuffers[binding] = buffer.Handle; - WriteUniform(binding, buffer.Handle, Math.Min(ClampRange(buffer, sizeBytes, offsetBytes: 0), 65536)); - } - - _uniformOffsets[binding] = offsetBytes; + _arena.SetUniform( + binding, + buffer.Handle.Handle, + offsetBytes, + Math.Min(ClampRange(buffer, sizeBytes, offsetBytes: 0), 65536)); } /// Binds all three sets with the current dynamic offsets. internal void Bind(CommandBuffer commands, VulkanGpuDevice device) { + (int index, int slot, bool needsWrite) = _arena.Resolve(); + if (slot < 0) + { + slot = _sets.Count; + _sets.Add(AllocatePair()); + _arena.AssignSlot(index, slot); + } + + if (needsWrite) + WritePair(_sets[slot]); + DescriptorSet* sets = stackalloc DescriptorSet[3]; - sets[0] = _storageSet; - sets[1] = _uniformSet; + sets[0] = _sets[slot].Storage; + sets[1] = _sets[slot].Uniform; sets[2] = device.TextureTable.Set; int dynamicCount = DynamicStorageBindings.Length + 2; @@ -206,9 +184,9 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable // 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]; + offsets[i] = _arena.StorageOffset(DynamicStorageBindings[i]); + offsets[DynamicStorageBindings.Length + 0] = _arena.UniformOffset(GpuBindingModel.UniformSceneLighting); + offsets[DynamicStorageBindings.Length + 1] = _arena.UniformOffset(GpuBindingModel.UniformTerrainTiling); _vk.CmdBindDescriptorSets( commands, @@ -221,6 +199,74 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable offsets); } + private void WritePair((DescriptorSet Storage, DescriptorSet Uniform) pair) + { + for (uint binding = 0; binding < GpuBindingModel.StorageBindingCount; binding++) + { + WriteStorage( + pair.Storage, + binding, + new Silk.NET.Vulkan.Buffer(_arena.StorageBuffer(binding)), + _arena.StorageDescriptorOffset(binding), + _arena.StorageRange(binding), + VulkanPipelineLayouts.IsDynamicStorageBinding(binding)); + } + + // Only the two bindings the layout declares exist; the rest of the array + // is bookkeeping so the offsets stay index-aligned. + WriteUniform( + pair.Uniform, + GpuBindingModel.UniformSceneLighting, + new Silk.NET.Vulkan.Buffer(_arena.UniformBuffer(GpuBindingModel.UniformSceneLighting)), + _arena.UniformRange(GpuBindingModel.UniformSceneLighting)); + WriteUniform( + pair.Uniform, + GpuBindingModel.UniformTerrainTiling, + new Silk.NET.Vulkan.Buffer(_arena.UniformBuffer(GpuBindingModel.UniformTerrainTiling)), + _arena.UniformRange(GpuBindingModel.UniformTerrainTiling)); + } + + private (DescriptorSet Storage, DescriptorSet Uniform) AllocatePair() + { + if (_sets.Count % PairsPerPool == 0) + _pools.Add(CreatePool()); + DescriptorPool pool = _pools[^1]; + return (Allocate(pool, _layouts.Storage), Allocate(pool, _layouts.Uniform)); + } + + private DescriptorPool CreatePool() + { + DescriptorPoolSize* sizes = stackalloc DescriptorPoolSize[3]; + sizes[0] = new DescriptorPoolSize + { + Type = DescriptorType.StorageBufferDynamic, + DescriptorCount = VulkanPipelineLayouts.DynamicStorageBindingCount * PairsPerPool, + }; + sizes[1] = new DescriptorPoolSize + { + Type = DescriptorType.StorageBuffer, + DescriptorCount = + (GpuBindingModel.StorageBindingCount - VulkanPipelineLayouts.DynamicStorageBindingCount) + * PairsPerPool, + }; + sizes[2] = new DescriptorPoolSize + { + Type = DescriptorType.UniformBufferDynamic, + DescriptorCount = DynamicUniformBindingCount * PairsPerPool, + }; + var poolCreate = new DescriptorPoolCreateInfo + { + SType = StructureType.DescriptorPoolCreateInfo, + MaxSets = 2 * PairsPerPool, + PoolSizeCount = 3, + PPoolSizes = sizes, + }; + VulkanInterop.Check( + _vk.CreateDescriptorPool(_device, &poolCreate, null, out DescriptorPool pool), + "vkCreateDescriptorPool (frame bindings)"); + return pool; + } + private static uint ClampRange(VulkanGpuBuffer buffer, uint requested, uint offsetBytes) { long remaining = buffer.SizeBytes - offsetBytes; @@ -237,13 +283,13 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable return requested == 0 ? available : Math.Min(Math.Max(requested, 16), available); } - private DescriptorSet Allocate(DescriptorSetLayout layout) + private DescriptorSet Allocate(DescriptorPool pool, DescriptorSetLayout layout) { DescriptorSetLayout handle = layout; var allocate = new DescriptorSetAllocateInfo { SType = StructureType.DescriptorSetAllocateInfo, - DescriptorPool = _pool, + DescriptorPool = pool, DescriptorSetCount = 1, PSetLayouts = &handle, }; @@ -254,6 +300,7 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable } private void WriteStorage( + DescriptorSet set, uint binding, Silk.NET.Vulkan.Buffer buffer, uint offsetBytes, @@ -269,7 +316,7 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable var write = new WriteDescriptorSet { SType = StructureType.WriteDescriptorSet, - DstSet = _storageSet, + DstSet = set, DstBinding = binding, DescriptorCount = 1, DescriptorType = dynamic @@ -280,7 +327,11 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable _vk.UpdateDescriptorSets(_device, 1, &write, 0, null); } - private void WriteUniform(uint binding, Silk.NET.Vulkan.Buffer buffer, uint rangeBytes) + private void WriteUniform( + DescriptorSet set, + uint binding, + Silk.NET.Vulkan.Buffer buffer, + uint rangeBytes) { var info = new DescriptorBufferInfo { @@ -291,7 +342,7 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable var write = new WriteDescriptorSet { SType = StructureType.WriteDescriptorSet, - DstSet = _uniformSet, + DstSet = set, DstBinding = binding, DescriptorCount = 1, DescriptorType = DescriptorType.UniformBufferDynamic, @@ -305,7 +356,13 @@ internal sealed unsafe class VulkanFrameBindings : IDisposable if (_disposed) return; _disposed = true; - if (_pool.Handle != 0) - _vk.DestroyDescriptorPool(_device, _pool, null); + foreach (DescriptorPool pool in _pools) + { + if (pool.Handle != 0) + _vk.DestroyDescriptorPool(_device, pool, null); + } + + _pools.Clear(); + _sets.Clear(); } } diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs index 753fd2ea..3137c11d 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.Resources.cs @@ -114,6 +114,9 @@ internal sealed unsafe partial class VulkanGpuDevice private void BeginFrameResources(int slotIndex) => _timerPool?.BeginSlot(slotIndex); + /// Slice V6i: the descriptor-set arena for one flight slot. + private VulkanFrameBindings FrameBindingsAt(int slotIndex) => _frameBindings[slotIndex]; + private void EndFrameResources(int slotIndex, CommandBuffer commands) { _ = slotIndex; diff --git a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs index bb6b732f..31b4daec 100644 --- a/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs +++ b/src/AcDream.App/Rendering/Gpu/Vk/VulkanGpuDevice.cs @@ -322,6 +322,10 @@ internal sealed unsafe partial class VulkanGpuDevice : IGpuDevice _uploads.ReleaseCompleted(CompletedSerial()); _ringStates[slot].Reset(); + // Campaign V slice V6i: the descriptor-set arena rewinds with the ring, + // and for the same reason — this slot's previous submission has retired, + // so no recorded draw can still be reading the sets it hands out. + FrameBindingsAt(slot).BeginFrame(); _acquiredImageIndex = null; if (_backbuffer is not null) diff --git a/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanBindingScopeArenaTests.cs b/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanBindingScopeArenaTests.cs new file mode 100644 index 00000000..17c06316 --- /dev/null +++ b/tests/AcDream.App.Tests/Rendering/Gpu/Vk/VulkanBindingScopeArenaTests.cs @@ -0,0 +1,235 @@ +using AcDream.App.Rendering.Gpu; +using AcDream.App.Rendering.Gpu.Vk; + +namespace AcDream.App.Tests.Rendering.Gpu.Vk; + +/// +/// Campaign V slice V6i. Plan §5.5.8's second recorded gap was that pointing one +/// binding at a second buffer within a frame silently corrupts the draws already +/// recorded against it, because a descriptor set is read when the command buffer +/// executes and not when it was recorded. These tests pin the property that +/// closes it: within one frame, an entry a draw is already bound to is never +/// rewritten. +/// +public sealed class VulkanBindingScopeArenaTests +{ + private const ulong RingBuffer = 0x1000; + private const ulong DummyBuffer = 0x2000; + private const ulong DispatcherBuffer = 0x3000; + private const ulong EnvCellBuffer = 0x4000; + + private static VulkanBindingScopeArena CreateArena() + { + var arena = new VulkanBindingScopeArena( + (int)GpuBindingModel.StorageBindingCount, + VulkanFrameBindings.UniformBindingCount, + VulkanPipelineLayouts.IsDynamicStorageBinding); + for (uint binding = 0; binding < GpuBindingModel.StorageBindingCount; binding++) + arena.SeedStorage(binding, DummyBuffer, offsetBytes: 0, rangeBytes: 65536); + for (uint binding = 0; binding < VulkanFrameBindings.UniformBindingCount; binding++) + arena.SeedUniform(binding, DummyBuffer, rangeBytes: 65536); + return arena; + } + + [Fact] + public void TwoRenderersBindingTheSameBindingToDifferentBuffersGetDifferentEntries() + { + VulkanBindingScopeArena arena = CreateArena(); + arena.BeginFrame(); + + arena.SetStorage(GpuBindingModel.StorageInstances, DispatcherBuffer, 0, 4096); + (int firstIndex, _, bool firstWrite) = arena.Resolve(); + arena.AssignSlot(firstIndex, slot: 0); + Assert.True(firstWrite); + + arena.SetStorage(GpuBindingModel.StorageInstances, EnvCellBuffer, 0, 4096); + (int secondIndex, _, bool secondWrite) = arena.Resolve(); + arena.AssignSlot(secondIndex, slot: 1); + + Assert.True(secondWrite); + Assert.NotEqual(firstIndex, secondIndex); + Assert.Equal(2, arena.Count); + Assert.Equal(2, arena.LiveCount); + } + + [Fact] + public void ReturningToAnEarlierRenderersBuffersReusesItsEntryWithoutRewriting() + { + VulkanBindingScopeArena arena = CreateArena(); + arena.BeginFrame(); + + arena.SetStorage(GpuBindingModel.StorageInstances, DispatcherBuffer, 0, 4096); + (int dispatcher, _, _) = arena.Resolve(); + arena.AssignSlot(dispatcher, slot: 0); + + arena.SetStorage(GpuBindingModel.StorageInstances, EnvCellBuffer, 0, 4096); + (int envCell, _, _) = arena.Resolve(); + arena.AssignSlot(envCell, slot: 1); + + arena.SetStorage(GpuBindingModel.StorageInstances, DispatcherBuffer, 0, 4096); + (int again, int slot, bool needsWrite) = arena.Resolve(); + + Assert.Equal(dispatcher, again); + Assert.Equal(0, slot); + Assert.False(needsWrite); + Assert.Equal(2, arena.Count); + } + + [Fact] + public void ADynamicBindingsMovingOffsetCostsNoNewEntryAndNoWrite() + { + VulkanBindingScopeArena arena = CreateArena(); + arena.BeginFrame(); + + arena.SetStorage(GpuBindingModel.StorageInstances, RingBuffer, 0, 4096); + (int index, _, _) = arena.Resolve(); + arena.AssignSlot(index, slot: 0); + + arena.SetStorage(GpuBindingModel.StorageInstances, RingBuffer, 8192, 4096); + (int second, _, bool needsWrite) = arena.Resolve(); + + Assert.Equal(index, second); + Assert.False(needsWrite); + Assert.Equal(1, arena.Count); + Assert.Equal(8192u, arena.StorageOffset(GpuBindingModel.StorageInstances)); + // A dynamic descriptor addresses the whole range and slides with the + // bind call, so its descriptor offset stays zero. + Assert.Equal(0u, arena.StorageDescriptorOffset(GpuBindingModel.StorageInstances)); + } + + [Fact] + public void APlainBindingsMovingOffsetIsDescriptorVisibleAndTakesANewEntry() + { + VulkanBindingScopeArena arena = CreateArena(); + arena.BeginFrame(); + + // Binding 4 (global lights) is plain: V6g's rule keeps only ring-fed + // bindings dynamic, so a plain binding's offset lives in its descriptor. + Assert.False(VulkanPipelineLayouts.IsDynamicStorageBinding(GpuBindingModel.StorageGlobalLights)); + + arena.SetStorage(GpuBindingModel.StorageGlobalLights, RingBuffer, 0, 256); + (int first, _, _) = arena.Resolve(); + arena.AssignSlot(first, slot: 0); + + arena.SetStorage(GpuBindingModel.StorageGlobalLights, RingBuffer, 512, 256); + (int second, _, bool needsWrite) = arena.Resolve(); + arena.AssignSlot(second, slot: 1); + + Assert.NotEqual(first, second); + Assert.True(needsWrite); + Assert.Equal(512u, arena.StorageDescriptorOffset(GpuBindingModel.StorageGlobalLights)); + } + + [Fact] + public void ASteadyFrameRewritesNothing() + { + VulkanBindingScopeArena arena = CreateArena(); + + arena.BeginFrame(); + arena.SetStorage(GpuBindingModel.StorageInstances, DispatcherBuffer, 0, 4096); + (int index, _, bool firstWrite) = arena.Resolve(); + arena.AssignSlot(index, slot: 0); + Assert.True(firstWrite); + + // The next frame on the same flight slot binds the same buffers. The + // previous frame has retired, so its entry is free to reclaim — and its + // descriptors already say exactly this, so nothing is written. + arena.BeginFrame(); + arena.SetStorage(GpuBindingModel.StorageInstances, DispatcherBuffer, 0, 4096); + (int reused, int slot, bool needsWrite) = arena.Resolve(); + + Assert.Equal(0, reused); + Assert.Equal(0, slot); + Assert.False(needsWrite); + Assert.Equal(1, arena.Count); + } + + [Fact] + public void AnEntryReclaimedFromTheLastFrameIsProtectedForTheRestOfThisOne() + { + VulkanBindingScopeArena arena = CreateArena(); + + arena.BeginFrame(); + arena.SetStorage(GpuBindingModel.StorageInstances, DispatcherBuffer, 0, 4096); + (int dispatcher, _, _) = arena.Resolve(); + arena.AssignSlot(dispatcher, slot: 0); + arena.SetStorage(GpuBindingModel.StorageInstances, EnvCellBuffer, 0, 4096); + (int envCell, _, _) = arena.Resolve(); + arena.AssignSlot(envCell, slot: 1); + + // Next frame, in the opposite order. The EnvCell entry is matched from + // above the cursor and swapped down; the dispatcher must then get the + // OTHER entry rather than overwriting the one EnvCell's draw is bound to. + arena.BeginFrame(); + arena.SetStorage(GpuBindingModel.StorageInstances, EnvCellBuffer, 0, 4096); + (int reusedEnvCell, int envCellSlot, bool envCellWrite) = arena.Resolve(); + arena.SetStorage(GpuBindingModel.StorageInstances, DispatcherBuffer, 0, 4096); + (int reusedDispatcher, int dispatcherSlot, bool dispatcherWrite) = arena.Resolve(); + + Assert.False(envCellWrite); + Assert.False(dispatcherWrite); + Assert.NotEqual(reusedEnvCell, reusedDispatcher); + Assert.NotEqual(envCellSlot, dispatcherSlot); + Assert.Equal(2, arena.Count); + Assert.Equal(2, arena.LiveCount); + } + + [Fact] + public void AThirdDistinctStateInOneFrameMaterialisesAThirdEntry() + { + VulkanBindingScopeArena arena = CreateArena(); + arena.BeginFrame(); + + Bind(arena, DispatcherBuffer, slot: 0); + Bind(arena, EnvCellBuffer, slot: 1); + Bind(arena, RingBuffer, slot: 2); + + Assert.Equal(3, arena.Count); + Assert.Equal(3, arena.LiveCount); + + static void Bind(VulkanBindingScopeArena arena, ulong buffer, int slot) + { + arena.SetStorage(GpuBindingModel.StorageInstances, buffer, 0, 4096); + (int index, int existing, bool needsWrite) = arena.Resolve(); + Assert.True(needsWrite); + Assert.Equal(-1, existing); + arena.AssignSlot(index, slot); + } + } + + [Fact] + public void AUniformBufferChangeAlsoSeparatesScopes() + { + VulkanBindingScopeArena arena = CreateArena(); + arena.BeginFrame(); + + arena.SetUniform(GpuBindingModel.UniformSceneLighting, DispatcherBuffer, 0, 256); + (int first, _, _) = arena.Resolve(); + arena.AssignSlot(first, slot: 0); + + arena.SetUniform(GpuBindingModel.UniformTerrainTiling, EnvCellBuffer, 0, 576); + (int second, _, bool needsWrite) = arena.Resolve(); + arena.AssignSlot(second, slot: 1); + + Assert.NotEqual(first, second); + Assert.True(needsWrite); + } + + [Fact] + public void AUniformBuffersMovingOffsetCostsNoNewEntry() + { + VulkanBindingScopeArena arena = CreateArena(); + arena.BeginFrame(); + + arena.SetUniform(GpuBindingModel.UniformSceneLighting, RingBuffer, 0, 256); + (int index, _, _) = arena.Resolve(); + arena.AssignSlot(index, slot: 0); + + arena.SetUniform(GpuBindingModel.UniformSceneLighting, RingBuffer, 1024, 256); + (int second, _, bool needsWrite) = arena.Resolve(); + + Assert.Equal(index, second); + Assert.False(needsWrite); + Assert.Equal(1024u, arena.UniformOffset(GpuBindingModel.UniformSceneLighting)); + } +}