diff --git a/src/dawn/common/Mutex.h b/src/dawn/common/Mutex.h index c5a3a8cbf7..d728013d0e 100644 --- a/src/dawn/common/Mutex.h +++ b/src/dawn/common/Mutex.h @@ -29,7 +29,7 @@ class Mutex : public RefCounted, NonCopyable { public: template struct AutoLockBase : NonMovable { - AutoLockBase() = delete; + AutoLockBase() : mMutex(nullptr) {} explicit AutoLockBase(MutexRef mutex) : mMutex(std::move(mutex)) { if (mMutex != nullptr) { mMutex->Lock(); diff --git a/src/dawn/native/d3d12/D3D12Backend.cpp b/src/dawn/native/d3d12/D3D12Backend.cpp index 164276f3d5..8ab9bb112b 100644 --- a/src/dawn/native/d3d12/D3D12Backend.cpp +++ b/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::Create( WGPUDevice device, const ExternalImageDescriptorDXGISharedHandle* descriptor) { Device* backendDevice = ToBackend(FromAPI(device)); + auto deviceLock(backendDevice->GetScopedLock()); std::unique_ptr 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); } diff --git a/src/dawn/native/d3d12/DeviceD3D12.cpp b/src/dawn/native/d3d12/DeviceD3D12.cpp index c4b3e2948d..e22ed65e22 100644 --- a/src/dawn/native/d3d12/DeviceD3D12.cpp +++ b/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; diff --git a/src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp b/src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp index b66a37ac15..f6eb6824a8 100644 --- a/src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp +++ b/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(mUsage))) { dawn::ErrorLog() << "Texture usage is not valid for external image"; return nullptr; } + ASSERT(mBackendDevice != nullptr); + TextureDescriptor textureDescriptor = {}; textureDescriptor.usage = static_cast(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)); diff --git a/src/dawn/native/d3d12/ExternalImageDXGIImpl.h b/src/dawn/native/d3d12/ExternalImageDXGIImpl.h index 23080db8af..a6a7d77f81 100644 --- a/src/dawn/native/d3d12/ExternalImageDXGIImpl.h +++ b/src/dawn/native/d3d12/ExternalImageDXGIImpl.h @@ -21,6 +21,7 @@ #include #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(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 mBackendDevice; Microsoft::WRL::ComPtr mD3D12Resource; const bool mUseFenceSynchronization; diff --git a/src/dawn/tests/end2end/D3D12ResourceWrappingTests.cpp b/src/dawn/tests/end2end/D3D12ResourceWrappingTests.cpp index a60e33ab98..3329126f91 100644 --- a/src/dawn/tests/end2end/D3D12ResourceWrappingTests.cpp +++ b/src/dawn/tests/end2end/D3D12ResourceWrappingTests.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -1058,9 +1059,76 @@ TEST_P(D3D12SharedHandleUsageTests, SRGBReinterpretation) { utils::RGBA8(247, 125, 105, 255), texture, 0, 0); } +class D3D12SharedHandleMultithreadTests : public D3D12SharedHandleUsageTests { + protected: + std::vector GetRequiredFeatures() override { + std::vector 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 d3d11Texture; + std::unique_ptr 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 d3d11Texture; + std::unique_ptr 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});