From a5a77604775c500c91fa663cb0eec517960de0c9 Mon Sep 17 00:00:00 2001 From: tanmaysachan Date: Wed, 2 Apr 2025 14:25:51 +0530 Subject: [PATCH 1/5] Fix race on resource release + fixes for unleashed --- plume_metal.cpp | 16 +++++++--------- plume_metal.h | 1 + 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/plume_metal.cpp b/plume_metal.cpp index 50a02b8..6409306 100644 --- a/plume_metal.cpp +++ b/plume_metal.cpp @@ -26,11 +26,6 @@ namespace plume { static constexpr size_t PUSH_CONSTANT_MAX_INDEX = 15; static constexpr size_t VERTEX_BUFFER_MAX_INDEX = 30; - static constexpr uint32_t COLOR_COUNT = DESCRIPTOR_SET_MAX_INDEX; - static constexpr uint32_t DEPTH_INDEX = COLOR_COUNT; - static constexpr uint32_t STENCIL_INDEX = DEPTH_INDEX + 1; - static constexpr uint32_t ATTACHMENT_COUNT = STENCIL_INDEX + 1; - // MARK: - Prototypes MTL::PixelFormat mapPixelFormat(RenderFormat format); @@ -1605,6 +1600,10 @@ namespace plume { } MetalDescriptorSet::~MetalDescriptorSet() { + for (const auto resource: toReleaseOnDestruction) { + resource->release(); + } + for (const auto &entry : resourceEntries) { if (entry.resource != nullptr) { entry.resource->release(); @@ -1700,8 +1699,7 @@ namespace plume { if (dtype != MTL::DataTypeSampler) { if (resourceEntries[descriptorIndex].resource != nullptr) { - resourceEntries[descriptorIndex].resource->release(); - resourceEntries[descriptorIndex].resource = nullptr; + toReleaseOnDestruction.push_back(resourceEntries[descriptorIndex].resource); } } @@ -2185,7 +2183,7 @@ namespace plume { pushConstants[rangeIndex].binding = range.binding; pushConstants[rangeIndex].set = range.set; pushConstants[rangeIndex].offset = range.offset; - pushConstants[rangeIndex].size = range.size; + pushConstants[rangeIndex].size = alignUp(range.size); pushConstants[rangeIndex].stageFlags = range.stageFlags; dirtyComputeState.pushConstants = 1; @@ -2236,7 +2234,7 @@ namespace plume { pushConstants[rangeIndex].binding = range.binding; pushConstants[rangeIndex].set = range.set; pushConstants[rangeIndex].offset = range.offset; - pushConstants[rangeIndex].size = range.size; + pushConstants[rangeIndex].size = alignUp(range.size); pushConstants[rangeIndex].stageFlags = range.stageFlags; dirtyGraphicsState.pushConstants = 1; diff --git a/plume_metal.h b/plume_metal.h index 55b0b6b..7364db4 100644 --- a/plume_metal.h +++ b/plume_metal.h @@ -209,6 +209,7 @@ namespace plume { std::vector descriptors; MetalArgumentBuffer argumentBuffer; std::vector resourceEntries; + std::vector toReleaseOnDestruction; MetalDescriptorSet(MetalDevice *device, const RenderDescriptorSetDesc &desc); MetalDescriptorSet(MetalDevice *device, uint32_t entryCount); From 9e867e3240460d2e1cc9b0d7985a322cfb75de59 Mon Sep 17 00:00:00 2001 From: tanmaysachan Date: Wed, 2 Apr 2025 14:52:53 +0530 Subject: [PATCH 2/5] Add nullBuffer and stride=0 support --- plume_metal.cpp | 35 +++++++++++++++++++++++++++++++---- plume_metal.h | 2 ++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/plume_metal.cpp b/plume_metal.cpp index 6409306..2e34729 100644 --- a/plume_metal.cpp +++ b/plume_metal.cpp @@ -1430,9 +1430,18 @@ namespace plume { // Set index right after push constants, clamp at Metal's limit of 31 const uint32_t vertexBufferIndex = std::min(PUSH_CONSTANT_MAX_INDEX + 1 + inputSlot.index, VERTEX_BUFFER_MAX_INDEX); MTL::VertexBufferLayoutDescriptor *layout = vertexDescriptor->layouts()->object(vertexBufferIndex); - layout->setStride(inputSlot.stride); - layout->setStepFunction(mapVertexStepFunction(inputSlot.classification)); - layout->setStepRate((layout->stepFunction() == MTL::VertexStepFunctionPerInstance) ? inputSlot.stride : 1); + if (inputSlot.stride == 0) { + // Metal does not support stride 0, we must provide a + // substitute "null" buffer to match behaviour of other robust APIs. + // hasNullBuffer = true; + layout->setStride(1); + layout->setStepFunction(MTL::VertexStepFunctionConstant); + layout->setStepRate(0); + } else { + layout->setStride(inputSlot.stride); + layout->setStepFunction(mapVertexStepFunction(inputSlot.classification)); + layout->setStepRate((layout->stepFunction() == MTL::VertexStepFunctionPerInstance) ? inputSlot.stride : 1); + } } for (uint32_t i = 0; i < desc.inputElementsCount; i++) { @@ -2289,9 +2298,14 @@ namespace plume { // Check for changes in bindings for (uint32_t i = 0; i < viewCount; i++) { const MetalBuffer* interfaceBuffer = static_cast(views[i].buffer.ref); - const uint64_t newOffset = views[i].buffer.offset; + uint64_t newOffset = views[i].buffer.offset; const uint32_t newIndex = startSlot + i; + if (interfaceBuffer == nullptr) { + interfaceBuffer = static_cast(queue->device->nullBuffer.get()); + newOffset = 0; + } + vertexBuffers[i] = interfaceBuffer->mtl; vertexBufferOffsets[i] = newOffset; vertexBufferIndices[i] = newIndex; @@ -2929,10 +2943,21 @@ namespace plume { } if (dirtyGraphicsState.vertexBuffers) { + std::vector slotUsed(31, false); + for (uint32_t i = 0; i < viewCount; i++) { // Bind right after the push constants, up till the max vertex buffer index const uint32_t bindIndex = std::min(PUSH_CONSTANT_MAX_INDEX + 1 + vertexBufferIndices[i], VERTEX_BUFFER_MAX_INDEX); activeRenderEncoder->setVertexBuffer(vertexBuffers[i], vertexBufferOffsets[i], bindIndex); + slotUsed[bindIndex] = true; + } + + // Bind all unbound slots to the null buffer + auto nullBuffer = static_cast(queue->device->nullBuffer.get()); + for (uint32_t i = PUSH_CONSTANT_MAX_INDEX + 1; i <= VERTEX_BUFFER_MAX_INDEX; i++) { + if (!slotUsed[i]) { + activeRenderEncoder->setVertexBuffer(nullBuffer->mtl, 0, i); + } } stateCache.lastVertexBuffers = vertexBuffers; @@ -3229,6 +3254,8 @@ namespace plume { capabilities.dynamicDepthBias = true; capabilities.uma = mtl->hasUnifiedMemory(); capabilities.queryPools = false; + + nullBuffer = createBuffer(RenderBufferDesc::DefaultBuffer(16, RenderBufferFlag::VERTEX)); } MetalDevice::~MetalDevice() { diff --git a/plume_metal.h b/plume_metal.h index 7364db4..375f1c4 100644 --- a/plume_metal.h +++ b/plume_metal.h @@ -594,6 +594,8 @@ namespace plume { // Blit functionality MTL::BlitPassDescriptor *sharedBlitDescriptor = nullptr; + std::unique_ptr nullBuffer; + explicit MetalDevice(MetalInterface *renderInterface, const std::string &preferredDeviceName); ~MetalDevice() override; std::unique_ptr createDescriptorSet(const RenderDescriptorSetDesc &desc) override; From a20b6199e2038e541d82ee1177f17c6328acb3de Mon Sep 17 00:00:00 2001 From: squidbus <175574877+squidbus@users.noreply.github.com> Date: Mon, 28 Jul 2025 12:50:56 -0700 Subject: [PATCH 3/5] Use array instead of vector for vertex slot tracking. --- plume_metal.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plume_metal.cpp b/plume_metal.cpp index 2e34729..251024f 100644 --- a/plume_metal.cpp +++ b/plume_metal.cpp @@ -2943,7 +2943,7 @@ namespace plume { } if (dirtyGraphicsState.vertexBuffers) { - std::vector slotUsed(31, false); + std::array slotUsed = {false}; for (uint32_t i = 0; i < viewCount; i++) { // Bind right after the push constants, up till the max vertex buffer index From 62089d8278e196fe3864a8c55691230c57e4baa8 Mon Sep 17 00:00:00 2001 From: squidbus <175574877+squidbus@users.noreply.github.com> Date: Mon, 28 Jul 2025 12:54:04 -0700 Subject: [PATCH 4/5] Remove extra comment --- plume_metal.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/plume_metal.cpp b/plume_metal.cpp index 251024f..4187c47 100644 --- a/plume_metal.cpp +++ b/plume_metal.cpp @@ -1433,7 +1433,6 @@ namespace plume { if (inputSlot.stride == 0) { // Metal does not support stride 0, we must provide a // substitute "null" buffer to match behaviour of other robust APIs. - // hasNullBuffer = true; layout->setStride(1); layout->setStepFunction(MTL::VertexStepFunctionConstant); layout->setStepRate(0); From cefce65f31e0fd5f430c832a265df80c815c443e Mon Sep 17 00:00:00 2001 From: squidbus <175574877+squidbus@users.noreply.github.com> Date: Mon, 28 Jul 2025 13:02:32 -0700 Subject: [PATCH 5/5] Fix slotUsed initialization --- plume_metal.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plume_metal.cpp b/plume_metal.cpp index 4187c47..d4ffe4d 100644 --- a/plume_metal.cpp +++ b/plume_metal.cpp @@ -2942,7 +2942,7 @@ namespace plume { } if (dirtyGraphicsState.vertexBuffers) { - std::array slotUsed = {false}; + std::array slotUsed{}; for (uint32_t i = 0; i < viewCount; i++) { // Bind right after the push constants, up till the max vertex buffer index