D3D12's external image: Add multithread support.

Bug: dawn:1662
Change-Id: Id2d720af4e5e654b5f34aebf485bededc6d247ca
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/122740
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Quyen Le <lehoangquyen@chromium.org>

src/dawn/common/Mutex.h

@@ -29,7 +29,7 @@ class Mutex : public RefCounted, NonCopyable {
   public:
     template <typename MutexRef>
     struct AutoLockBase : NonMovable {
-        AutoLockBase() = delete;
+        AutoLockBase() : mMutex(nullptr) {}
         explicit AutoLockBase(MutexRef mutex) : mMutex(std::move(mutex)) {
             if (mMutex != nullptr) {
                 mMutex->Lock();

src/dawn/native/d3d12/D3D12Backend.cpp

@@ -56,19 +56,11 @@ WGPUTexture ExternalImageDXGI::ProduceTexture(
 
 WGPUTexture ExternalImageDXGI::BeginAccess(
     const ExternalImageDXGIBeginAccessDescriptor* descriptor) {
-    if (!IsValid()) {
-        dawn::ErrorLog() << "Cannot use external image after device destruction";
-        return nullptr;
-    }
     return mImpl->BeginAccess(descriptor);
 }
 
 void ExternalImageDXGI::EndAccess(WGPUTexture texture,
                                   ExternalImageDXGIFenceDescriptor* signalFence) {
-    if (!IsValid()) {
-        dawn::ErrorLog() << "Cannot use external image after device destruction";
-        return;
-    }
     mImpl->EndAccess(texture, signalFence);
 }
 
@@ -77,6 +69,7 @@ std::unique_ptr<ExternalImageDXGI> ExternalImageDXGI::Create(
     WGPUDevice device,
     const ExternalImageDescriptorDXGISharedHandle* descriptor) {
     Device* backendDevice = ToBackend(FromAPI(device));
+    auto deviceLock(backendDevice->GetScopedLock());
     std::unique_ptr<ExternalImageDXGIImpl> impl =
         backendDevice->CreateExternalImageDXGIImpl(descriptor);
     if (!impl) {
@@ -91,6 +84,8 @@ uint64_t SetExternalMemoryReservation(WGPUDevice device,
                                       MemorySegment memorySegment) {
     Device* backendDevice = ToBackend(FromAPI(device));
 
+    auto deviceLock(backendDevice->GetScopedLock());
+
     return backendDevice->GetResidencyManager()->SetExternalMemoryReservation(
         memorySegment, requestedReservationSize);
 }

src/dawn/native/d3d12/DeviceD3D12.cpp

@@ -396,6 +396,8 @@ void Device::ForceEventualFlushOfCommands() {
 }
 
 MaybeError Device::ExecutePendingCommandContext() {
+    ASSERT(IsLockedByCurrentThreadIfNeeded());
+
     return mPendingCommands.ExecuteCommandList(this);
 }
 
@@ -729,8 +731,8 @@ void Device::DestroyImpl() {
 
     while (!mExternalImageList.empty()) {
         ExternalImageDXGIImpl* externalImage = mExternalImageList.head()->value();
-        // ExternalImageDXGIImpl::Destroy() calls RemoveFromList().
-        externalImage->Destroy();
+        // ExternalImageDXGIImpl::DestroyInternal() calls RemoveFromList().
+        externalImage->DestroyInternal();
     }
 
     mZeroBuffer = nullptr;

src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp

@@ -53,32 +53,56 @@ ExternalImageDXGIImpl::ExternalImageDXGIImpl(Device* backendDevice,
 }
 
 ExternalImageDXGIImpl::~ExternalImageDXGIImpl() {
-    Destroy();
+    auto deviceLock(GetScopedDeviceLock());
+    DestroyInternal();
+}
+
+Mutex::AutoLock ExternalImageDXGIImpl::GetScopedDeviceLock() const {
+    if (mBackendDevice != nullptr) {
+        return mBackendDevice->GetScopedLock();
+    }
+    return Mutex::AutoLock();
 }
 
 bool ExternalImageDXGIImpl::IsValid() const {
+    auto deviceLock(GetScopedDeviceLock());
+
     return IsInList();
 }
 
-void ExternalImageDXGIImpl::Destroy() {
+void ExternalImageDXGIImpl::DestroyInternal() {
+    // Linked list is not thread safe. A mutex must already be locked before
+    // endtering this method. Either via Device::DestroyImpl() or ~ExternalImageDXGIImpl.
+    ASSERT(mBackendDevice == nullptr || mBackendDevice->IsLockedByCurrentThreadIfNeeded());
     if (IsInList()) {
         RemoveFromList();
-        mBackendDevice = nullptr;
         mD3D12Resource = nullptr;
         mD3D11on12ResourceCache = nullptr;
+
+        // We don't set mBackendDevice to null here to let its mutex accessible until dtor. This
+        // also to avoid data racing with non-null check in GetScopedDeviceLock()
     }
 }
 
 WGPUTexture ExternalImageDXGIImpl::BeginAccess(
     const ExternalImageDXGIBeginAccessDescriptor* descriptor) {
-    ASSERT(mBackendDevice != nullptr);
     ASSERT(descriptor != nullptr);
+
+    auto deviceLock(GetScopedDeviceLock());
+
+    if (!IsInList()) {
+        dawn::ErrorLog() << "Cannot use external image after device destruction";
+        return nullptr;
+    }
+
     // Ensure the texture usage is allowed
     if (!IsSubset(descriptor->usage, static_cast<WGPUTextureUsageFlags>(mUsage))) {
         dawn::ErrorLog() << "Texture usage is not valid for external image";
         return nullptr;
     }
 
+    ASSERT(mBackendDevice != nullptr);
+
     TextureDescriptor textureDescriptor = {};
     textureDescriptor.usage = static_cast<wgpu::TextureUsage>(descriptor->usage);
     textureDescriptor.dimension = mDimension;
@@ -130,6 +154,14 @@ WGPUTexture ExternalImageDXGIImpl::BeginAccess(
 
 void ExternalImageDXGIImpl::EndAccess(WGPUTexture texture,
                                       ExternalImageDXGIFenceDescriptor* signalFence) {
+    auto deviceLock(GetScopedDeviceLock());
+
+    if (!IsInList()) {
+        dawn::ErrorLog() << "Cannot use external image after device destruction";
+        return;
+    }
+
+    ASSERT(mBackendDevice != nullptr);
     ASSERT(signalFence != nullptr);
 
     Texture* backendTexture = ToBackend(FromAPI(texture));

src/dawn/native/d3d12/ExternalImageDXGIImpl.h

@@ -21,6 +21,7 @@
 #include <vector>
 
 #include "dawn/common/LinkedList.h"
+#include "dawn/common/Mutex.h"
 #include "dawn/native/D3D12Backend.h"
 #include "dawn/native/Error.h"
 #include "dawn/native/Forward.h"
@@ -49,15 +50,19 @@ class ExternalImageDXGIImpl : public LinkNode<ExternalImageDXGIImpl> {
     ExternalImageDXGIImpl(const ExternalImageDXGIImpl&) = delete;
     ExternalImageDXGIImpl& operator=(const ExternalImageDXGIImpl&) = delete;
 
-    void Destroy();
-
     bool IsValid() const;
 
     WGPUTexture BeginAccess(const ExternalImageDXGIBeginAccessDescriptor* descriptor);
 
     void EndAccess(WGPUTexture texture, ExternalImageDXGIFenceDescriptor* signalFence);
 
+    // This method should only be called by internal code. Don't call this from D3D12Backend side,
+    // or without locking.
+    void DestroyInternal();
+
   private:
+    [[nodiscard]] Mutex::AutoLock GetScopedDeviceLock() const;
+
     Ref<Device> mBackendDevice;
     Microsoft::WRL::ComPtr<ID3D12Resource> mD3D12Resource;
     const bool mUseFenceSynchronization;

src/dawn/tests/end2end/D3D12ResourceWrappingTests.cpp

@@ -19,6 +19,7 @@
 #include <wrl/client.h>
 
 #include <memory>
+#include <thread>
 #include <utility>
 #include <vector>
 
@@ -1058,9 +1059,76 @@ TEST_P(D3D12SharedHandleUsageTests, SRGBReinterpretation) {
         utils::RGBA8(247, 125, 105, 255), texture, 0, 0);
 }
 
+class D3D12SharedHandleMultithreadTests : public D3D12SharedHandleUsageTests {
+  protected:
+    std::vector<wgpu::FeatureName> GetRequiredFeatures() override {
+        std::vector<wgpu::FeatureName> features;
+        // TODO(crbug.com/dawn/1678): DawnWire doesn't support thread safe API yet.
+        if (!UsesWire()) {
+            features.push_back(wgpu::FeatureName::ImplicitDeviceSynchronization);
+        }
+        return features;
+    }
+
+    void SetUp() override {
+        D3D12SharedHandleUsageTests::SetUp();
+        // TODO(crbug.com/dawn/1678): DawnWire doesn't support thread safe API yet.
+        DAWN_TEST_UNSUPPORTED_IF(UsesWire());
+    }
+};
+
+// Test the destroy device before destroying the external image won't cause deadlock.
+TEST_P(D3D12SharedHandleMultithreadTests, DestroyDeviceBeforeImageNoDeadLock) {
+    wgpu::Texture texture;
+    ComPtr<ID3D11Texture2D> d3d11Texture;
+    std::unique_ptr<dawn::native::d3d12::ExternalImageDXGI> externalImage;
+    WrapSharedHandle(&baseDawnDescriptor, &baseD3dDescriptor, &texture, &d3d11Texture,
+                     &externalImage);
+
+    ASSERT_NE(texture.Get(), nullptr);
+
+    dawn::native::d3d12::ExternalImageDXGIFenceDescriptor signalFence;
+    externalImage->EndAccess(texture.Get(), &signalFence);
+    EXPECT_TRUE(externalImage->IsValid());
+
+    // Destroy device, it should destroy image internally.
+    device.Destroy();
+    EXPECT_FALSE(externalImage->IsValid());
+}
+
+// Test that using the external image and destroying the device simultaneously on different threads
+// won't race.
+TEST_P(D3D12SharedHandleMultithreadTests, DestroyDeviceAndUseImageInParallel) {
+    wgpu::Texture texture;
+    ComPtr<ID3D11Texture2D> d3d11Texture;
+    std::unique_ptr<dawn::native::d3d12::ExternalImageDXGI> externalImage;
+    WrapSharedHandle(&baseDawnDescriptor, &baseD3dDescriptor, &texture, &d3d11Texture,
+                     &externalImage);
+
+    ASSERT_NE(texture.Get(), nullptr);
+    EXPECT_TRUE(externalImage->IsValid());
+
+    std::thread thread1([&] {
+        dawn::native::d3d12::ExternalImageDXGIFenceDescriptor signalFence;
+        externalImage->EndAccess(texture.Get(), &signalFence);
+    });
+
+    std::thread thread2([&] {
+        // Destroy device, it should destroy image internally.
+        device.Destroy();
+        EXPECT_FALSE(externalImage->IsValid());
+    });
+
+    thread1.join();
+    thread2.join();
+}
+
 DAWN_INSTANTIATE_TEST_P(D3D12SharedHandleValidation,
                         {D3D12Backend()},
                         {SyncMode::kKeyedMutex, SyncMode::kFence});
 DAWN_INSTANTIATE_TEST_P(D3D12SharedHandleUsageTests,
                         {D3D12Backend()},
                         {SyncMode::kKeyedMutex, SyncMode::kFence});
+DAWN_INSTANTIATE_TEST_P(D3D12SharedHandleMultithreadTests,
+                        {D3D12Backend()},
+                        {SyncMode::kKeyedMutex, SyncMode::kFence});