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>
diff --git a/src/dawn_native/d3d12/CommandRecordingContext.cpp b/src/dawn_native/d3d12/CommandRecordingContext.cpp
index ce7c22a..ad20764 100644
--- a/src/dawn_native/d3d12/CommandRecordingContext.cpp
+++ b/src/dawn_native/d3d12/CommandRecordingContext.cpp
@@ -61,6 +61,7 @@
// 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 @@
ID3D12CommandList* d3d12CommandList = GetCommandList();
device->GetCommandQueue()->ExecuteCommandLists(1, &d3d12CommandList);
+ for (Texture* texture : mSharedTextures) {
+ texture->ReleaseKeyedMutex();
+ }
+
mIsOpen = false;
mSharedTextures.clear();
mHeapsPendingUsage.clear();
diff --git a/src/dawn_native/d3d12/D3D11on12Util.cpp b/src/dawn_native/d3d12/D3D11on12Util.cpp
index 0c526a0..55b2393 100644
--- a/src/dawn_native/d3d12/D3D11on12Util.cpp
+++ b/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 @@
return;
}
+ if (mAcquireCount > 0) {
+ mDXGIKeyedMutex->ReleaseSync(kDXGIKeyedMutexAcquireReleaseKey);
+ }
+
ComPtr<ID3D11Resource> d3d11Resource;
if (FAILED(mDXGIKeyedMutex.As(&d3d11Resource))) {
return;
@@ -85,9 +92,25 @@
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 @@
return entry;
}
-} // namespace dawn_native::d3d12
\ No newline at end of file
+} // namespace dawn_native::d3d12
diff --git a/src/dawn_native/d3d12/D3D11on12Util.h b/src/dawn_native/d3d12/D3D11on12Util.h
index ab622f0..9abf7c6 100644
--- a/src/dawn_native/d3d12/D3D11on12Util.h
+++ b/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 @@
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 @@
private:
ComPtr<IDXGIKeyedMutex> mDXGIKeyedMutex;
ComPtr<ID3D11On12Device> mD3D11on12Device;
+ int64_t mAcquireCount = 0;
};
// |D3D11on12ResourceCache| maintains a cache of 11 wrapped resources.
diff --git a/src/dawn_native/d3d12/TextureD3D12.cpp b/src/dawn_native/d3d12/TextureD3D12.cpp
index bf07cad..623ac02 100644
--- a/src/dawn_native/d3d12/TextureD3D12.cpp
+++ b/src/dawn_native/d3d12/TextureD3D12.cpp
@@ -552,10 +552,6 @@
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 @@
// 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 @@
}
}
+ 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) {
diff --git a/src/dawn_native/d3d12/TextureD3D12.h b/src/dawn_native/d3d12/TextureD3D12.h
index fc08940..9bc6ced 100644
--- a/src/dawn_native/d3d12/TextureD3D12.h
+++ b/src/dawn_native/d3d12/TextureD3D12.h
@@ -64,9 +64,13 @@
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);
diff --git a/src/tests/end2end/D3D12ResourceWrappingTests.cpp b/src/tests/end2end/D3D12ResourceWrappingTests.cpp
index f12e98c..81a6b52 100644
--- a/src/tests/end2end/D3D12ResourceWrappingTests.cpp
+++ b/src/tests/end2end/D3D12ResourceWrappingTests.cpp
@@ -624,6 +624,51 @@
}
}
+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());
