From 9cc10b9307e694498d89379f09b4b25f080a1802 Mon Sep 17 00:00:00 2001 From: baldurk Date: Thu, 14 Nov 2024 13:30:20 +0000 Subject: [PATCH] Always give new ASs a new ID, never re-use an ID * For ease of tracking this is simpler as there is currently no situation where we need to know the providence of an AS and what ID it was before if it's rebuilt in place with the same size (e.g. an in-place update). On the other hand it is useful to know when an AS is a new build for potential TLAS invalidation or lifetime checking. --- .../driver/d3d12/d3d12_command_list4_wrap.cpp | 98 +++++++------------ renderdoc/driver/d3d12/d3d12_replay.cpp | 4 +- renderdoc/driver/d3d12/d3d12_resources.cpp | 36 +++---- 3 files changed, 53 insertions(+), 85 deletions(-) diff --git a/renderdoc/driver/d3d12/d3d12_command_list4_wrap.cpp b/renderdoc/driver/d3d12/d3d12_command_list4_wrap.cpp index 652cd7339..279213231 100644 --- a/renderdoc/driver/d3d12/d3d12_command_list4_wrap.cpp +++ b/renderdoc/driver/d3d12/d3d12_command_list4_wrap.cpp @@ -732,78 +732,52 @@ bool WrappedID3D12GraphicsCommandList::ProcessASBuildAfterSubmission(ResourceId UINT64 byteSize, ASBuildData *buildData) { - bool success = false; - D3D12ResourceManager *resManager = m_pDevice->GetResourceManager(); + D3D12ResourceManager *rm = m_pDevice->GetResourceManager(); + WrappedID3D12Resource *dstASB = rm->GetCurrentAs(destASBId); + + // unconditionally create a new AS at this location, never allow re-use even in the case of + // in-place update builds. This makes it easier to track ASs and we will not run out of + // ResourceIds. There is no case where we need to track an update (it may as well be a new build + // for our purposes), and when validating if a TLAS should be rebuilt based on the existing inputs + // it is easier if each build is new, as each build of a BLAS invalidates the TLAS. + // + // CreateAccStruct deletes any previous overlapping ASs on the ASB D3D12AccelerationStructure *accStructAtDestOffset = NULL; - - WrappedID3D12Resource *dstASB = resManager->GetCurrentAs(destASBId); - - // See if acc already exist at the given offset - bool accStructExistAtDestOffset = - dstASB->GetAccStructIfExist(destASBOffset, &accStructAtDestOffset); - - bool createAccStruct = false; - - if(accStructExistAtDestOffset) + if(dstASB->CreateAccStruct(destASBOffset, byteSize, &accStructAtDestOffset)) { - if(accStructAtDestOffset && accStructAtDestOffset->Size() != byteSize) + D3D12ResourceRecord *record = rm->AddResourceRecord(accStructAtDestOffset->GetResourceID()); + record->type = Resource_AccelerationStructure; + record->Length = 0; + accStructAtDestOffset->SetResourceRecord(record); + rm->MarkDirtyResource(accStructAtDestOffset->GetResourceID()); + + record->AddParent(rm->GetResourceRecord(accStructAtDestOffset->GetBackingBufferResourceId())); + + // register this AS so its resource can be created during replay + m_pDevice->CreateAS(dstASB, destASBOffset, byteSize, accStructAtDestOffset); + + m_pDevice->AddForcedReference(record); + // in case we're currently capturing, immediately consider the AS as referenced + GetResourceManager()->MarkResourceFrameReferenced(accStructAtDestOffset->GetResourceID(), + eFrameRef_Read); + + if(buildData) { - dstASB->DeleteAccStructAtOffset(destASBOffset); - createAccStruct = true; - } - else - { - // if the AS is being rebuilt in place, that's also successful - success = true; + // release any existing build data we had, this is a new version + SAFE_RELEASE(accStructAtDestOffset->buildData); + + // take ownership of the implicit ref + accStructAtDestOffset->buildData = buildData; } } else { - createAccStruct = true; + RDCERR("Unable to create acceleration structure"); + return false; } - if(createAccStruct) - { - // CreateAccStruct also deletes any previous overlapping ASs on the ASB - if(dstASB->CreateAccStruct(destASBOffset, byteSize, &accStructAtDestOffset)) - { - success = true; - D3D12ResourceRecord *record = - resManager->AddResourceRecord(accStructAtDestOffset->GetResourceID()); - record->type = Resource_AccelerationStructure; - record->Length = 0; - accStructAtDestOffset->SetResourceRecord(record); - resManager->MarkDirtyResource(accStructAtDestOffset->GetResourceID()); - - record->AddParent( - resManager->GetResourceRecord(accStructAtDestOffset->GetBackingBufferResourceId())); - - // register this AS so its resource can be created during replay - m_pDevice->CreateAS(dstASB, destASBOffset, byteSize, accStructAtDestOffset); - - m_pDevice->AddForcedReference(record); - // in case we're currently capturing, immediately consider the AS as referenced - GetResourceManager()->MarkResourceFrameReferenced(accStructAtDestOffset->GetResourceID(), - eFrameRef_Read); - } - else - { - RDCERR("Unable to create acceleration structure"); - success = false; - } - } - - if(success && buildData) - { - // release any existing build data we had, this is a new version - SAFE_RELEASE(accStructAtDestOffset->buildData); - - // take ownership of the implicit ref - accStructAtDestOffset->buildData = buildData; - } - - return success; + return true; } bool WrappedID3D12GraphicsCommandList::PatchAccStructBlasAddress( diff --git a/renderdoc/driver/d3d12/d3d12_replay.cpp b/renderdoc/driver/d3d12/d3d12_replay.cpp index 8aa9cce6d..e74a2f7bd 100644 --- a/renderdoc/driver/d3d12/d3d12_replay.cpp +++ b/renderdoc/driver/d3d12/d3d12_replay.cpp @@ -833,9 +833,7 @@ void D3D12Replay::FillDescriptor(Descriptor &dst, const D3D12Descriptor *src) { // we *should* get an AS here D3D12AccelerationStructure *as = NULL; - asRes->GetAccStructIfExist(dst.byteOffset, &as); - - if(as) + if(asRes->GetAccStructIfExist(dst.byteOffset, &as)) { dst.resource = rm->GetOriginalID(as->GetResourceID()); dst.byteOffset = 0; diff --git a/renderdoc/driver/d3d12/d3d12_resources.cpp b/renderdoc/driver/d3d12/d3d12_resources.cpp index f40177bda..698436bad 100644 --- a/renderdoc/driver/d3d12/d3d12_resources.cpp +++ b/renderdoc/driver/d3d12/d3d12_resources.cpp @@ -162,25 +162,23 @@ bool WrappedID3D12Resource::CreateAccStruct(D3D12BufferOffset bufferOffset, UINT D3D12AccelerationStructure **accStruct) { SCOPED_LOCK(m_accStructResourcesCS); - if(m_accelerationStructMap.find(bufferOffset) == m_accelerationStructMap.end()) + auto existing = m_accelerationStructMap.find(bufferOffset); + if(existing != m_accelerationStructMap.end()) { - m_accelerationStructMap[bufferOffset] = - new D3D12AccelerationStructure(m_pDevice, this, bufferOffset, byteSize); - - if(accStruct) - { - *accStruct = m_accelerationStructMap[bufferOffset]; - - if(IsCaptureMode(m_pDevice->GetState())) - { - DeleteOverlappingAccStructsInRangeAtOffset(bufferOffset); - } - } - - return true; + if(IsCaptureMode(m_pDevice->GetState())) + RDCASSERTEQUAL((uint32_t)existing->second->Release(), 0); + m_accelerationStructMap.erase(existing); } - return false; + m_accelerationStructMap[bufferOffset] = + new D3D12AccelerationStructure(m_pDevice, this, bufferOffset, byteSize); + + *accStruct = m_accelerationStructMap[bufferOffset]; + + if(IsCaptureMode(m_pDevice->GetState())) + DeleteOverlappingAccStructsInRangeAtOffset(bufferOffset); + + return true; } WrappedID3D12Resource::~WrappedID3D12Resource() @@ -434,10 +432,8 @@ bool WrappedID3D12Resource::DeleteAccStructAtOffset(D3D12BufferOffset bufferOffs D3D12AccelerationStructure *accStruct = NULL; if(GetAccStructIfExist(bufferOffset, &accStruct)) { - if(m_accelerationStructMap[bufferOffset]->Release() == 0) - { - m_accelerationStructMap.erase(bufferOffset); - } + RDCASSERTEQUAL((uint32_t)accStruct->Release(), 0); + m_accelerationStructMap.erase(bufferOffset); return true; }