Metal: Expose function to wait for commands to be scheduled.

This is to allow proper synchronization with other devices and APIs on
macOS. There is a global graphics queue so we usually don't need
synchronization but on Metal, commands are inserted on this queue only
when the command buffer is scheduled.

Metal's schedule and completed handlers can be fired on a different
thread so this CL also makes the code there data-race free.

BUG=chromium:938895
BUG=dawn:112

Change-Id: Id45a66fb4d13216b9d01f75e0766732f6e09ddf0
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/5700
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/metal/DeviceMTL.h

@@ -24,8 +24,9 @@
 #import <Metal/Metal.h>
 #import <QuartzCore/CAMetalLayer.h>
 
+#include <atomic>
 #include <memory>
-#include <type_traits>
+#include <mutex>
 
 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<std::unique_ptr<StagingBufferBase>> 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<MTLDevice> mMtlDevice = nil;
         id<MTLCommandQueue> mCommandQueue = nil;
         std::unique_ptr<MapRequestTracker> mMapTracker;
 
-        Serial mCompletedSerial = 0;
         Serial mLastSubmittedSerial = 0;
         id<MTLCommandBuffer> 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<uint64_t> 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<MTLCommandBuffer> mLastSubmittedCommands = nil;
     };
 
 }}  // namespace dawn_native::metal

src/dawn_native/metal/DeviceMTL.mm

@@ -31,12 +31,15 @@
 #include "dawn_native/metal/SwapChainMTL.h"
 #include "dawn_native/metal/TextureMTL.h"
 
+#include <type_traits>
+
 namespace dawn_native { namespace metal {
 
     Device::Device(AdapterBase* adapter, id<MTLDevice> 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<Serial, uint64_t>::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<std::mutex> 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<MTLCommandBuffer> pendingCommands = mPendingCommands;
+
+        [mPendingCommands addScheduledHandler:^(id<MTLCommandBuffer>) {
+            // This is DRF because we hold the mutex for mLastSubmittedCommands and pendingCommands
+            // is a local value (and not the member itself).
+            std::lock_guard<std::mutex> 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<MTLCommandBuffer>) {
+            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

src/dawn_native/metal/MetalBackend.mm

@@ -38,4 +38,9 @@ namespace dawn_native { namespace metal {
         return reinterpret_cast<DawnTexture>(texture);
     }
 
+    void WaitForCommandsToBeScheduled(DawnDevice cDevice) {
+        Device* device = reinterpret_cast<Device*>(cDevice);
+        device->WaitForCommandsToBeScheduled();
+    }
+
 }}  // namespace dawn_native::metal

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__

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);