diff --git a/src/dawn_native/metal/DeviceMTL.h b/src/dawn_native/metal/DeviceMTL.h index 9bcbf2f577..072e605e00 100644 --- a/src/dawn_native/metal/DeviceMTL.h +++ b/src/dawn_native/metal/DeviceMTL.h @@ -24,8 +24,9 @@ #import #import +#include #include -#include +#include namespace dawn_native { namespace metal { @@ -54,6 +55,7 @@ namespace dawn_native { namespace metal { TextureBase* CreateTextureWrappingIOSurface(const TextureDescriptor* descriptor, IOSurfaceRef ioSurface, uint32_t plane); + void WaitForCommandsToBeScheduled(); ResultOrError> CreateStagingBuffer(size_t size) override; MaybeError CopyFromStagingToBuffer(StagingBufferBase* source, @@ -85,15 +87,21 @@ namespace dawn_native { namespace metal { TextureBase* texture, const TextureViewDescriptor* descriptor) override; - void OnCompletedHandler(); - id mMtlDevice = nil; id mCommandQueue = nil; std::unique_ptr mMapTracker; - Serial mCompletedSerial = 0; Serial mLastSubmittedSerial = 0; id mPendingCommands = nil; + + // The completed serial is updated in a Metal completion handler that can be fired on a + // different thread, so it needs to be atomic. + std::atomic mCompletedSerial; + + // mLastSubmittedCommands will be accessed in a Metal schedule handler that can be fired on + // a different thread so we guard access to it with a mutex. + std::mutex mLastSubmittedCommandsMutex; + id mLastSubmittedCommands = nil; }; }} // namespace dawn_native::metal diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm index 3a5fa1a9f1..3b8639206c 100644 --- a/src/dawn_native/metal/DeviceMTL.mm +++ b/src/dawn_native/metal/DeviceMTL.mm @@ -31,12 +31,15 @@ #include "dawn_native/metal/SwapChainMTL.h" #include "dawn_native/metal/TextureMTL.h" +#include + namespace dawn_native { namespace metal { Device::Device(AdapterBase* adapter, id mtlDevice) : DeviceBase(adapter), mMtlDevice([mtlDevice retain]), - mMapTracker(new MapRequestTracker(this)) { + mMapTracker(new MapRequestTracker(this)), + mCompletedSerial(0) { [mMtlDevice retain]; mCommandQueue = [mMtlDevice newCommandQueue]; } @@ -47,7 +50,7 @@ namespace dawn_native { namespace metal { // store the pendingSerial before SubmitPendingCommandBuffer then wait for it to be passed. // Instead we submit and wait for the serial before the next pendingCommandSerial. SubmitPendingCommandBuffer(); - while (mCompletedSerial != mLastSubmittedSerial) { + while (GetCompletedCommandSerial() != mLastSubmittedSerial) { usleep(100); } Tick(); @@ -118,7 +121,8 @@ namespace dawn_native { namespace metal { } Serial Device::GetCompletedCommandSerial() const { - return mCompletedSerial; + static_assert(std::is_same::value, ""); + return mCompletedSerial.load(); } Serial Device::GetLastSubmittedCommandSerial() const { @@ -130,12 +134,14 @@ namespace dawn_native { namespace metal { } void Device::TickImpl() { - mDynamicUploader->Tick(mCompletedSerial); - mMapTracker->Tick(mCompletedSerial); + Serial completedSerial = GetCompletedCommandSerial(); + + mDynamicUploader->Tick(completedSerial); + mMapTracker->Tick(completedSerial); if (mPendingCommands != nil) { SubmitPendingCommandBuffer(); - } else if (mCompletedSerial == mLastSubmittedSerial) { + } else if (completedSerial == mLastSubmittedSerial) { // If there's no GPU work in flight we still need to artificially increment the serial // so that CPU operations waiting on GPU completion can know they don't have to wait. mCompletedSerial++; @@ -160,18 +166,45 @@ namespace dawn_native { namespace metal { return; } - // Ok, ObjC blocks are weird. My understanding is that local variables are captured by value - // so this-> works as expected. However it is unclear how members are captured, (are they - // captured using this-> or by value?) so we make a copy of the pendingCommandSerial on the - // stack. mLastSubmittedSerial++; + + // Replace mLastSubmittedCommands with the mutex held so we avoid races between the + // schedule handler and this code. + { + std::lock_guard lock(mLastSubmittedCommandsMutex); + [mLastSubmittedCommands release]; + mLastSubmittedCommands = mPendingCommands; + } + + // Ok, ObjC blocks are weird. My understanding is that local variables are captured by + // value so this-> works as expected. However it is unclear how members are captured, (are + // they captured using this-> or by value?). To be safe we copy members to local variables + // to ensure they are captured "by value". + + // Free mLastSubmittedCommands as soon as it is scheduled so that it doesn't hold + // references to its resources. Make a local copy of pendingCommands first so it is + // captured "by-value" by the block. + id pendingCommands = mPendingCommands; + + [mPendingCommands addScheduledHandler:^(id) { + // This is DRF because we hold the mutex for mLastSubmittedCommands and pendingCommands + // is a local value (and not the member itself). + std::lock_guard lock(mLastSubmittedCommandsMutex); + if (this->mLastSubmittedCommands == pendingCommands) { + [this->mLastSubmittedCommands release]; + this->mLastSubmittedCommands = nil; + } + }]; + + // Update the completed serial once the completed handler is fired. Make a local copy of + // mLastSubmittedSerial so it is captured by value. Serial pendingSerial = mLastSubmittedSerial; [mPendingCommands addCompletedHandler:^(id) { + ASSERT(pendingSerial > mCompletedSerial.load()); this->mCompletedSerial = pendingSerial; }]; [mPendingCommands commit]; - [mPendingCommands release]; mPendingCommands = nil; } @@ -216,4 +249,10 @@ namespace dawn_native { namespace metal { return new Texture(this, descriptor, ioSurface, plane); } + + void Device::WaitForCommandsToBeScheduled() { + SubmitPendingCommandBuffer(); + [mLastSubmittedCommands waitUntilScheduled]; + } + }} // namespace dawn_native::metal diff --git a/src/dawn_native/metal/MetalBackend.mm b/src/dawn_native/metal/MetalBackend.mm index f7d5398f7b..e5c88673ff 100644 --- a/src/dawn_native/metal/MetalBackend.mm +++ b/src/dawn_native/metal/MetalBackend.mm @@ -38,4 +38,9 @@ namespace dawn_native { namespace metal { return reinterpret_cast(texture); } + void WaitForCommandsToBeScheduled(DawnDevice cDevice) { + Device* device = reinterpret_cast(cDevice); + device->WaitForCommandsToBeScheduled(); + } + }} // namespace dawn_native::metal diff --git a/src/include/dawn_native/MetalBackend.h b/src/include/dawn_native/MetalBackend.h index 1a3001311c..7588b97896 100644 --- a/src/include/dawn_native/MetalBackend.h +++ b/src/include/dawn_native/MetalBackend.h @@ -37,6 +37,13 @@ namespace dawn_native { namespace metal { const DawnTextureDescriptor* descriptor, IOSurfaceRef ioSurface, uint32_t plane); + + // When making Metal interop with other APIs, we need to be careful that QueueSubmit doesn't + // mean that the operations will be visible to other APIs/Metal devices right away. macOS + // does have a global queue of graphics operations, but the command buffers are inserted there + // when they are "scheduled". Submitting other operations before the command buffer is + // scheduled could lead to races in who gets scheduled first and incorrect rendering. + DAWN_NATIVE_EXPORT void WaitForCommandsToBeScheduled(DawnDevice device); }} // namespace dawn_native::metal #ifdef __OBJC__ diff --git a/src/tests/end2end/IOSurfaceWrappingTests.cpp b/src/tests/end2end/IOSurfaceWrappingTests.cpp index 9390cbfd65..c7f1fb7c70 100644 --- a/src/tests/end2end/IOSurfaceWrappingTests.cpp +++ b/src/tests/end2end/IOSurfaceWrappingTests.cpp @@ -345,18 +345,8 @@ class IOSurfaceUsageTests : public IOSurfaceTestBase { dawn::CommandBuffer commands = encoder.Finish(); queue.Submit(1, &commands); - // Use a fence to know that GPU rendering is finished. - // TODO(cwallez@chromium.org): IOSurfaceLock should wait for previous GPU use of the - // IOSurface to be completed but this appears to not be the case. - // Maybe it is because the Metal command buffer has been submitted but not "scheduled" yet? - dawn::FenceDescriptor fenceDescriptor; - fenceDescriptor.initialValue = 0u; - dawn::Fence fence = queue.CreateFence(&fenceDescriptor); - queue.Signal(fence, 1); - - while (fence.GetCompletedValue() < 1) { - WaitABit(); - } + // Wait for the commands touching the IOSurface to be scheduled + dawn_native::metal::WaitForCommandsToBeScheduled(device.Get()); // Check the correct data was written IOSurfaceLock(ioSurface, kIOSurfaceLockReadOnly, nullptr);