From 801509d9d5e9cf424768523ac6a621445740adb7 Mon Sep 17 00:00:00 2001 From: catbot Date: Wed, 17 Jun 2026 17:50:01 +0000 Subject: [PATCH] perf(rt): allocate TLAS metadata buffer in BAR/VRAM, not system RAM (#75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TLAS metadata buffer is CPU-written every frame (one userMetadata word per element in BuildTLAS's copy loop) and read by the ray shaders as a STORAGE_BUFFER via device address every frame. Allocated HOST_VISIBLE | HOST_COHERENT (system RAM), every per-frame shader read traversed PCIe — the same cost the sibling instance buffer paid before #65. Upgrade the request to HOST_VISIBLE with a best-effort DEVICE_LOCAL preference (BAR/VRAM), keeping the single persistently-mapped buffer. The CPU's writes are write-only, sequential, never-read-back — the ideal write-combined BAR load — and the shader reads now hit local VRAM. Unlike the instance buffer there is no GPU co-write, so the direct upgrade is unconditional. Staging is still deliberately avoided: this buffer is rewritten by the CPU every frame, so a per-frame stage+copy+barrier would defeat the purpose (#75 caveat 2). Correctness: - DEVICE_LOCAL is a preference only (depends on #59): GetMemoryType falls back to plain HOST_VISIBLE (the previous behaviour) on GPUs without resizable BAR. - HOST_COHERENT is dropped from the required set — the combined type may not be coherent. A FlushDevice() is added after the copy loop to make the host writes available on a non-coherent type; it self-gates to a no-op on a coherent type (#60), so the previous behaviour is preserved wherever the type ends up coherent. The metadata buffer is not an AS build input and has no GPU co-write, so it needs neither the build-stage barrier the instance buffer takes nor a HOST->shader command barrier: the queue submit already orders host writes made available before it against every command in the submission, exactly as it did when this buffer was HOST_COHERENT. Extends the TLASHighWaterMark hardware test with a metadata-buffer assertion block mirroring #65's instance-buffer check: the chosen memory type must equal what GetMemoryType resolves for the HOST_VISIBLE | prefer-DEVICE_LOCAL request, and must be DEVICE_LOCAL wherever a host-visible device-local type is reachable. The zero-validation-errors check covers the dropped HOST_COHERENT path. Co-Authored-By: Claude Opus 4.8 --- .../Crafter.Graphics-RenderingElement3D.cpp | 27 +++++++++- tests/TLASHighWaterMark/main.cpp | 52 +++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/implementations/Crafter.Graphics-RenderingElement3D.cpp b/implementations/Crafter.Graphics-RenderingElement3D.cpp index 1da2a3a..d06630c 100644 --- a/implementations/Crafter.Graphics-RenderingElement3D.cpp +++ b/implementations/Crafter.Graphics-RenderingElement3D.cpp @@ -121,10 +121,23 @@ void RenderingElement3D::BuildTLAS(VkCommandBuffer cmd, std::uint32_t index, RTB // visibility, gating its flush-skip on the *chosen* type's flags. The // single shared buffer is preserved: a whole-buffer copy would clobber // the GPU-written transforms, so staging is deliberately avoided. + // + // metadataBuffer (#75) gets the same HOST_VISIBLE | prefer-DEVICE_LOCAL + // upgrade: it is CPU-written every frame (the copy loop below) and read + // by the ray shaders as a STORAGE_BUFFER every frame, so it has the same + // per-frame-shader-read-over-PCIe cost. Unlike instanceBuffer there is + // no GPU co-write, so the direct upgrade is unconditional — but staging + // is still wrong here: a per-frame-rewritten buffer would pay a stage+ + // copy+barrier every frame, defeating the point. HOST_COHERENT is + // likewise dropped; the FlushDevice() after the copy loop makes the host + // writes available before the consuming submit on a non-coherent type + // (no-op when the chosen type is coherent — the previous behaviour), + // and the queue submit carries the host-write -> shader-read ordering as + // it always has. if (tlas.instanceBuffer.buffer == VK_NULL_HANDLE || primitiveCount > tlas.instanceBuffer.size / sizeof(VkAccelerationStructureInstanceKHR)) { tlas.instanceBuffer.Resize(VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT | VK_BUFFER_USAGE_TRANSFER_DST_BIT | VK_BUFFER_USAGE_ACCELERATION_STRUCTURE_BUILD_INPUT_READ_ONLY_BIT_KHR | VK_BUFFER_USAGE_STORAGE_BUFFER_BIT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, primitiveCount, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); - tlas.metadataBuffer.Resize(VK_BUFFER_USAGE_STORAGE_BUFFER_BIT | VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_COHERENT_BIT, primitiveCount); + tlas.metadataBuffer.Resize(VK_BUFFER_USAGE_STORAGE_BUFFER_BIT | VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, primitiveCount, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); } } @@ -147,6 +160,18 @@ void RenderingElement3D::BuildTLAS(VkCommandBuffer cmd, std::uint32_t index, RTB tlas.instanceBuffer.FlushDevice(cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); + // Make the per-frame metadata host writes available before the consuming + // submit. metadataBuffer is not an AS build input (the build never reads it) + // and has no GPU co-write — only the ray shaders read it, in a later pass — + // so it needs neither the build-stage barrier instanceBuffer takes above nor + // a HOST->shader command barrier: the queue submit already orders host + // writes made available before it against every command in the submission, + // exactly as it did when this buffer was HOST_COHERENT. This plain host + // flush is what now makes those writes available on a non-coherent + // (BAR/VRAM) type; it self-gates to a no-op on a coherent type (#60), + // preserving the previous behaviour where the type ends up coherent. + tlas.metadataBuffer.FlushDevice(); + VkAccelerationStructureGeometryInstancesDataKHR instancesData { .sType = VK_STRUCTURE_TYPE_ACCELERATION_STRUCTURE_GEOMETRY_INSTANCES_DATA_KHR, .arrayOfPointers = VK_FALSE, diff --git a/tests/TLASHighWaterMark/main.cpp b/tests/TLASHighWaterMark/main.cpp index 55bd544..987b960 100644 --- a/tests/TLASHighWaterMark/main.cpp +++ b/tests/TLASHighWaterMark/main.cpp @@ -58,6 +58,15 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA // HOST_COHERENT: the FlushDevice(cmd, ...) in BuildTLAS gates on the chosen // type's coherency flag, so a non-coherent BAR type still flushes correctly. // +// Issue #75: the metadata buffer gets the identical upgrade — it is CPU-written +// every frame and read by the ray shaders as a STORAGE_BUFFER every frame, so +// the same HOST_VISIBLE | prefer-DEVICE_LOCAL, no-mandatory-HOST_COHERENT +// request keeps those per-frame shader reads in local VRAM. A second assertion +// block after the first build checks its chosen memory type the same way. The +// dropped HOST_COHERENT is covered by the zero-validation-errors check: the +// FlushDevice() added after the copy loop in BuildTLAS makes the host writes +// available on a non-coherent type and self-gates to a no-op on a coherent one. +// // Validation layers are required for the last check to be meaningful; the build // marks this test as needing the SDK layers. @@ -260,6 +269,49 @@ int main() { } } + // ── Issue #75: the metadata buffer carries the same memory-type request as + // the instance buffer — HOST_VISIBLE (required, mappable) with a best-effort + // DEVICE_LOCAL preference (BAR/VRAM) and the mandatory HOST_COHERENT dropped. + // It is CPU-written every frame and read by the ray shaders every frame, so + // landing it in local VRAM keeps those per-frame storage-buffer reads off + // PCIe. Same device-independent check: assert the recorded chosen flags equal + // what GetMemoryType resolves for this buffer's memoryTypeBits with that + // request, and that DEVICE_LOCAL is chosen wherever a host-visible + // device-local type is reachable. ─────────────────────────────────────────── + { + auto& mb = RenderingElement3D::tlases[0].metadataBuffer; + VkMemoryRequirements req{}; + vkGetBufferMemoryRequirements(Device::device, mb.buffer, &req); + std::uint32_t expectIdx = Device::GetMemoryType( + req.memoryTypeBits, + VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, + VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); + VkMemoryPropertyFlags expectFlags = + Device::memoryProperties.memoryTypes[expectIdx].propertyFlags; + Check((mb.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT) != 0, + "metadata buffer memory is HOST_VISIBLE (mappable — the required flag)"); + Check(mb.memoryPropertyFlagsChosen == expectFlags, + "metadata buffer memory matches the HOST_VISIBLE | prefer-DEVICE_LOCAL request (#75)"); + + bool barReachable = false; + constexpr VkMemoryPropertyFlags kBar = + VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; + for (std::uint32_t i = 0; i < Device::memoryProperties.memoryTypeCount; ++i) { + if ((req.memoryTypeBits & (1u << i)) + && (Device::memoryProperties.memoryTypes[i].propertyFlags & kBar) == kBar) { + barReachable = true; + break; + } + } + if (barReachable) { + Check((mb.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT) != 0, + "metadata buffer landed on a DEVICE_LOCAL (BAR/VRAM) type where one is reachable"); + } else { + std::println("INFO no host-visible device-local type reachable — " + "metadata fell back to plain HOST_VISIBLE (expected on non-BAR GPUs)"); + } + } + // ── 2. Grow to 16 (past capacity) → REALLOCATES. ──────────────────────── SetCount(pool, 16); BuildOnce(); -- 2.54.0