diff --git a/implementations/Crafter.Graphics-Mesh.cpp b/implementations/Crafter.Graphics-Mesh.cpp index f9cc990..e518f2d 100644 --- a/implementations/Crafter.Graphics-Mesh.cpp +++ b/implementations/Crafter.Graphics-Mesh.cpp @@ -281,6 +281,16 @@ void Mesh::Build(const CompressedMeshAsset& asset, VkCommandBuffer cmd, RTBuildO VK_PIPELINE_STAGE_2_ACCELERATION_STRUCTURE_BUILD_BIT_KHR, VK_ACCESS_2_ACCELERATION_STRUCTURE_READ_BIT_KHR); + // The compressed staging is only read by the decompress recorded above; the + // subsequent BLAS build reads the decompressed vertex/index buffers, never + // this. So hand it to the fence-keyed deletion queue (#101/#102) now rather + // than pinning host-visible memory for the mesh's whole life. The recorded + // vkCmdDecompressMemoryEXT still references compressedStaging.address, so it + // must outlive this submit — which the queue guarantees: it retires the + // allocation only after framesInFlight frames have elapsed, by which point + // the decompress submit's fence has cleared. + compressedStaging.DeferredClear(); + allowUpdate = options.allowUpdate; builtInputCount = asset.vertexCount; RecordBLASBuild(*this, asset.vertexCount, asset.indexCount, BlasFlags(options), /*update*/ false, cmd); diff --git a/implementations/Crafter.Graphics-RenderingElement3D.cpp b/implementations/Crafter.Graphics-RenderingElement3D.cpp index 5eea69c..d06630c 100644 --- a/implementations/Crafter.Graphics-RenderingElement3D.cpp +++ b/implementations/Crafter.Graphics-RenderingElement3D.cpp @@ -106,10 +106,38 @@ void RenderingElement3D::BuildTLAS(VkCommandBuffer cmd, std::uint32_t index, RTB // STORAGE_BUFFER_BIT is required because the application's compute // shaders bind these buffers as storage SSBOs (e.g. to write // per-instance transforms directly into the TLAS instance data). + // + // instanceBuffer prefers HOST_VISIBLE | DEVICE_LOCAL (BAR/VRAM) so the + // two per-frame GPU accesses stay on-chip instead of crossing PCIe: the + // compute shader writes the transform field in place, and the AS build + // reads the whole instance. The CPU still writes the host-authored + // fields below — those are write-only, sequential, never-read-back + // writes, the ideal write-combined BAR workload. DEVICE_LOCAL is a + // best-effort preference: GetMemoryType falls back to plain + // HOST_VISIBLE (the previous behaviour) when no combined type exists + // (no resizable BAR). HOST_COHERENT is intentionally dropped from the + // required set — the combined type may not be coherent, and the + // FlushDevice(cmd, ...) below already establishes host->build + // 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 | 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 | VK_MEMORY_PROPERTY_HOST_COHERENT_BIT, primitiveCount); + 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, primitiveCount, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT); } } @@ -132,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/implementations/Crafter.Graphics-Window.cpp b/implementations/Crafter.Graphics-Window.cpp index 0afe9d8..7be3d4e 100644 --- a/implementations/Crafter.Graphics-Window.cpp +++ b/implementations/Crafter.Graphics-Window.cpp @@ -1200,11 +1200,29 @@ void Window::SaveFrame(const std::filesystem::path& path) { VkMemoryRequirements memReqs; vkGetBufferMemoryRequirements(Device::device, stagingBuf, &memReqs); + + // Readback path: the CPU *reads* this whole buffer back. The first + // HOST_VISIBLE|HOST_COHERENT type is usually write-combined (uncached) on + // discrete GPUs, where CPU reads are pathologically slow. Ask for + // HOST_CACHED so reads hit the CPU cache, preferring a cached-coherent type + // when one exists. HOST_CACHED is near-universal but not spec-guaranteed, + // so fall back to any HOST_VISIBLE type (#59) if absent — that path then + // relies on the coherency-gated invalidate below. (Deliberately NOT routed + // through the upload-strategy helper, which steers toward write-combined + // DEVICE_LOCAL|HOST_VISIBLE — the worst type for readback. See issue #89.) + std::uint32_t memTypeIndex; + try { + memTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits, + VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_CACHED_BIT, + VK_MEMORY_PROPERTY_HOST_COHERENT_BIT); + } catch (const std::runtime_error&) { + memTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits, + VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT); + } VkMemoryAllocateInfo mai{ .sType = VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO, .allocationSize = memReqs.size, - .memoryTypeIndex = Device::GetMemoryType(memReqs.memoryTypeBits, - VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT | VK_MEMORY_PROPERTY_HOST_COHERENT_BIT), + .memoryTypeIndex = memTypeIndex, }; VkDeviceMemory stagingMem = VK_NULL_HANDLE; Device::CheckVkResult(vkAllocateMemory(Device::device, &mai, nullptr, &stagingMem)); @@ -1289,6 +1307,24 @@ void Window::SaveFrame(const std::filesystem::path& path) { // Read back, swizzle BGRA → RGBA if needed, write PNG. void* mapped = nullptr; Device::CheckVkResult(vkMapMemory(Device::device, stagingMem, 0, VK_WHOLE_SIZE, 0, &mapped)); + + // If the chosen type is cached-but-not-coherent, the transfer's writes may + // not yet be visible to the CPU read; invalidate to pull them in. Gate on + // the chosen type's actual coherency (not the requested flags) — a coherent + // type needs no invalidate. Whole-range invalidate sidesteps + // nonCoherentAtomSize. This is the read-path counterpart of FlushDevice (#60). + if (!(Device::memoryProperties.memoryTypes[memTypeIndex].propertyFlags & + VK_MEMORY_PROPERTY_HOST_COHERENT_BIT)) { + VkMappedMemoryRange invalidateRange{ + .sType = VK_STRUCTURE_TYPE_MAPPED_MEMORY_RANGE, + .memory = stagingMem, + .offset = 0, + .size = VK_WHOLE_SIZE, + }; + Device::CheckVkResult( + vkInvalidateMappedMemoryRanges(Device::device, 1, &invalidateRange)); + } + const std::uint8_t* src = static_cast(mapped); std::vector rgba(static_cast(width) * height * 4); diff --git a/interfaces/Crafter.Graphics-ImageVulkan.cppm b/interfaces/Crafter.Graphics-ImageVulkan.cppm index 6630e2c..2c40a01 100644 --- a/interfaces/Crafter.Graphics-ImageVulkan.cppm +++ b/interfaces/Crafter.Graphics-ImageVulkan.cppm @@ -75,9 +75,12 @@ export namespace Crafter { VkImage image; VkDeviceMemory imageMemory; VulkanBuffer buffer; - // Lives until the compressed Update path's cmd buffer completes. - // Same lifetime contract as Mesh::compressedStaging — caller must - // not destroy / re-Update before the submit fence is signaled. + // Transient host-visible staging for the compressed Update path. Same + // lifetime contract as Mesh::compressedStaging: the compressed Update + // releases it via DeferredClear() right after recording the decompress, + // so the fence-keyed deletion queue (#101/#102) frees it once that + // submit's frame has cleared instead of pinning it for the image's life. + // Between Updates the handle is null; the next Update's Resize re-creates it. VulkanBuffer compressedStaging; VkImageView imageView; VkDescriptorImageInfo descriptor; @@ -299,6 +302,16 @@ export namespace Crafter { VK_PIPELINE_STAGE_2_COPY_BIT, VK_ACCESS_2_TRANSFER_READ_BIT); + // Compressed staging is read only by the decompress recorded above; + // the buffer→image copy below reads `buffer` (the decompress dst), + // never this. Release it to the fence-keyed deletion queue + // (#101/#102) now instead of pinning host-visible memory for the + // image's whole life. The recorded vkCmdDecompressMemoryEXT still + // references compressedStaging.address, so it must outlive this + // submit — which the queue guarantees by retiring the allocation + // only after framesInFlight frames (i.e. after the submit's fence). + compressedStaging.DeferredClear(); + // Continue with the existing buffer→image upload + layout transitions. // We've already inserted the decompress→transfer-read barrier, // so we skip the FlushDevice host-write barrier the regular Update diff --git a/interfaces/Crafter.Graphics-Mesh.cppm b/interfaces/Crafter.Graphics-Mesh.cppm index 91341a5..1a84574 100644 --- a/interfaces/Crafter.Graphics-Mesh.cppm +++ b/interfaces/Crafter.Graphics-Mesh.cppm @@ -83,12 +83,13 @@ export namespace Crafter { // Lifetime contract matches vertexBuffer/indexBuffer: must stay // alive until the build submitted on `cmd` completes. 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 - // submit signals — caller must not re-Build or destroy the Mesh - // before that submit's fence is signaled (same contract as the - // existing uncompressed path). + // Transient host-visible staging for the compressed Build path. Kept as + // a member only so the recorded vkCmdDecompressMemoryEXT has a stable + // address to reference; the compressed Build releases it via + // DeferredClear() right after recording the decompress, so it is freed + // by the fence-keyed deletion queue (#101/#102) once that submit's frame + // has cleared — no longer pinned for the mesh's life. Between Builds the + // handle is null; the next Build's Resize re-creates it. VulkanBuffer compressedStaging; VkAccelerationStructureGeometryTrianglesDataKHR blasData; VkAccelerationStructureGeometryKHR blas; diff --git a/interfaces/Crafter.Graphics-PipelineRTVulkan.cppm b/interfaces/Crafter.Graphics-PipelineRTVulkan.cppm index 9da555e..527753d 100644 --- a/interfaces/Crafter.Graphics-PipelineRTVulkan.cppm +++ b/interfaces/Crafter.Graphics-PipelineRTVulkan.cppm @@ -103,7 +103,30 @@ export namespace Crafter { hitRegion.size = hitGroups.size() * sbtStride; std::size_t bufferSize = hitRegion.deviceAddress + hitRegion.size; - sbtBuffer.Create(VK_BUFFER_USAGE_2_SHADER_BINDING_TABLE_BIT_KHR | VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, bufferSize); + // The SBT is written once here (the memcpys below) and read by the + // GPU on every vkCmdTraceRaysKHR for the pipeline's lifetime — the + // textbook write-once/read-many buffer (issue #72). Get it into + // device-local memory so trace dispatches read raygen/miss/hit + // records out of VRAM instead of over PCIe from system RAM. + // + // Route the placement through #89: when a DEVICE_LOCAL|HOST_VISIBLE + // type exists (ReBAR/UMA, or a BAR window — the SBT is tiny so the + // window budget is never the constraint), prefer DEVICE_LOCAL on top + // of the required HOST_VISIBLE so the allocation lands in device + // memory we can still map. The one-time memcpy goes write-combined + // over PCIe (write-only, sequential — ideal); GPU reads then hit + // local VRAM. When no combined type exists at all (no spec + // guarantee), PreferDirectDeviceWrite returns false and the preferred + // hint is dropped, so GetMemoryType falls back to plain HOST_VISIBLE + // (current behaviour, the per-trace PCIe read) — the cheaper fallback + // the issue blesses while staging isn't wired here. + // + // Either way the buffer stays mapped, and the FlushDevice below gates + // its flush on the *chosen* memory type's flags (issue #60), so a + // direct-write type lacking HOST_COHERENT is still flushed correctly. + VkMemoryPropertyFlags sbtPreferred = Device::PreferDirectDeviceWrite(bufferSize) + ? VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT : 0; + sbtBuffer.Create(VK_BUFFER_USAGE_2_SHADER_BINDING_TABLE_BIT_KHR | VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT, VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, bufferSize, sbtPreferred); std::uint8_t* offset = sbtBuffer.value; std::uint8_t* handleOffset = shaderHandles.data(); diff --git a/project.cpp b/project.cpp index 879108f..97d51b5 100644 --- a/project.cpp +++ b/project.cpp @@ -617,6 +617,40 @@ extern "C" Configuration CrafterBuildProject(std::span a dc.GetInterfacesAndImplementations(ifaces, deferredImpls); cfg.tests.push_back(std::move(deferredTest)); + // Issue #67: the compressed Mesh::Build path no longer pins its + // host-visible `compressedStaging` for the mesh's life — it releases it + // via DeferredClear() right after recording the GPU decompress, so the + // fence-keyed deletion queue (#101/#102) frees it once that submit's + // frame has cleared. Drives the real VK_EXT_memory_decompression / + // GDeflate path on a headless device (no swapchain/window — a decompress + // + BLAS build only needs the queue + command pool) and asserts the + // staging is enqueued (not pinned), the build is validation-clean with + // correct decompressed data, and the entry retires only after + // framesInFlight frames. Shares the native build settings; needs the + // asset pipeline for SaveCompressed/LoadCompressedMesh (cfg.dependencies + // already carries Crafter.Asset). + Test meshStagingTest; + Configuration& msc = meshStagingTest.config; + msc.path = cfg.path; + msc.name = "MeshDecompressStagingRelease"; + msc.outputName = "MeshDecompressStagingRelease"; + msc.type = ConfigurationType::Executable; + msc.target = cfg.target; + msc.march = cfg.march; + msc.mtune = cfg.mtune; + msc.debug = cfg.debug; + msc.sysroot = cfg.sysroot; + msc.dependencies = cfg.dependencies; + msc.externalDependencies = cfg.externalDependencies; + msc.compileFlags = cfg.compileFlags; + msc.linkFlags = cfg.linkFlags; + msc.defines = cfg.defines; + msc.cFiles = cfg.cFiles; + std::vector meshStagingImpls(impls.begin(), impls.end()); + meshStagingImpls.emplace_back("tests/MeshDecompressStagingRelease/main"); + msc.GetInterfacesAndImplementations(ifaces, meshStagingImpls); + cfg.tests.push_back(std::move(meshStagingTest)); + // Issue #89: Device::PreferDirectDeviceWrite chooses the upload strategy // for a CPU-written, GPU-read buffer — direct HOST_VISIBLE|DEVICE_LOCAL // map+write on ReBAR/UMA vs. staged-into-pure-DEVICE_LOCAL on a small diff --git a/tests/MeshDecompressStagingRelease/main.cpp b/tests/MeshDecompressStagingRelease/main.cpp new file mode 100644 index 0000000..9f93e77 --- /dev/null +++ b/tests/MeshDecompressStagingRelease/main.cpp @@ -0,0 +1,197 @@ +/* +Crafter®.Graphics +Copyright (C) 2026 Catcrafts® +catcrafts.net + +This library is free software; you can redistribute it and/or +modify it under the terms of the GNU Lesser General Public +License version 3.0 as published by the Free Software Foundation; + +This library is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public +License along with this library; if not, write to the Free Software +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +// Issue #67: the compressed Mesh::Build path used to keep `compressedStaging` +// — a per-mesh host-visible staging buffer holding the GDeflate streams — +// alive for the whole life of the mesh, pinning host-visible memory long after +// the one decompress that reads it has retired. Build now releases it via +// VulkanBuffer::DeferredClear() right after recording the decompress, so the +// fence-keyed deletion queue (#101/#102) frees it once that submit's frame has +// cleared instead of pinning it forever. +// +// This drives the real hardware GPU-decompress path (VK_EXT_memory_decompression, +// GDeflate 1.0) on a headless device — no swapchain/window needed, a decompress + +// BLAS build only touches the queue + command pool — and asserts: +// - After Build, mesh.compressedStaging owns no handle (released). +// - Exactly that one allocation was handed to Device's deletion queue, tagged +// with the current frameCounter (so it retires on a later frame, not now). +// - The decompress + BLAS build still complete with ZERO validation errors and +// a non-zero BLAS address — proof the staging genuinely outlived the submit +// (the queue had not yet retired it), so this is no use-after-free. +// - The GPU-decompressed vertex/index data read back byte-equal to the source, +// so releasing the staging didn't corrupt the result. +// - The deferred entry actually retires (and frees) once framesInFlight frames +// elapse, and not before — the whole point of routing through the queue. +// +// On hardware without VK_EXT_memory_decompression the compressed Build takes the +// CPU-decode fallback, which never allocates compressedStaging, so the new +// behavior is moot — the test reports that and skips the GPU-path assertions. + +#include "vulkan/vulkan.h" +#include + +import Crafter.Graphics; +import Crafter.Asset; +import Crafter.Math; +import std; + +using namespace Crafter; +namespace fs = std::filesystem; + +namespace { + +int failures = 0; + +void Check(bool ok, std::string_view what) { + std::println("{} {}", ok ? "PASS" : "FAIL", what); + if (!ok) ++failures; +} + +// One-time command buffer helpers — record, submit, block. Mirrors the other +// headless RT tests (BLASBuildOptions). +VkCommandBuffer BeginCmd() { + VkCommandBufferAllocateInfo allocInfo { + .sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO, + .commandPool = Device::commandPool, + .level = VK_COMMAND_BUFFER_LEVEL_PRIMARY, + .commandBufferCount = 1, + }; + VkCommandBuffer cmd = VK_NULL_HANDLE; + Device::CheckVkResult(vkAllocateCommandBuffers(Device::device, &allocInfo, &cmd)); + VkCommandBufferBeginInfo beginInfo { + .sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO, + .flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT, + }; + Device::CheckVkResult(vkBeginCommandBuffer(cmd, &beginInfo)); + return cmd; +} + +void SubmitWait(VkCommandBuffer cmd) { + Device::CheckVkResult(vkEndCommandBuffer(cmd)); + VkSubmitInfo submitInfo { + .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO, + .commandBufferCount = 1, + .pCommandBuffers = &cmd, + }; + Device::CheckVkResult(vkQueueSubmit(Device::queue, 1, &submitInfo, VK_NULL_HANDLE)); + Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + vkFreeCommandBuffers(Device::device, Device::commandPool, 1, &cmd); +} + +// A unit cube (8 verts, 36 indices) — small enough to fit one GDeflate tile, +// enough topology for a real BLAS. +MeshAsset MakeCubeMesh() { + MeshAsset mesh; + mesh.vertexes = { + {-1.f, -1.f, -1.f}, { 1.f, -1.f, -1.f}, + { 1.f, 1.f, -1.f}, {-1.f, 1.f, -1.f}, + {-1.f, -1.f, 1.f}, { 1.f, -1.f, 1.f}, + { 1.f, 1.f, 1.f}, {-1.f, 1.f, 1.f}, + }; + mesh.indexes = { + 0,1,2, 0,2,3, 4,6,5, 4,7,6, + 0,4,5, 0,5,1, 3,2,6, 3,6,7, + 1,5,6, 1,6,2, 0,3,7, 0,7,4, + }; + return mesh; +} + +} // namespace + +int main() { + Device::Initialize(); + Device::validationErrorCount = 0; + + // Build → compress → load a cube via the real asset pipeline. + MeshAsset srcMesh = MakeCubeMesh(); + const fs::path meshPath = + fs::temp_directory_path() / "crafter_meshdecompress_staging_release.cmesh"; + srcMesh.SaveCompressed(meshPath); + CompressedMeshAsset asset = LoadCompressedMesh(meshPath); + fs::remove(meshPath); + + Check(asset.vertexCount == srcMesh.vertexes.size() + && asset.indexCount == srcMesh.indexes.size(), + "LoadCompressedMesh round-trips the cube header"); + + if (!Device::memoryDecompressionSupported) { + // No GPU codec → compressed Build takes the CPU-decode fallback, which + // never allocates compressedStaging. The release behavior under test + // only exists on the GPU path, so there is nothing to assert here. + std::println("VK_EXT_memory_decompression absent — GPU decompress path " + "not exercised; skipping staging-release assertions."); + return failures == 0 ? 0 : 1; + } + + // Control the deferred-deletion clock: enqueue at frame 0, retire after 2. + Device::framesInFlight = 2; + Device::frameCounter = 0; + Device::deletionQueue.clear(); + + Mesh mesh; + VkCommandBuffer cmd = BeginCmd(); + mesh.Build(asset, cmd); + + // ── The fix: staging is released to the queue at record time, not pinned. ── + Check(mesh.compressedStaging.buffer == VK_NULL_HANDLE, + "Build releases compressedStaging (no handle pinned to the mesh)"); + Check(Device::deletionQueue.size() == 1, + "exactly one allocation (the staging) handed to the deletion queue"); + const bool taggedNow = !Device::deletionQueue.empty() + && Device::deletionQueue.front().retireAfter == 0 + && Device::deletionQueue.front().buffer != VK_NULL_HANDLE; + Check(taggedNow, + "deferred staging is tagged with the enqueue frame and still a live handle"); + + // The decompress recorded above still references the staging's address; it + // must outlive THIS submit. The queue hasn't retired it (frame 0, retires at + // 2), so the GPU reads valid memory. Zero validation errors + a real BLAS + // address prove there's no use-after-free. + SubmitWait(cmd); + Check(mesh.blasAddr != 0, "compressed Build produced a non-zero BLAS address"); + Check(Device::validationErrorCount == 0, + "decompress + BLAS build raised no validation errors"); + + // The GPU decompress wrote into the host-visible vertex/index buffers; + // invalidate and read them back — releasing the staging must not corrupt + // the decompressed result. + mesh.vertexBuffer.FlushHost(); + mesh.indexBuffer.FlushHost(); + const bool vertsMatch = std::memcmp( + mesh.vertexBuffer.value, srcMesh.vertexes.data(), + srcMesh.vertexes.size() * sizeof(srcMesh.vertexes[0])) == 0; + const bool idxMatch = std::memcmp( + mesh.indexBuffer.value, srcMesh.indexes.data(), + srcMesh.indexes.size() * sizeof(srcMesh.indexes[0])) == 0; + Check(vertsMatch, "GPU-decompressed vertices are byte-equal to the source"); + Check(idxMatch, "GPU-decompressed indices are byte-equal to the source"); + + // ── Retire timing: the staging frees once framesInFlight frames elapse, + // and not a frame before. We've already wait-idled, so freeing is safe. ── + Device::ReclaimDeletions(); + Check(Device::deletionQueue.size() == 1, + "staging is NOT freed before framesInFlight frames have elapsed"); + Device::frameCounter = 2; // retireAfter(0) + framesInFlight(2) <= 2 + Device::ReclaimDeletions(); + Check(Device::deletionQueue.empty(), + "staging is freed once its retire frame is reached"); + + std::println("{}", failures == 0 ? "ALL PASS" : "FAILURES PRESENT"); + return failures == 0 ? 0 : 1; +} diff --git a/tests/TLASHighWaterMark/main.cpp b/tests/TLASHighWaterMark/main.cpp index d35de8a..987b960 100644 --- a/tests/TLASHighWaterMark/main.cpp +++ b/tests/TLASHighWaterMark/main.cpp @@ -50,6 +50,23 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA // — the strongest check that feeding an oversized instance buffer to the // AS build (with tlasRangeInfo.primitiveCount < capacity) is spec-correct. // +// Issue #65: this test also guards the instance buffer's memory-type request — +// HOST_VISIBLE (required) with a best-effort DEVICE_LOCAL preference and no +// mandatory HOST_COHERENT, so the per-frame GPU accesses stay in local VRAM +// (BAR) rather than crossing PCIe. See the assertion block after the first +// build. The zero-validation-errors check below also covers the dropped +// 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. @@ -207,6 +224,94 @@ int main() { Check(RenderingElement3D::tlases[0].builtInstanceCount == 4, "builtInstanceCount == 4 after first build"); + // ── Issue #65: the instance buffer is allocated HOST_VISIBLE (required, + // mappable) with a best-effort DEVICE_LOCAL preference (BAR/VRAM) and the + // mandatory HOST_COHERENT dropped — so the per-frame compute-write of the + // transform field and the AS build's read of the instances both stay in + // local VRAM instead of crossing PCIe. The chosen type must match exactly + // what GetMemoryType resolves for this buffer's memoryTypeBits with that + // prefer-but-don't-require request: DEVICE_LOCAL when a host-visible + // device-local type is reachable (resizable BAR), plain HOST_VISIBLE + // otherwise. Asserting equality with the recomputed selection makes the + // check device-independent — it tracks the call-site request, not the + // particular GPU this runs on. ──────────────────────────────────────────── + { + auto& ib = RenderingElement3D::tlases[0].instanceBuffer; + VkMemoryRequirements req{}; + vkGetBufferMemoryRequirements(Device::device, ib.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((ib.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT) != 0, + "instance buffer memory is HOST_VISIBLE (mappable — the required flag)"); + Check(ib.memoryPropertyFlagsChosen == expectFlags, + "instance buffer memory matches the HOST_VISIBLE | prefer-DEVICE_LOCAL request (#65)"); + + 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((ib.memoryPropertyFlagsChosen & VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT) != 0, + "instance buffer landed on a DEVICE_LOCAL (BAR/VRAM) type where one is reachable"); + } else { + std::println("INFO no host-visible device-local type reachable — " + "fell back to plain HOST_VISIBLE (expected on non-BAR GPUs)"); + } + } + + // ── 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();