Skip to content

Metal fixes for bugs encountered in Unleashed #24

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tanmaysachan wants to merge 2 commits into from
Closed

Metal fixes for bugs encountered in Unleashed #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a08648
Fix race on resource release + fixes for unleashed
tanmaysachan Apr 2, 2025
dc9bf8a
Add nullBuffer and stride=0 support
tanmaysachan Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions plume_metal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -1378,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++) {
Expand Down Expand Up @@ -1518,6 +1522,10 @@ namespace plume {
}

MetalDescriptorSet::~MetalDescriptorSet() {
for (const auto resource: toReleaseOnDestruction) {
resource->release();
}

for (const auto &entry : resourceEntries) {
if (entry.resource != nullptr) {
entry.resource->release();
Expand Down Expand Up @@ -1613,8 +1621,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);
}
}

Expand Down Expand Up @@ -2098,7 +2105,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;
Expand Down Expand Up @@ -2149,7 +2156,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;
Expand Down Expand Up @@ -2211,9 +2218,14 @@ namespace plume {
// Check for changes in bindings
for (uint32_t i = 0; i < viewCount; i++) {
const MetalBuffer* interfaceBuffer = static_cast<const MetalBuffer*>(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<const MetalBuffer*>(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];

Expand Down Expand Up @@ -2836,10 +2848,21 @@ namespace plume {
}

if (dirtyGraphicsState.vertexBuffers) {
std::vector<bool> 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<const MetalBuffer*>(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;
Expand Down Expand Up @@ -3136,6 +3159,8 @@ namespace plume {
capabilities.dynamicDepthBias = true;
capabilities.uma = mtl->hasUnifiedMemory();
capabilities.queryPools = false;

nullBuffer = createBuffer(RenderBufferDesc::DefaultBuffer(16, RenderBufferFlag::VERTEX));
}

MetalDevice::~MetalDevice() {
Expand Down
5 changes: 4 additions & 1 deletion plume_metal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -207,6 +207,7 @@ namespace plume {
std::vector<Descriptor> descriptors;
MetalArgumentBuffer argumentBuffer;
std::vector<ResourceEntry> resourceEntries;
std::vector<MTL::Resource *> toReleaseOnDestruction;

MetalDescriptorSet(MetalDevice *device, const RenderDescriptorSetDesc &desc);
MetalDescriptorSet(MetalDevice *device, uint32_t entryCount);
Expand Down Expand Up @@ -588,6 +589,8 @@ namespace plume {
// Blit functionality
MTL::BlitPassDescriptor *sharedBlitDescriptor = nullptr;

std::unique_ptr<RenderBuffer> nullBuffer;

explicit MetalDevice(MetalInterface *renderInterface, const std::string &preferredDeviceName);
~MetalDevice() override;
std::unique_ptr<RenderDescriptorSet> createDescriptorSet(const RenderDescriptorSetDesc &desc) override;
Expand Down