d3d12: Acquire/release keyed mutex around ExecuteCommandLists

D3D keyed mutex cannot be acquired recursively and therefore external
D3D textures cannot be concurrently read within Dawn (e.g. multiple
imports of the same video frame) or across Dawn and GL (e.g. both WebGPU
and WebGL importing a video frame).

Within Dawn, we can scope keyed mutex acquire/release to command list
submission so that Chromium can always guarantee that Dawn doesn't hold
access to any resources after running Dawn wire commands. We also check
that keyed mutexes aren't recursively acquired in Dawn by keeping track
of the acquire count per resource.

This solves the multiple acquire problem within Dawn and provides a path
for concurrent read access across Dawn and GL once the GL decoders are
changed so that they also don't hold access to resources after switching
contexts.

Bug: chromium:1241533
Change-Id: If88fd4a4f798b972836a134809e4fed8832ec89c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/75644
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Auto-Submit: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn_native/d3d12/CommandRecordingContext.cpp

@@ -61,6 +61,7 @@ namespace dawn_native::d3d12 {
             // common state right before command list submission. TransitionUsageNow itself ensures
             // no unnecessary transitions happen if the resources is already in the common state.
             for (Texture* texture : mSharedTextures) {
+                DAWN_TRY(texture->AcquireKeyedMutex());
                 texture->TrackAllUsageAndTransitionNow(this, D3D12_RESOURCE_STATE_COMMON);
             }
 
@@ -76,6 +77,10 @@ namespace dawn_native::d3d12 {
             ID3D12CommandList* d3d12CommandList = GetCommandList();
             device->GetCommandQueue()->ExecuteCommandLists(1, &d3d12CommandList);
 
+            for (Texture* texture : mSharedTextures) {
+                texture->ReleaseKeyedMutex();
+            }
+
             mIsOpen = false;
             mSharedTextures.clear();
             mHeapsPendingUsage.clear();

src/dawn_native/d3d12/D3D11on12Util.cpp

@@ -19,8 +19,11 @@
 
 #include "common/HashUtils.h"
 #include "common/Log.h"
+#include "dawn_native/d3d12/D3D12Error.h"
 #include "dawn_native/d3d12/DeviceD3D12.h"
 
+#include <dawn_native/D3D12Backend.h>
+
 namespace dawn_native::d3d12 {
 
     void Flush11On12DeviceToAvoidLeaks(ComPtr<ID3D11On12Device> d3d11on12Device) {
@@ -69,6 +72,10 @@ namespace dawn_native::d3d12 {
             return;
         }
 
+        if (mAcquireCount > 0) {
+            mDXGIKeyedMutex->ReleaseSync(kDXGIKeyedMutexAcquireReleaseKey);
+        }
+
         ComPtr<ID3D11Resource> d3d11Resource;
         if (FAILED(mDXGIKeyedMutex.As(&d3d11Resource))) {
             return;
@@ -85,9 +92,25 @@ namespace dawn_native::d3d12 {
         Flush11On12DeviceToAvoidLeaks(std::move(mD3D11on12Device));
     }
 
-    ComPtr<IDXGIKeyedMutex> D3D11on12ResourceCacheEntry::GetDXGIKeyedMutex() const {
+    MaybeError D3D11on12ResourceCacheEntry::AcquireKeyedMutex() {
         ASSERT(mDXGIKeyedMutex != nullptr);
-        return mDXGIKeyedMutex;
+        ASSERT(mAcquireCount >= 0);
+        if (mAcquireCount == 0) {
+            DAWN_TRY(CheckHRESULT(
+                mDXGIKeyedMutex->AcquireSync(kDXGIKeyedMutexAcquireReleaseKey, INFINITE),
+                "D3D12 acquiring shared mutex"));
+        }
+        mAcquireCount++;
+        return {};
+    }
+
+    void D3D11on12ResourceCacheEntry::ReleaseKeyedMutex() {
+        ASSERT(mDXGIKeyedMutex != nullptr);
+        ASSERT(mAcquireCount > 0);
+        mAcquireCount--;
+        if (mAcquireCount == 0) {
+            mDXGIKeyedMutex->ReleaseSync(kDXGIKeyedMutexAcquireReleaseKey);
+        }
     }
 
     size_t D3D11on12ResourceCacheEntry::HashFunc::operator()(
@@ -161,4 +184,4 @@ namespace dawn_native::d3d12 {
         return entry;
     }
 
-}  // namespace dawn_native::d3d12
+}  // namespace dawn_native::d3d12

src/dawn_native/d3d12/D3D11on12Util.h

@@ -16,6 +16,7 @@
 #define DAWNNATIVE_D3D11ON12UTIL_H_
 
 #include "common/RefCounted.h"
+#include "dawn_native/Error.h"
 #include "dawn_native/d3d12/d3d12_platform.h"
 
 #include <dawn_native/DawnNative.h>
@@ -35,7 +36,8 @@ namespace dawn_native::d3d12 {
                                     ComPtr<ID3D11On12Device> d3d11on12Device);
         ~D3D11on12ResourceCacheEntry();
 
-        ComPtr<IDXGIKeyedMutex> GetDXGIKeyedMutex() const;
+        MaybeError AcquireKeyedMutex();
+        void ReleaseKeyedMutex();
 
         // Functors necessary for the
         // unordered_set<D3D11on12ResourceCacheEntry&>-based cache.
@@ -51,6 +53,7 @@ namespace dawn_native::d3d12 {
       private:
         ComPtr<IDXGIKeyedMutex> mDXGIKeyedMutex;
         ComPtr<ID3D11On12Device> mD3D11on12Device;
+        int64_t mAcquireCount = 0;
     };
 
     // |D3D11on12ResourceCache| maintains a cache of 11 wrapped resources.

src/dawn_native/d3d12/TextureD3D12.cpp

@@ -552,10 +552,6 @@ namespace dawn_native::d3d12 {
         ComPtr<ID3D12Resource> d3d12Texture,
         Ref<D3D11on12ResourceCacheEntry> d3d11on12Resource,
         bool isSwapChainTexture) {
-        DAWN_TRY(CheckHRESULT(d3d11on12Resource->GetDXGIKeyedMutex()->AcquireSync(
-                                  kDXGIKeyedMutexAcquireReleaseKey, INFINITE),
-                              "D3D12 acquiring shared mutex"));
-
         mD3D11on12Resource = std::move(d3d11on12Resource);
         mSwapChainTexture = isSwapChainTexture;
 
@@ -672,10 +668,6 @@ namespace dawn_native::d3d12 {
         // We can set mSwapChainTexture to false to avoid passing a nullptr to
         // ID3D12SharingContract::Present.
         mSwapChainTexture = false;
-
-        if (mD3D11on12Resource != nullptr) {
-            mD3D11on12Resource->GetDXGIKeyedMutex()->ReleaseSync(kDXGIKeyedMutexAcquireReleaseKey);
-        }
     }
 
     DXGI_FORMAT Texture::GetD3D12Format() const {
@@ -707,6 +699,16 @@ namespace dawn_native::d3d12 {
         }
     }
 
+    MaybeError Texture::AcquireKeyedMutex() {
+        ASSERT(mD3D11on12Resource != nullptr);
+        return mD3D11on12Resource->AcquireKeyedMutex();
+    }
+
+    void Texture::ReleaseKeyedMutex() {
+        ASSERT(mD3D11on12Resource != nullptr);
+        mD3D11on12Resource->ReleaseKeyedMutex();
+    }
+
     void Texture::TrackUsageAndTransitionNow(CommandRecordingContext* commandContext,
                                              wgpu::TextureUsage usage,
                                              const SubresourceRange& range) {

src/dawn_native/d3d12/TextureD3D12.h

@@ -64,9 +64,13 @@ namespace dawn_native::d3d12 {
                                                        Aspect aspects,
                                                        bool depthReadOnly,
                                                        bool stencilReadOnly) const;
+
         void EnsureSubresourceContentInitialized(CommandRecordingContext* commandContext,
                                                  const SubresourceRange& range);
 
+        MaybeError AcquireKeyedMutex();
+        void ReleaseKeyedMutex();
+
         void TrackUsageAndGetResourceBarrierForPass(CommandRecordingContext* commandContext,
                                                     std::vector<D3D12_RESOURCE_BARRIER>* barrier,
                                                     const TextureSubresourceUsage& textureUsages);

src/tests/end2end/D3D12ResourceWrappingTests.cpp

@@ -624,6 +624,51 @@ TEST_P(D3D12SharedHandleUsageTests, ReuseExternalImage) {
     }
 }
 
+TEST_P(D3D12SharedHandleUsageTests, RecursiveExternalImageAccess) {
+    DAWN_TEST_UNSUPPORTED_IF(UsesWire());
+
+    // Create the first Dawn texture then clear it to red.
+    wgpu::Texture texture1;
+    ComPtr<ID3D11Texture2D> d3d11Texture;
+    std::unique_ptr<dawn_native::d3d12::ExternalImageDXGI> externalImage;
+    WrapSharedHandle(&baseDawnDescriptor, &baseD3dDescriptor, &texture1, &d3d11Texture,
+                     &externalImage);
+    {
+        const wgpu::Color solidRed{1.0f, 0.0f, 0.0f, 1.0f};
+        ASSERT_NE(texture1.Get(), nullptr);
+        ClearImage(texture1.Get(), solidRed, device);
+
+        EXPECT_PIXEL_RGBA8_EQ(RGBA8(0xFF, 0, 0, 0xFF), texture1.Get(), 0, 0);
+    }
+
+    // Create another Dawn texture then clear it with another color.
+    dawn_native::d3d12::ExternalImageAccessDescriptorDXGIKeyedMutex externalAccessDesc;
+    externalAccessDesc.isInitialized = true;
+    externalAccessDesc.usage = static_cast<WGPUTextureUsageFlags>(baseDawnDescriptor.usage);
+
+    // Acquire the ExternalImageDXGI again without destroying the original texture.
+    wgpu::Texture texture2 =
+        wgpu::Texture::Acquire(externalImage->ProduceTexture(device.Get(), &externalAccessDesc));
+
+    // Check again that the new texture is still red
+    EXPECT_PIXEL_RGBA8_EQ(RGBA8(0xFF, 0, 0, 0xFF), texture2.Get(), 0, 0);
+
+    // Clear the new texture to blue
+    {
+        const wgpu::Color solidBlue{0.0f, 0.0f, 1.0f, 1.0f};
+        ASSERT_NE(texture2.Get(), nullptr);
+        ClearImage(texture2.Get(), solidBlue, device);
+
+        EXPECT_PIXEL_RGBA8_EQ(RGBA8(0, 0, 0xFF, 0xFF), texture2.Get(), 0, 0);
+    }
+
+    // Check that the original texture is also blue
+    EXPECT_PIXEL_RGBA8_EQ(RGBA8(0, 0, 0xFF, 0xFF), texture1.Get(), 0, 0);
+
+    texture1.Destroy();
+    texture2.Destroy();
+}
+
 // Produce a new texture with a usage not specified in the external image.
 TEST_P(D3D12SharedHandleUsageTests, ExternalImageUsage) {
     DAWN_TEST_UNSUPPORTED_IF(UsesWire());