From 2d668e13b79ba6eafe1ea6ba849bf165ebab4003 Mon Sep 17 00:00:00 2001 From: baldurk Date: Thu, 23 Jan 2020 15:51:13 +0000 Subject: [PATCH] Prevent race condition if multiple threads create identical D3D12 object * We use HasWrapper() to deduplicate D3D12 objects which must not be created as duplicates, but we need to ensure that checking for existing wrappers and adding a new one all happens atomically with respect to other checks. --- renderdoc/driver/d3d12/d3d12_device.cpp | 2 + renderdoc/driver/d3d12/d3d12_device.h | 2 + renderdoc/driver/d3d12/d3d12_device_wrap.cpp | 52 ++++++++++++------- renderdoc/driver/d3d12/d3d12_device_wrap4.cpp | 18 +++++-- renderdoc/driver/d3d12/d3d12_resources.h | 9 ---- 5 files changed, 51 insertions(+), 32 deletions(-) diff --git a/renderdoc/driver/d3d12/d3d12_device.cpp b/renderdoc/driver/d3d12/d3d12_device.cpp index 15a0c31f8..039486eb2 100644 --- a/renderdoc/driver/d3d12/d3d12_device.cpp +++ b/renderdoc/driver/d3d12/d3d12_device.cpp @@ -1172,6 +1172,8 @@ IUnknown *WrappedID3D12Device::WrapSwapchainBuffer(IDXGISwapper *swapper, DXGI_F IDXGIResource *WrappedID3D12Device::WrapExternalDXGIResource(IDXGIResource *res) { + SCOPED_LOCK(m_WrapDeduplicateLock); + ID3D12Resource *d3d12res; res->QueryInterface(__uuidof(ID3D12Resource), (void **)&d3d12res); if(GetResourceManager()->HasWrapper(d3d12res)) diff --git a/renderdoc/driver/d3d12/d3d12_device.h b/renderdoc/driver/d3d12/d3d12_device.h index 029049c4f..8a23b6099 100644 --- a/renderdoc/driver/d3d12/d3d12_device.h +++ b/renderdoc/driver/d3d12/d3d12_device.h @@ -403,6 +403,8 @@ private: Threading::CriticalSection m_MapsLock; rdcarray m_Maps; + Threading::CriticalSection m_WrapDeduplicateLock; + bool ProcessChunk(ReadSerialiser &ser, D3D12Chunk context); unsigned int m_InternalRefcount; diff --git a/renderdoc/driver/d3d12/d3d12_device_wrap.cpp b/renderdoc/driver/d3d12/d3d12_device_wrap.cpp index 741252e7e..a04f9e634 100644 --- a/renderdoc/driver/d3d12/d3d12_device_wrap.cpp +++ b/renderdoc/driver/d3d12/d3d12_device_wrap.cpp @@ -949,17 +949,23 @@ HRESULT WrappedID3D12Device::CreateRootSignature(UINT nodeMask, const void *pBlo if(SUCCEEDED(ret)) { - // duplicate signatures can be returned, if Create is called with a previous equivalent blob - if(GetResourceManager()->HasWrapper(real)) - { - real->Release(); - ID3D12RootSignature *existing = (ID3D12RootSignature *)GetResourceManager()->GetWrapper(real); - existing->AddRef(); - *ppvRootSignature = existing; - return ret; - } + WrappedID3D12RootSignature *wrapped = NULL; - WrappedID3D12RootSignature *wrapped = new WrappedID3D12RootSignature(real, this); + { + SCOPED_LOCK(m_WrapDeduplicateLock); + + // duplicate signatures can be returned, if Create is called with a previous equivalent blob + if(GetResourceManager()->HasWrapper(real)) + { + real->Release(); + ID3D12RootSignature *existing = (ID3D12RootSignature *)GetResourceManager()->GetWrapper(real); + existing->AddRef(); + *ppvRootSignature = existing; + return ret; + } + + wrapped = new WrappedID3D12RootSignature(real, this); + } if(IsCaptureMode(m_State)) { @@ -2013,17 +2019,23 @@ HRESULT WrappedID3D12Device::CreateCommandSignature(const D3D12_COMMAND_SIGNATUR if(SUCCEEDED(ret)) { - if(GetResourceManager()->HasWrapper(real)) - { - real->Release(); - ID3D12CommandSignature *existing = - (ID3D12CommandSignature *)GetResourceManager()->GetWrapper(real); - existing->AddRef(); - *ppvCommandSignature = existing; - return ret; - } + WrappedID3D12CommandSignature *wrapped = NULL; - WrappedID3D12CommandSignature *wrapped = new WrappedID3D12CommandSignature(real, this); + { + SCOPED_LOCK(m_WrapDeduplicateLock); + + if(GetResourceManager()->HasWrapper(real)) + { + real->Release(); + ID3D12CommandSignature *existing = + (ID3D12CommandSignature *)GetResourceManager()->GetWrapper(real); + existing->AddRef(); + *ppvCommandSignature = existing; + return ret; + } + + wrapped = new WrappedID3D12CommandSignature(real, this); + } if(IsCaptureMode(m_State)) { diff --git a/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp b/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp index ece939135..b3f8c6839 100644 --- a/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp +++ b/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp @@ -504,9 +504,21 @@ D3D12_RESOURCE_ALLOCATION_INFO WrappedID3D12Device::GetResourceAllocationInfo1( ID3D12Fence *WrappedID3D12Device::CreateProtectedSessionFence(ID3D12Fence *real) { - // we basically treat this kind of like CreateFence and serialise it as such, and guess at the - // parameters to CreateFence. - WrappedID3D12Fence1 *wrapped = new WrappedID3D12Fence1(real, this); + WrappedID3D12Fence1 *wrapped = NULL; + + { + SCOPED_LOCK(m_WrapDeduplicateLock); + + // if we already have this fence wrapped, return the existing wrapper + if(GetResourceManager()->HasWrapper(real)) + { + return (ID3D12Fence *)GetResourceManager()->GetWrapper((ID3D12DeviceChild *)real); + } + + // we basically treat this kind of like CreateFence and serialise it as such, and guess at the + // parameters to CreateFence. + wrapped = new WrappedID3D12Fence1(real, this); + } if(IsCaptureMode(m_State)) { diff --git a/renderdoc/driver/d3d12/d3d12_resources.h b/renderdoc/driver/d3d12/d3d12_resources.h index ebcd17139..721bf8e07 100644 --- a/renderdoc/driver/d3d12/d3d12_resources.h +++ b/renderdoc/driver/d3d12/d3d12_resources.h @@ -521,15 +521,6 @@ public: else if(riid == __uuidof(ID3D12Fence1)) fence = (ID3D12Fence *)(ID3D12Fence1 *)iface; - // if we already have this fence wrapped, return the existing wrapper - if(m_pDevice->GetResourceManager()->HasWrapper(fence)) - { - *ppFence = - (ID3D12Fence *)m_pDevice->GetResourceManager()->GetWrapper((ID3D12DeviceChild *)fence); - return S_OK; - } - - // if not, record its creation *ppFence = m_pDevice->CreateProtectedSessionFence(fence); return S_OK; }