From 107f89048b344ee7913c2c0baa339daaa60018de Mon Sep 17 00:00:00 2001 From: baldurk Date: Mon, 9 Mar 2020 14:28:03 +0000 Subject: [PATCH] Fix race condition when keeping D3D12 buffers alive during capture * There was a gap between stopping active capturing (meaning newly created buffers are given an extra refcount) and releasing the extra refcount. Any newly created buffers in that gap would still be released but that would mean one of the application's ref's is removed, possibly destroying a buffer by mistake. * Instead we keep a specific list of buffers that we're extending so that we know that the buffers we release are exactly those we gave extra refs to. --- renderdoc/driver/d3d12/d3d12_device.cpp | 21 ++++++--- renderdoc/driver/d3d12/d3d12_device.h | 7 +++ renderdoc/driver/d3d12/d3d12_device_wrap.cpp | 47 +++++++++++++++++-- renderdoc/driver/d3d12/d3d12_device_wrap4.cpp | 12 +++++ renderdoc/driver/d3d12/d3d12_resources.cpp | 20 ++++---- renderdoc/driver/d3d12/d3d12_resources.h | 9 +--- 6 files changed, 87 insertions(+), 29 deletions(-) diff --git a/renderdoc/driver/d3d12/d3d12_device.cpp b/renderdoc/driver/d3d12/d3d12_device.cpp index 9531b32fe..c791113c8 100644 --- a/renderdoc/driver/d3d12/d3d12_device.cpp +++ b/renderdoc/driver/d3d12/d3d12_device.cpp @@ -1829,8 +1829,8 @@ void WrappedID3D12Device::StartFrameCapture(void *dev, void *wnd) eFrameRef_Read); } - // also keep buffers alive - WrappedID3D12Resource1::AddRefBuffersBeforeCapture(GetResourceManager()); + m_RefQueues = m_Queues; + m_RefBuffers = WrappedID3D12Resource1::AddRefBuffersBeforeCapture(GetResourceManager()); } GetResourceManager()->MarkResourceFrameReferenced(m_ResourceID, eFrameRef_Read); @@ -2136,14 +2136,14 @@ bool WrappedID3D12Device::EndFrameCapture(void *dev, void *wnd) SAFE_DELETE(m_HeaderChunk); for(auto it = queues.begin(); it != queues.end(); ++it) - { (*it)->ClearAfterCapture(); - // remove the reference held during capture, potentially releasing the queue. - (*it)->Release(); - } + // remove the references held during capture, potentially releasing the queue/buffer. + for(WrappedID3D12CommandQueue *q : m_RefQueues) + q->Release(); - WrappedID3D12Resource1::ReleaseBuffersAfterCapture(GetResourceManager()); + for(ID3D12Resource *r : m_RefBuffers) + r->Release(); GetResourceManager()->MarkUnwrittenResources(); @@ -2189,6 +2189,13 @@ bool WrappedID3D12Device::DiscardFrameCapture(void *dev, void *wnd) for(auto it = queues.begin(); it != queues.end(); ++it) (*it)->ClearAfterCapture(); + // remove the reference held during capture, potentially releasing the queue. + for(WrappedID3D12CommandQueue *q : m_RefQueues) + q->Release(); + + for(ID3D12Resource *r : m_RefBuffers) + r->Release(); + GetResourceManager()->MarkUnwrittenResources(); GetResourceManager()->ClearReferencedResources(); diff --git a/renderdoc/driver/d3d12/d3d12_device.h b/renderdoc/driver/d3d12/d3d12_device.h index 8a23b6099..9c54bcfeb 100644 --- a/renderdoc/driver/d3d12/d3d12_device.h +++ b/renderdoc/driver/d3d12/d3d12_device.h @@ -361,6 +361,13 @@ private: rdcarray m_Queues; rdcarray m_QueueFences; + // list of queues and buffers kept alive during capture artificially even if the user destroys + // them, so we can use them in the capture. Storing this separately prevents races where a + // queue/buffer is added between us transitioning away from active capturing (so we don't addref + // it) and us releasing our reference on them. + rdcarray m_RefQueues; + rdcarray m_RefBuffers; + // the queue we use for all internal work, the first DIRECT queue WrappedID3D12CommandQueue *m_Queue; diff --git a/renderdoc/driver/d3d12/d3d12_device_wrap.cpp b/renderdoc/driver/d3d12/d3d12_device_wrap.cpp index 981bffe7b..31057606b 100644 --- a/renderdoc/driver/d3d12/d3d12_device_wrap.cpp +++ b/renderdoc/driver/d3d12/d3d12_device_wrap.cpp @@ -136,15 +136,20 @@ HRESULT WrappedID3D12Device::CreateCommandQueue(const D3D12_COMMAND_QUEUE_DESC * { SCOPED_READLOCK(m_CapTransitionLock); capframe = IsActiveCapturing(m_State); + + // while capturing don't allow any queues to be freed, by adding another refcount, since we + // gather any commands submitted to them at the end of the capture. + if(capframe) + { + wrapped->AddRef(); + m_RefQueues.push_back(wrapped); + } } - // while capturing don't allow any queues to be freed, by adding another refcount, since we - // gather any commands submitted to them at the end of the capture. if(capframe) { GetResourceManager()->MarkResourceFrameReferenced( wrapped->GetCreationRecord()->GetResourceID(), eFrameRef_Read); - wrapped->AddRef(); } *ppCommandQueue = (ID3D12CommandQueue *)wrapped; @@ -1475,6 +1480,18 @@ HRESULT WrappedID3D12Device::CreateCommittedResource(const D3D12_HEAP_PROPERTIES } *ppvResource = (ID3D12Resource *)wrapped; + + // while actively capturing we keep all buffers around to prevent the address lookup from + // losing addresses we might need (or the manageable but annoying problem of an address being + // re-used) + { + SCOPED_READLOCK(m_CapTransitionLock); + if(IsActiveCapturing(m_State)) + { + wrapped->AddRef(); + m_RefBuffers.push_back(wrapped); + } + } } return ret; @@ -1750,6 +1767,18 @@ HRESULT WrappedID3D12Device::CreatePlacedResource(ID3D12Heap *pHeap, UINT64 Heap } *ppvResource = (ID3D12Resource *)wrapped; + + // while actively capturing we keep all buffers around to prevent the address lookup from + // losing addresses we might need (or the manageable but annoying problem of an address being + // re-used) + { + SCOPED_READLOCK(m_CapTransitionLock); + if(IsActiveCapturing(m_State)) + { + wrapped->AddRef(); + m_RefBuffers.push_back(wrapped); + } + } } return ret; @@ -2574,6 +2603,18 @@ HRESULT WrappedID3D12Device::OpenSharedHandleInternal(D3D12Chunk chunkType, REFI states.fill(GetNumSubresources(m_pDevice, &desc), InitialResourceState); } + + // while actively capturing we keep all buffers around to prevent the address lookup from + // losing addresses we might need (or the manageable but annoying problem of an address being + // re-used) + { + SCOPED_READLOCK(m_CapTransitionLock); + if(IsActiveCapturing(m_State)) + { + wrapped->AddRef(); + m_RefBuffers.push_back(wrapped); + } + } } CACHE_THREAD_SERIALISER(); diff --git a/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp b/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp index b3f8c6839..95e54619c 100644 --- a/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp +++ b/renderdoc/driver/d3d12/d3d12_device_wrap4.cpp @@ -379,6 +379,18 @@ HRESULT WrappedID3D12Device::CreateCommittedResource1( } *ppvResource = (ID3D12Resource *)wrapped; + + // while actively capturing we keep all buffers around to prevent the address lookup from + // losing addresses we might need (or the manageable but annoying problem of an address being + // re-used) + { + SCOPED_READLOCK(m_CapTransitionLock); + if(IsActiveCapturing(m_State)) + { + wrapped->AddRef(); + m_RefBuffers.push_back(wrapped); + } + } } return ret; diff --git a/renderdoc/driver/d3d12/d3d12_resources.cpp b/renderdoc/driver/d3d12/d3d12_resources.cpp index 0a231eb96..f05135766 100644 --- a/renderdoc/driver/d3d12/d3d12_resources.cpp +++ b/renderdoc/driver/d3d12/d3d12_resources.cpp @@ -463,26 +463,24 @@ void WrappedID3D12Resource1::RefBuffers(D3D12ResourceManager *rm) rm->MarkResourceFrameReferenced(m_Addresses.addresses[i].id, eFrameRef_Read); } -void WrappedID3D12Resource1::AddRefBuffersBeforeCapture(D3D12ResourceManager *rm) +rdcarray WrappedID3D12Resource1::AddRefBuffersBeforeCapture(D3D12ResourceManager *rm) { - SCOPED_READLOCK(m_Addresses.addressLock); - for(size_t i = 0; i < m_Addresses.addresses.size(); i++) - rm->GetCurrentResource(m_Addresses.addresses[i].id)->AddRef(); -} + rdcarray ret; -void WrappedID3D12Resource1::ReleaseBuffersAfterCapture(D3D12ResourceManager *rm) -{ - // make a copy because we might release the last reference on a buffer which will need to modify - // the actual m_Addresses rdcarray addresses; - { SCOPED_READLOCK(m_Addresses.addressLock); addresses = m_Addresses.addresses; } for(size_t i = 0; i < addresses.size(); i++) - rm->GetCurrentResource(addresses[i].id)->Release(); + { + ID3D12Resource *resource = (ID3D12Resource *)rm->GetCurrentResource(m_Addresses.addresses[i].id); + resource->AddRef(); + ret.push_back(resource); + } + + return ret; } WrappedID3D12DescriptorHeap::WrappedID3D12DescriptorHeap(ID3D12DescriptorHeap *real, diff --git a/renderdoc/driver/d3d12/d3d12_resources.h b/renderdoc/driver/d3d12/d3d12_resources.h index 43396b12e..4edc0addc 100644 --- a/renderdoc/driver/d3d12/d3d12_resources.h +++ b/renderdoc/driver/d3d12/d3d12_resources.h @@ -879,8 +879,7 @@ public: static void RefBuffers(D3D12ResourceManager *rm); - static void AddRefBuffersBeforeCapture(D3D12ResourceManager *rm); - static void ReleaseBuffersAfterCapture(D3D12ResourceManager *rm); + static rdcarray AddRefBuffersBeforeCapture(D3D12ResourceManager *rm); static void GetResIDFromAddr(D3D12_GPU_VIRTUAL_ADDRESS addr, ResourceId &id, UINT64 &offs) { @@ -924,12 +923,6 @@ public: range.id = GetResourceID(); m_Addresses.AddTo(range); - - // while actively capturing we keep all buffers around to prevent the address lookup from - // losing addresses we might need (or the manageable but annoying problem of an address being - // re-used) - if(IsActiveCapturing(device->GetState())) - AddRef(); } } virtual ~WrappedID3D12Resource1();