From 5f858509c81ed0be7ea3480cc08b890d3bcb3c66 Mon Sep 17 00:00:00 2001 From: catbot Date: Wed, 17 Jun 2026 17:49:25 +0000 Subject: [PATCH] perf(mesh): place RT geometry in device-local memory via #89 upload strategy (#73) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vertexBuffer/indexBuffer/aabbBuffer were HOST_VISIBLE (system RAM), so the BLAS build, every refit, and any hit-shader geometry fetch read them over PCIe. The usage flags (SHADER_DEVICE_ADDRESS, plus STORAGE on the index buffer) exist precisely to expose geometry for hit-shader fetch, the dominant RT shading pattern. Add VulkanBuffer::UploadDeviceLocal, which places a CPU-written/GPU-read buffer in device-local memory and picks the upload mechanism at runtime via the #89 strategy (Device::PreferDirectDeviceWrite): - ReBAR/UMA: allocate HOST_VISIBLE|DEVICE_LOCAL (DEVICE_LOCAL preferred), map transiently, memcpy, flush-if-non-coherent. No staging buffer. - no/small BAR: allocate pure DEVICE_LOCAL + TRANSFER_DST, fill a transient HOST_VISIBLE staging buffer, vkCmdCopyBuffer, then DeferredClear the staging buffer onto the fence-keyed deletion queue (#101/#102) so it outlives the copy submit. The geometry buffers become non-mapped so the destination is free to be device-local-only. Same-size re-uploads reuse the allocation, so the device address stays stable across an in-place AS UPDATE refit. The compressed Build path now allocates vertex/index as pure DEVICE_LOCAL — the GPU decompressor fills them directly, no host write. Tests: BLASBuildOptions asserts device-local placement on the triangle, procedural, and (budget-forced) staged paths, and exercises the staged copy + deferred-deletion + in-place refit under GPU-assisted validation with zero validation errors. Co-Authored-By: Claude Opus 4.8 --- implementations/Crafter.Graphics-Mesh.cpp | 45 ++++++----- interfaces/Crafter.Graphics-Mesh.cppm | 12 ++- interfaces/Crafter.Graphics-VulkanBuffer.cppm | 77 +++++++++++++++++++ tests/BLASBuildOptions/main.cpp | 50 ++++++++++++ 4 files changed, 161 insertions(+), 23 deletions(-) diff --git a/implementations/Crafter.Graphics-Mesh.cpp b/implementations/Crafter.Graphics-Mesh.cpp index f324b4d..f9cc990 100644 --- a/implementations/Crafter.Graphics-Mesh.cpp +++ b/implementations/Crafter.Graphics-Mesh.cpp @@ -201,14 +201,13 @@ namespace { } void Mesh::Build(std::span> verticies, std::span indicies, VkCommandBuffer cmd, RTBuildOptions options) { - vertexBuffer.Resize(kVertexUsageBase, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, verticies.size()); - indexBuffer.Resize(kIndexUsageBase, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, indicies.size()); - - std::memcpy(vertexBuffer.value, verticies.data(), verticies.size() * sizeof(Vector)); - std::memcpy(indexBuffer.value, indicies.data(), indicies.size() * sizeof(std::uint32_t)); - - vertexBuffer.FlushDevice(cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); - indexBuffer.FlushDevice(cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); + // Place both inputs in device-local memory (direct map on ReBAR/UMA, + // staged copy otherwise — UploadDeviceLocal decides) and barrier the + // upload before the BLAS build reads them. Replaces the previous + // HOST_VISIBLE allocation that the build (and any hit-shader fetch) would + // read over PCIe every frame (#73). + vertexBuffer.UploadDeviceLocal(kVertexUsageBase, verticies.data(), static_cast(verticies.size()), cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); + indexBuffer.UploadDeviceLocal(kIndexUsageBase, indicies.data(), static_cast(indicies.size()), cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); allowUpdate = options.allowUpdate; builtInputCount = static_cast(verticies.size()); @@ -235,13 +234,17 @@ void Mesh::Build(const CompressedMeshAsset& asset, VkCommandBuffer cmd, RTBuildO return; } + // The GPU decompressor writes vertex/index directly (the build input is + // never host-written on this path), so they want pure DEVICE_LOCAL — no + // host visibility, no map. This is where the BLAS build and any hit-shader + // fetch then read them from (#73). vertexBuffer.Resize( kVertexUsageBase | VK_BUFFER_USAGE_2_MEMORY_DECOMPRESSION_BIT_EXT, - VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, + VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, asset.vertexCount); indexBuffer.Resize( kIndexUsageBase | VK_BUFFER_USAGE_2_MEMORY_DECOMPRESSION_BIT_EXT, - VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, + VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, asset.indexCount); compressedStaging.Resize( @@ -317,14 +320,13 @@ namespace { void RecordProceduralBuild(Mesh& self, std::span aabbs, VkBuildAccelerationStructureFlagsKHR flags, bool update, VkCommandBuffer cmd) { // 24-byte-stride VkAabbPositionsKHR-compatible build input // (static_assert'd in the interface). Same usage set as the triangle - // inputs: AS-build read-only + device address. A refit reuses the - // existing same-sized buffer (count is unchanged), so the device - // address — and the geometry it feeds — stays stable. - if (!update) { - self.aabbBuffer.Resize(kVertexUsageBase, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, static_cast(aabbs.size())); - } - std::memcpy(self.aabbBuffer.value, aabbs.data(), aabbs.size() * sizeof(RTAabb)); - self.aabbBuffer.FlushDevice(cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); + // inputs: AS-build read-only + device address. UploadDeviceLocal places + // it in device-local memory (direct map on ReBAR/UMA, staged otherwise, + // #73) and barriers the upload before the build reads it. A refit + // re-uploads into the same same-sized buffer: Resize reuses the existing + // allocation (count unchanged), so the device address — and the geometry + // it feeds — stays stable for an in-place UPDATE. + self.aabbBuffer.UploadDeviceLocal(kVertexUsageBase, aabbs.data(), static_cast(aabbs.size()), cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); RecordProceduralBuildFromAddress(self, self.aabbBuffer.address, static_cast(aabbs.size()), sizeof(RTAabb), flags, update, cmd); } @@ -382,8 +384,11 @@ void Mesh::Refit(std::span> verticies, std::span)); - vertexBuffer.FlushDevice(cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); + // Re-upload only the vertex positions; UploadDeviceLocal reuses the same + // device-local allocation (count unchanged) so the address the UPDATE reads + // stays stable, then barriers the write before the build. On the staged + // path this re-stages per refit (the deforming-mesh fallback, #73). + vertexBuffer.UploadDeviceLocal(kVertexUsageBase, verticies.data(), static_cast(verticies.size()), cmd, VK_ACCESS_MEMORY_READ_BIT, VK_PIPELINE_STAGE_ACCELERATION_STRUCTURE_BUILD_BIT_KHR); RecordBLASBuild(*this, static_cast(verticies.size()), static_cast(indicies.size()), buildFlags, /*update*/ true, cmd); } diff --git a/interfaces/Crafter.Graphics-Mesh.cppm b/interfaces/Crafter.Graphics-Mesh.cppm index e5f1c6a..91341a5 100644 --- a/interfaces/Crafter.Graphics-Mesh.cppm +++ b/interfaces/Crafter.Graphics-Mesh.cppm @@ -71,12 +71,18 @@ export namespace Crafter { public: VulkanBuffer scratchBuffer; VulkanBuffer blasBuffer; - VulkanBuffer, true> vertexBuffer; - VulkanBuffer indexBuffer; + // Non-mapped (device-local) RT geometry: VulkanBuffer::UploadDeviceLocal + // places these in device memory and picks direct-map vs staged-copy per + // the #89 upload strategy, so they live in VRAM for hit-shader fetch and + // BLAS-build/refit reads instead of being read from system RAM over PCIe + // every trace (#73). The compressed Build path allocates them pure + // DEVICE_LOCAL and lets the GPU decompressor fill them directly. + VulkanBuffer, false> vertexBuffer; + VulkanBuffer indexBuffer; // AABB build input for the procedural path (BuildProcedural). // Lifetime contract matches vertexBuffer/indexBuffer: must stay // alive until the build submitted on `cmd` completes. - VulkanBuffer aabbBuffer; + VulkanBuffer aabbBuffer; // Lives until the cmd buffer issued by the compressed Build path // completes execution. Kept as a member so the recorded // vkCmdDecompressMemoryEXT references valid memory until the queue diff --git a/interfaces/Crafter.Graphics-VulkanBuffer.cppm b/interfaces/Crafter.Graphics-VulkanBuffer.cppm index a499062..fe0e60a 100644 --- a/interfaces/Crafter.Graphics-VulkanBuffer.cppm +++ b/interfaces/Crafter.Graphics-VulkanBuffer.cppm @@ -235,6 +235,83 @@ namespace Crafter { ); } + // Upload `count` host elements from `src` into this buffer as + // device-local, GPU-read geometry, choosing placement at runtime via + // the #89 upload strategy (Device::PreferDirectDeviceWrite) and then + // recording a barrier from the upload to (dstStageMask, dstAccessMask) + // on `cmd`: + // ReBAR / UMA (direct) — allocate HOST_VISIBLE with DEVICE_LOCAL as a + // best-effort preference (GetMemoryType lands the host-visible + // device-local type that the strategy already proved exists), map + // transiently, memcpy, flush if the chosen type isn't coherent, + // unmap. No staging buffer: it would be pure overhead on a bar where + // the device-local heap is itself host-writable. + // No / small BAR (staged) — allocate pure DEVICE_LOCAL (+ TRANSFER_DST), + // fill a transient HOST_VISIBLE staging buffer, vkCmdCopyBuffer into + // this buffer, then hand the staging buffer to the fence-keyed + // deferred-deletion queue (#101/#102) so it outlives the copy submit. + // Requires !Mapped: the destination must be free to be device-local-only + // (a persistent map would force HOST_VISIBLE), so the direct path maps + // just long enough to write. A same-size re-upload reuses the allocation + // (Resize), so the device address stays stable across an in-place AS + // UPDATE refit that re-calls this. + void UploadDeviceLocal(VkBufferUsageFlags2 usageFlags, const T* src, std::uint32_t count, VkCommandBuffer cmd, VkAccessFlags dstAccessMask, VkPipelineStageFlags dstStageMask) requires(!Mapped) { + VkDeviceSize bytes = static_cast(count) * sizeof(T); + VkAccessFlags srcAccessMask; + VkPipelineStageFlags srcStageMask; + if (Device::PreferDirectDeviceWrite(bytes)) { + Resize(usageFlags, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, count, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); + void* mapped = nullptr; + Device::CheckVkResult(vkMapMemory(Device::device, memory, 0, VK_WHOLE_SIZE, 0, &mapped)); + std::memcpy(mapped, src, bytes); + // Match FlushDevice()'s gate: a non-coherent type needs an + // explicit flush before the device reads the written range. + if (!(memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)) { + VkMappedMemoryRange range { + .sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE, + .memory = memory, + .offset = 0, + .size = VK_WHOLE_SIZE + }; + vkFlushMappedMemoryRanges(Device::device, 1, &range); + } + vkUnmapMemory(Device::device, memory); + srcAccessMask = VK_ACCESS_HOST_WRITE_BIT; + srcStageMask = VK_PIPELINE_STAGE_HOST_BIT; + } else { + Resize(usageFlags | VK_BUFFER_USAGE_TRANSFER_DST_BIT, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, count); + VulkanBuffer staging; + // SHADER_DEVICE_ADDRESS: Create always queries the buffer device + // address (and allocates with the device-address bit); the + // staging buffer's own address is otherwise unused. + staging.Create(VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, count); + std::memcpy(staging.value, src, bytes); + staging.FlushDevice(); + VkBufferCopy region { .srcOffset = 0, .dstOffset = 0, .size = bytes }; + vkCmdCopyBuffer(cmd, staging.buffer, buffer, 1, ®ion); + // The queued copy still reads the staging buffer after this call + // returns, so a plain Clear() would be a use-after-free; hand it + // to the fence-keyed queue (#101/#102), which frees it once the + // copy's frame clears its fence. DeferredClear nulls the handle, + // so `staging`'s destructor here is a no-op. + staging.DeferredClear(); + srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT; + srcStageMask = VK_PIPELINE_STAGE_TRANSFER_BIT; + } + + VkBufferMemoryBarrier barrier = { + .sType = VK_STRUCTURE_TYPE_BUFFER_MEMORY_BARRIER, + .srcAccessMask = srcAccessMask, + .dstAccessMask = dstAccessMask, + .srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED, + .dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED, + .buffer = buffer, + .offset = 0, + .size = VK_WHOLE_SIZE + }; + vkCmdPipelineBarrier(cmd, srcStageMask, dstStageMask, 0, 0, NULL, 1, &barrier, 0, NULL); + } + void FlushDevice() requires(Mapped) { // Coherent memory needs no explicit flush — host writes are // automatically visible to the device. diff --git a/tests/BLASBuildOptions/main.cpp b/tests/BLASBuildOptions/main.cpp index e8520d8..538fae9 100644 --- a/tests/BLASBuildOptions/main.cpp +++ b/tests/BLASBuildOptions/main.cpp @@ -132,6 +132,15 @@ int main() { // refit below can reuse it (build scratch ≥ update scratch, no resize). Check(tri.scratchBuffer.buffer != VK_NULL_HANDLE, "allowUpdate=true → scratch retained for refit (issue #66)"); + // Geometry is placed in device-local memory regardless of the upload + // strategy taken (issue #73): the direct ReBAR/UMA path lands a + // HOST_VISIBLE | DEVICE_LOCAL type, the staged path a pure DEVICE_LOCAL + // one — either way the DEVICE_LOCAL bit is set, so the BLAS build and any + // hit-shader fetch read from VRAM, not system RAM. + Check((tri.vertexBuffer.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT) != 0, + "triangle vertexBuffer placed in device-local memory (#73)"); + Check((tri.indexBuffer.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT) != 0, + "triangle indexBuffer placed in device-local memory (#73)"); const VkDeviceAddress triAddrBefore = tri.blasAddr; const VkAccelerationStructureKHR triHandleBefore = tri.accelerationStructure; @@ -198,6 +207,8 @@ int main() { Check(proc.builtPrimitiveCount == 2, "procedural BLAS reports 2 primitives"); Check(proc.scratchBuffer.buffer != VK_NULL_HANDLE, "procedural allowUpdate=true → scratch retained for refit (issue #66)"); + Check((proc.aabbBuffer.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT) != 0, + "procedural aabbBuffer placed in device-local memory (#73)"); const VkDeviceAddress procAddrBefore = proc.blasAddr; const VkAccelerationStructureKHR procHandleBefore = proc.accelerationStructure; @@ -293,6 +304,45 @@ int main() { Check(devProc.aabbBuffer.buffer == VK_NULL_HANDLE, "device-buffer refit still never touched the host aabbBuffer"); + // ── Force the STAGED upload path (issue #73). On this ReBAR dev host the + // direct map path is normally taken; temporarily zero the cached upload + // budget so PreferDirectDeviceWrite returns false, exercising Mesh's + // pure-DEVICE_LOCAL + transient-staging-buffer + vkCmdCopyBuffer path + // (and its deferred-deletion of the staging buffer). The validation + // layer check below is the real assertion that the staged copy and its + // barriers are spec-correct on a fresh build and an in-place refit. ─── + { + const VkDeviceSize savedBudget = Device::directWriteBudget; + Device::directWriteBudget = 0; // 0 → PreferDirectDeviceWrite is always false + + Mesh staged; + auto verts = CubeVerts(1.0f); + auto idx = CubeIndices(); + { + VkCommandBuffer cmd = BeginCmd(); + staged.Build(verts, idx, cmd, RTBuildOptions{ + .preference = RTBuildPreference::FastTrace, .allowUpdate = true }); + SubmitWait(cmd); + } + Check(staged.blasAddr != 0, "staged-upload Build produced a non-zero blasAddr (#73)"); + // Staged geometry lives in pure DEVICE_LOCAL — no HOST_VISIBLE bit. + Check((staged.vertexBuffer.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT) == 0, + "staged vertexBuffer is device-local-only, not host-visible (#73)"); + + const VkAccelerationStructureKHR stagedHandle = staged.accelerationStructure; + { + // Same topology, deformed positions → in-place UPDATE, re-staged. + auto verts2 = CubeVerts(1.5f); + VkCommandBuffer cmd = BeginCmd(); + staged.Refit(verts2, idx, cmd); + SubmitWait(cmd); + } + Check(staged.accelerationStructure == stagedHandle, + "staged-upload Refit kept the same AS handle (in-place UPDATE, #73)"); + + Device::directWriteBudget = savedBudget; + } + Check(Device::validationErrorCount == 0, std::format("no Vulkan validation errors ({} seen)", Device::validationErrorCount));