From 8901880d1557c008d59ea8a0d18a4d9bd1ceea39 Mon Sep 17 00:00:00 2001 From: baldurk Date: Thu, 23 Sep 2021 17:48:45 +0100 Subject: [PATCH] Ensure reference data for map comparisons is taken from serialised data * If we serialise from the map, and then separately memcpy from it to populate the reference data then we risk another thread's writes happening in between and not being detected on subsequence checks. Instead we need to copy from the serialised data, that way regardless of what we snapshot there we always compare against it to detect any further writes. --- .../driver/d3d12/d3d12_command_queue_wrap.cpp | 7 ++-- renderdoc/driver/d3d12/d3d12_commands.cpp | 3 +- renderdoc/driver/d3d12/d3d12_common.h | 1 + renderdoc/driver/d3d12/d3d12_device.cpp | 40 ++++++++++++++----- renderdoc/driver/d3d12/d3d12_device.h | 4 +- renderdoc/driver/d3d12/d3d12_stringise.cpp | 3 +- 6 files changed, 41 insertions(+), 17 deletions(-) diff --git a/renderdoc/driver/d3d12/d3d12_command_queue_wrap.cpp b/renderdoc/driver/d3d12/d3d12_command_queue_wrap.cpp index 8b5c47fe7..9980b650f 100644 --- a/renderdoc/driver/d3d12/d3d12_command_queue_wrap.cpp +++ b/renderdoc/driver/d3d12/d3d12_command_queue_wrap.cpp @@ -920,8 +920,6 @@ void WrappedID3D12CommandQueue::ExecuteCommandListsInternal(UINT NumCommandLists D3D12_RANGE range = {diffStart, diffEnd}; - m_pDevice->MapDataWrite(res, subres, data, range); - if(ref == NULL) { res->AllocShadow(subres, size); @@ -929,8 +927,9 @@ void WrappedID3D12CommandQueue::ExecuteCommandListsInternal(UINT NumCommandLists ref = res->GetShadow(subres); } - // update comparison shadow for next time - memcpy(ref, data, size); + // passing true here asks the serialisation function to update the shadow pointer for + // this resource + m_pDevice->MapDataWrite(res, subres, data, range, true); GetResourceManager()->MarkDirtyResource(res->GetResourceID()); } diff --git a/renderdoc/driver/d3d12/d3d12_commands.cpp b/renderdoc/driver/d3d12/d3d12_commands.cpp index d39e46575..6a32847ef 100644 --- a/renderdoc/driver/d3d12/d3d12_commands.cpp +++ b/renderdoc/driver/d3d12/d3d12_commands.cpp @@ -792,8 +792,9 @@ bool WrappedID3D12CommandQueue::ProcessChunk(ReadSerialiser &ser, D3D12Chunk chu case D3D12Chunk::PopMarker: ret = m_ReplayList->Serialise_EndEvent(ser); break; case D3D12Chunk::SetMarker: ret = m_ReplayList->Serialise_SetMarker(ser, 0, NULL, 0); break; + case D3D12Chunk::CoherentMapWrite: case D3D12Chunk::Resource_Unmap: - ret = m_pDevice->Serialise_MapDataWrite(ser, NULL, 0, NULL, D3D12_RANGE()); + ret = m_pDevice->Serialise_MapDataWrite(ser, NULL, 0, NULL, D3D12_RANGE(), false); break; case D3D12Chunk::Resource_WriteToSubresource: ret = m_pDevice->Serialise_WriteToSubresource(ser, NULL, 0, NULL, NULL, 0, 0); diff --git a/renderdoc/driver/d3d12/d3d12_common.h b/renderdoc/driver/d3d12/d3d12_common.h index 788281338..ff88a5ff1 100644 --- a/renderdoc/driver/d3d12/d3d12_common.h +++ b/renderdoc/driver/d3d12/d3d12_common.h @@ -902,5 +902,6 @@ enum class D3D12Chunk : uint32_t Device_CreateCommittedResource2, Device_CreatePlacedResource1, Device_CreateCommandQueue1, + CoherentMapWrite, Max, }; diff --git a/renderdoc/driver/d3d12/d3d12_device.cpp b/renderdoc/driver/d3d12/d3d12_device.cpp index b5b068f0a..94638ec9c 100644 --- a/renderdoc/driver/d3d12/d3d12_device.cpp +++ b/renderdoc/driver/d3d12/d3d12_device.cpp @@ -1694,13 +1694,13 @@ void WrappedID3D12Device::Unmap(ID3D12Resource *Resource, UINT Subresource, byte } if(capframe) - MapDataWrite(Resource, Subresource, mapPtr, range); + MapDataWrite(Resource, Subresource, mapPtr, range, false); } template bool WrappedID3D12Device::Serialise_MapDataWrite(SerialiserType &ser, ID3D12Resource *Resource, UINT Subresource, byte *MappedData, - D3D12_RANGE range) + D3D12_RANGE range, bool coherentFlush) { SERIALISE_ELEMENT(Resource).Important(); SERIALISE_ELEMENT(Subresource); @@ -1737,12 +1737,31 @@ bool WrappedID3D12Device::Serialise_MapDataWrite(SerialiserType &ser, ID3D12Reso ser.Serialise("MappedData"_lit, MappedData, range.End - range.Begin, flags).Important(); + size_t dataOffset = 0; + if(ser.IsWriting()) + dataOffset = ser.GetWriter()->GetOffset() - (range.End - range.Begin); + SERIALISE_ELEMENT(range); uint64_t rangeSize = range.End - range.Begin; SERIALISE_CHECK_READ_ERRORS(); + if(ser.IsWriting() && coherentFlush) + { + byte *ref = GetWrapped(Resource)->GetShadow(Subresource); + const byte *serialisedData = ser.GetWriter()->GetData() + dataOffset; + + if(ref) + { + memcpy(ref + range.Begin, serialisedData, size_t(range.End - range.Begin)); + } + else + { + RDCERR("Shadow data not allocated when coherent map flush is processed!"); + } + } + // don't do anything if end <= begin because the range is empty. if(IsReplayingAndReading() && Resource && range.End > range.Begin) { @@ -1824,19 +1843,21 @@ bool WrappedID3D12Device::Serialise_MapDataWrite(SerialiserType &ser, ID3D12Reso } template bool WrappedID3D12Device::Serialise_MapDataWrite(ReadSerialiser &ser, - ID3D12Resource *Resource, UINT Subresource, - byte *MappedData, D3D12_RANGE range); + ID3D12Resource *Resource, + UINT Subresource, byte *MappedData, + D3D12_RANGE range, bool coherentFlush); template bool WrappedID3D12Device::Serialise_MapDataWrite(WriteSerialiser &ser, - ID3D12Resource *Resource, UINT Subresource, - byte *MappedData, D3D12_RANGE range); + ID3D12Resource *Resource, + UINT Subresource, byte *MappedData, + D3D12_RANGE range, bool coherentFlush); void WrappedID3D12Device::MapDataWrite(ID3D12Resource *Resource, UINT Subresource, byte *mapPtr, - D3D12_RANGE range) + D3D12_RANGE range, bool coherentFlush) { CACHE_THREAD_SERIALISER(); - SCOPED_SERIALISE_CHUNK(D3D12Chunk::Resource_Unmap); - Serialise_MapDataWrite(ser, Resource, Subresource, mapPtr, range); + SCOPED_SERIALISE_CHUNK(coherentFlush ? D3D12Chunk::CoherentMapWrite : D3D12Chunk::Resource_Unmap); + Serialise_MapDataWrite(ser, Resource, Subresource, mapPtr, range, coherentFlush); m_FrameCaptureRecord->AddChunk(scope.Get()); @@ -3814,6 +3835,7 @@ bool WrappedID3D12Device::ProcessChunk(ReadSerialiser &ser, D3D12Chunk context) case D3D12Chunk::List_IndirectSubCommand: case D3D12Chunk::Swapchain_Present: case D3D12Chunk::List_ClearState: + case D3D12Chunk::CoherentMapWrite: RDCERR("Unexpected chunk while processing initialisation: %s", ToStr(context).c_str()); return false; diff --git a/renderdoc/driver/d3d12/d3d12_device.h b/renderdoc/driver/d3d12/d3d12_device.h index 6a8016631..fb424070e 100644 --- a/renderdoc/driver/d3d12/d3d12_device.h +++ b/renderdoc/driver/d3d12/d3d12_device.h @@ -1072,8 +1072,8 @@ public: return E_NOINTERFACE; } - IMPLEMENT_FUNCTION_THREAD_SERIALISED(void, MapDataWrite, ID3D12Resource *Resource, - UINT Subresource, byte *mapPtr, D3D12_RANGE range); + IMPLEMENT_FUNCTION_THREAD_SERIALISED(void, MapDataWrite, ID3D12Resource *Resource, UINT Subresource, + byte *mapPtr, D3D12_RANGE range, bool coherentFlush); IMPLEMENT_FUNCTION_THREAD_SERIALISED(void, WriteToSubresource, ID3D12Resource *Resource, UINT Subresource, const D3D12_BOX *pDstBox, const void *pSrcData, UINT SrcRowPitch, UINT SrcDepthPitch); diff --git a/renderdoc/driver/d3d12/d3d12_stringise.cpp b/renderdoc/driver/d3d12/d3d12_stringise.cpp index 3b5f66346..551cbb9f2 100644 --- a/renderdoc/driver/d3d12/d3d12_stringise.cpp +++ b/renderdoc/driver/d3d12/d3d12_stringise.cpp @@ -28,7 +28,7 @@ template <> rdcstr DoStringise(const D3D12Chunk &el) { - RDCCOMPILE_ASSERT((uint32_t)D3D12Chunk::Max == 1113, "Chunks changed without updating names"); + RDCCOMPILE_ASSERT((uint32_t)D3D12Chunk::Max == 1114, "Chunks changed without updating names"); BEGIN_ENUM_STRINGISE(D3D12Chunk) { @@ -204,6 +204,7 @@ rdcstr DoStringise(const D3D12Chunk &el) STRINGISE_ENUM_CLASS_NAMED(Device_CreatePlacedResource1, "ID3D12Device8::CreatePlacedResource1"); STRINGISE_ENUM_CLASS_NAMED(Device_CreateCommandQueue1, "ID3D12Device9::CreateCommandQueue1"); + STRINGISE_ENUM_CLASS_NAMED(CoherentMapWrite, "Internal::Coherent Mapped Memory Write"); STRINGISE_ENUM_CLASS_NAMED(Max, "Max Chunk"); } END_ENUM_STRINGISE()