[dawn][metal] Tighten mutex for argument buffer encoding The MTLArgumentEncoder for a BindGroupLayout needs a mutex so that CreateBindGroup can be called on multiple threads against the same BGL at the same time. Bug: 363031535 Change-Id: I0771d8a2dbc4ffef7f42a0756a02bcec81f256f6 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/353235 Commit-Queue: Kai Ninomiya <kainino@chromium.org> Reviewed-by: Loko Kung <lokokung@google.com>
diff --git a/src/dawn/native/metal/BindGroupLayoutMTL.h b/src/dawn/native/metal/BindGroupLayoutMTL.h index 9e0a56e..40edb91 100644 --- a/src/dawn/native/metal/BindGroupLayoutMTL.h +++ b/src/dawn/native/metal/BindGroupLayoutMTL.h
@@ -56,7 +56,7 @@ void DeallocateBindGroup(BindGroup* bindGroup); void ReduceMemoryUsage() override; - NSPRef<id<MTLArgumentEncoder>> GetArgumentEncoder() const; + MutexProtected<NSPRef<id<MTLArgumentEncoder>>>& GetArgumentEncoder(); void FillTintArgumentBufferInfo(tint::msl::writer::ArgumentBufferInfo& info) const; private: @@ -65,7 +65,8 @@ MutexProtected<SlabAllocator<BindGroup>> mBindGroupAllocator; - NSPRef<id<MTLArgumentEncoder>> mArgumentEncoder; + // MTLArgumentEncoder is stateful, mutex prevents multiple CreateBindGroups from conflicting. + MutexProtected<NSPRef<id<MTLArgumentEncoder>>> mArgumentEncoder; }; } // namespace dawn::native::metal
diff --git a/src/dawn/native/metal/BindGroupLayoutMTL.mm b/src/dawn/native/metal/BindGroupLayoutMTL.mm index 208c0d0..5b3738b 100644 --- a/src/dawn/native/metal/BindGroupLayoutMTL.mm +++ b/src/dawn/native/metal/BindGroupLayoutMTL.mm
@@ -97,7 +97,7 @@ if (!descriptors.empty()) { NSArray* ary = [NSArray arrayWithObjects:descriptors.data() count:descriptors.size()]; - mArgumentEncoder = AcquireNSPRef( + mArgumentEncoder->Acquire( [ToBackend(device)->GetMTLDevice() newArgumentEncoderWithArguments:ary]); } } @@ -119,7 +119,7 @@ mBindGroupAllocator->DeleteEmptySlabs(); } -NSPRef<id<MTLArgumentEncoder>> BindGroupLayout::GetArgumentEncoder() const { +MutexProtected<NSPRef<id<MTLArgumentEncoder>>>& BindGroupLayout::GetArgumentEncoder() { return mArgumentEncoder; }
diff --git a/src/dawn/native/metal/BindGroupMTL.mm b/src/dawn/native/metal/BindGroupMTL.mm index 3d98430..9bce0ab 100644 --- a/src/dawn/native/metal/BindGroupMTL.mm +++ b/src/dawn/native/metal/BindGroupMTL.mm
@@ -59,64 +59,65 @@ return {}; } - // TODO(crbug.com/477311786): The argument buffers should probably work in some kind of pool - // instead of being allocated here - auto layout = ToBackend(GetLayout()); + layout->GetArgumentEncoder().Use([this, device, layout](auto encoder) { + NSUInteger argumentBufferLength = [**encoder encodedLength]; + // Avoid zero-sized buffers by rounding up the size. + if (argumentBufferLength == 0) { + argumentBufferLength = 8; + } - auto encoder = layout->GetArgumentEncoder(); - NSUInteger argumentBufferLength = [*encoder encodedLength]; - // Avoid zero-sized buffers by rounding up the size. - if (argumentBufferLength == 0) { - argumentBufferLength = 8; - } + // TODO(crbug.com/477311786): The argument buffers should probably work in some kind of pool + // instead of being allocated here + mArgumentBuffer = + AcquireNSPRef([device->GetMTLDevice() newBufferWithLength:argumentBufferLength + options:0]); + [**encoder setArgumentBuffer:*mArgumentBuffer offset:0]; - mArgumentBuffer = AcquireNSPRef([device->GetMTLDevice() newBufferWithLength:argumentBufferLength - options:0]); - [*encoder setArgumentBuffer:*mArgumentBuffer offset:0]; + for (BindingIndex bindingIndex : Range(layout->GetBindingCount())) { + const BindingInfo& bindingInfo = layout->GetBindingInfo(bindingIndex); + uint32_t dstBinding = ToMTLArgumentBufferIndex(bindingIndex - bindingInfo.indexInArray); - for (BindingIndex bindingIndex : Range(layout->GetBindingCount())) { - const BindingInfo& bindingInfo = layout->GetBindingInfo(bindingIndex); - uint32_t dstBinding = ToMTLArgumentBufferIndex(bindingIndex - bindingInfo.indexInArray); + auto HandleTextureBinding = [&]() { + auto textureView = ToBackend(GetBindingAsTextureView(bindingIndex)); + id<MTLTexture> texture = + textureView->GetTexture()->IsDestroyed() ? nil : textureView->GetMTLTexture(); + [**encoder setTexture:texture atIndex:dstBinding]; + }; - auto HandleTextureBinding = [&]() { - auto textureView = ToBackend(GetBindingAsTextureView(bindingIndex)); - id<MTLTexture> texture = - textureView->GetTexture()->IsDestroyed() ? nil : textureView->GetMTLTexture(); - [*encoder setTexture:texture atIndex:dstBinding]; - }; - - // Note, if a resource is destroyed, we will write nil to that slot. - // Validation should ensure we never actually try to use it. - MatchVariant( - bindingInfo.bindingLayout, - [&](const BufferBindingInfo& layout) { - const BufferBinding& binding = GetBindingAsBufferBinding(bindingIndex); - const id<MTLBuffer> buffer = - binding.buffer->IsDestroyed() ? nil : ToBackend(binding.buffer)->GetMTLBuffer(); - [*encoder setBuffer:buffer offset:binding.offset atIndex:dstBinding]; - }, - [&](const SamplerBindingInfo&) { - auto sampler = ToBackend(GetBindingAsSampler(bindingIndex)); - id<MTLSamplerState> samplerState = sampler->GetMTLSamplerState(); - [*encoder setSamplerState:samplerState atIndex:dstBinding]; - }, - [&](const StaticSamplerBindingInfo&) { - // Static samplers are handled in the frontend. - // TODO(crbug.com/dawn/2482): Implement static samplers in the - // Metal backend. - DAWN_UNREACHABLE(); - }, - [&](const TextureBindingInfo&) { HandleTextureBinding(); }, - [&](const StorageTextureBindingInfo&) { HandleTextureBinding(); }, - [&](const TexelBufferBindingInfo&) { - // Metal does not support texel buffers. - // TODO(crbug/382544164): Prototype texel buffer feature - DAWN_UNREACHABLE(); - }, - [](const InputAttachmentBindingInfo&) { DAWN_UNREACHABLE(); }, - [](const ExternalTextureBindingInfo&) { DAWN_UNREACHABLE(); }); - } + // Note, if a resource is destroyed, we will write nil to that slot. + // Validation should ensure we never actually try to use it. + MatchVariant( + bindingInfo.bindingLayout, + [&](const BufferBindingInfo& layout) { + const BufferBinding& binding = GetBindingAsBufferBinding(bindingIndex); + const id<MTLBuffer> buffer = binding.buffer->IsDestroyed() + ? nil + : ToBackend(binding.buffer)->GetMTLBuffer(); + [**encoder setBuffer:buffer offset:binding.offset atIndex:dstBinding]; + }, + [&](const SamplerBindingInfo&) { + auto sampler = ToBackend(GetBindingAsSampler(bindingIndex)); + id<MTLSamplerState> samplerState = sampler->GetMTLSamplerState(); + [**encoder setSamplerState:samplerState atIndex:dstBinding]; + }, + [&](const StaticSamplerBindingInfo&) { + // Static samplers are handled in the frontend. + // TODO(crbug.com/dawn/2482): Implement static samplers in the + // Metal backend. + DAWN_UNREACHABLE(); + }, + [&](const TextureBindingInfo&) { HandleTextureBinding(); }, + [&](const StorageTextureBindingInfo&) { HandleTextureBinding(); }, + [&](const TexelBufferBindingInfo&) { + // Metal does not support texel buffers. + // TODO(crbug/382544164): Prototype texel buffer feature + DAWN_UNREACHABLE(); + }, + [](const InputAttachmentBindingInfo&) { DAWN_UNREACHABLE(); }, + [](const ExternalTextureBindingInfo&) { DAWN_UNREACHABLE(); }); + } + }); return {}; }
diff --git a/src/dawn/native/metal/DeviceMTL.mm b/src/dawn/native/metal/DeviceMTL.mm index 0385087..f1197fc 100644 --- a/src/dawn/native/metal/DeviceMTL.mm +++ b/src/dawn/native/metal/DeviceMTL.mm
@@ -413,13 +413,6 @@ } std::optional<DeviceGuard> Device::UseGuardForCreateBindGroup() { - // TODO(crbug.com/363031535): When argument buffers are enabled, BindGroup::InitializeImpl() - // encodes into a shared MTLArgumentEncoder stored on the BindGroupLayout without - // synchronization. We can remove this guard if we synchronize the shared encoder's state in the - // future. - if (IsToggleEnabled(Toggle::MetalUseArgumentBuffers)) { - return GetGuard(); - } return std::nullopt; }