From 3a086486b6f926086327f569bdf0622b54cc5f60 Mon Sep 17 00:00:00 2001 From: tanmaysachan Date: Wed, 2 Apr 2025 14:25:51 +0530 Subject: [PATCH 1/2] Fix race on resource release + fixes for unleashed --- plume_metal.cpp | 18 ++++++++---------- plume_metal.h | 3 ++- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/plume_metal.cpp b/plume_metal.cpp index 23e4f25..fa68dc6 100644 --- a/plume_metal.cpp +++ b/plume_metal.cpp @@ -23,14 +23,9 @@ namespace plume { // MARK: - Constants static constexpr size_t MAX_DRAWABLES = 3; - static constexpr size_t PUSH_CONSTANT_MAX_INDEX = 15; + static constexpr size_t PUSH_CONSTANT_MAX_INDEX = 4; 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); @@ -1518,6 +1513,10 @@ namespace plume { } MetalDescriptorSet::~MetalDescriptorSet() { + for (const auto resource: toReleaseOnDestruction) { + resource->release(); + } + for (const auto &entry : resourceEntries) { if (entry.resource != nullptr) { entry.resource->release(); @@ -1613,8 +1612,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); } } @@ -2098,7 +2096,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; @@ -2149,7 +2147,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 9c317f8..384fbd0 100644 --- a/plume_metal.h +++ b/plume_metal.h @@ -41,7 +41,7 @@ namespace plume { static constexpr size_t MAX_CLEAR_RECTS = 16; static constexpr uint32_t MAX_BINDING_NUMBER = 128; - static constexpr size_t DESCRIPTOR_SET_MAX_INDEX = 8; + static constexpr size_t DESCRIPTOR_SET_MAX_INDEX = 4; struct MetalInterface; struct MetalDevice; @@ -207,6 +207,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 dc9bf8a5ad7abbca350c75930bb0625ecf9124e5 Mon Sep 17 00:00:00 2001 From: tanmaysachan Date: Wed, 2 Apr 2025 14:52:53 +0530 Subject: [PATCH 2/2] 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 fa68dc6..10cbece 100644 --- a/plume_metal.cpp +++ b/plume_metal.cpp @@ -1373,9 +1373,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++) { @@ -2209,9 +2218,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; + } + // Check if this binding differs from current state needsUpdate = i >= stateCache.lastVertexBuffers.size() || interfaceBuffer->mtl != stateCache.lastVertexBuffers[i] || newOffset != stateCache.lastVertexBufferOffsets[i] || newIndex != stateCache.lastVertexBufferIndices[i]; @@ -2834,10 +2848,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; @@ -3134,6 +3159,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 384fbd0..3684c12 100644 --- a/plume_metal.h +++ b/plume_metal.h @@ -589,6 +589,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;