Skip to content

Commit

Permalink
Refactor locking
Browse files Browse the repository at this point in the history 
Handle a potential suballocator zero-ing that could occur between mapping and unmapping dirtying the writewatch

Need to map immediately to handle aliased resources nicer.
  • Loading branch information
@misyltoad
misyltoad committed Aug 16, 2021
1 parent 9d49603 commit 3208ff0
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 96 deletions.
114 changes: 48 additions & 66 deletions helpers/d3d12pba.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ struct _D3D12_MAP_DESC
void* pData;
SIZE_T Size;
_D3D12_MAPPING_TYPE MappingType;
uint32_t RefCount;

struct
{
Expand All @@ -62,21 +61,19 @@ struct _D3D12_MAP_DESC
pData(pData),
Size(Size),
MappingType(Type),
RefCount(1),
ExceptionPath({ {}, OldProtect })
{}

_D3D12_MAP_DESC() :
pData(nullptr),
Size(0),
MappingType(_D3D12_MAPPING_INVALID),
RefCount(1),
ExceptionPath({ {}, 0 })
{}
};

extern std::map<SIZE_T, std::map<UINT, _D3D12_MAP_DESC>> g_D3D12AddressMappings;
extern std::mutex g_D3D12AddressMappingsMutex;
extern std::recursive_mutex g_D3D12AddressMappingsMutex;

static inline bool
_guard_mapped_memory(const _D3D12_MAP_DESC& Desc, DWORD* OldProtect)
Expand Down Expand Up @@ -150,6 +147,7 @@ _flush_mapping_watch_memcpys(_D3D12_MAP_DESC& mapping)
}

size_t size = size_t(end_copy_range - start_copy_range);
assert(size <= mapping.Size);
trace::fakeMemcpy(reinterpret_cast<void*>(start_copy_range), size);
#else
uintptr_t start_copy_range = std::max(base_address, resource_start);
Expand Down Expand Up @@ -239,7 +237,7 @@ _get_heap_flags(ID3D12Resource* pResource)
}

static inline void
_map_resource(ID3D12Resource* pResource, UINT Subresource, void* pData)
_map_subresource(ID3D12Resource* pResource, UINT Subresource, void* pData)
{
D3D12_HEAP_FLAGS flags = _get_heap_flags(pResource);

Expand All @@ -258,12 +256,7 @@ _map_resource(ID3D12Resource* pResource, UINT Subresource, void* pData)
resource_iter = g_D3D12AddressMappings.find(key);

auto subresource_iter = resource_iter->second.find(Subresource);
if (subresource_iter != resource_iter->second.end()) {
if (!subresource_iter->second.pData && pData)
subresource_iter->second.pData = pData;
subresource_iter->second.RefCount++;
}
else
if (subresource_iter == resource_iter->second.end())
resource_iter->second[Subresource] = _D3D12_MAP_DESC(_D3D12_MAPPING_WRITE_WATCH, pData, _getMapSize(pResource, Subresource));
}

Expand Down Expand Up @@ -297,7 +290,7 @@ static inline size_t _d3d12_AllocationSize(const void *pAddress)


static inline void
_map_resource(ID3D12Heap* pResource, void* pData)
_map_heap(ID3D12Heap* pResource, void* pData)
{
SIZE_T allocation_size = _d3d12_AllocationSize(pData);
if (!allocation_size)
Expand All @@ -315,14 +308,12 @@ _map_resource(ID3D12Heap* pResource, void* pData)
resource_iter = g_D3D12AddressMappings.find(key);

auto subresource_iter = resource_iter->second.find(0);
if (subresource_iter != resource_iter->second.end())
subresource_iter->second.RefCount++;
else
if (subresource_iter == resource_iter->second.end())
resource_iter->second[0] = _D3D12_MAP_DESC(_D3D12_MAPPING_WRITE_WATCH, pData, allocation_size);
}

static inline void
_unmap_resource(SIZE_T key, UINT subresource, bool forceUnmap)
_unmap_subresource(SIZE_T key, UINT subresource)
{
auto resource = g_D3D12AddressMappings.find(key);
if (resource == g_D3D12AddressMappings.end())
Expand All @@ -332,66 +323,54 @@ _unmap_resource(SIZE_T key, UINT subresource, bool forceUnmap)
if (iter == resource->second.end())
return;

assert(iter->second.RefCount);
iter->second.RefCount--;

// If this is going to release the mapping, then flush memcpys here.
if (iter->second.RefCount == 0 || forceUnmap) {
auto& mapping = iter->second;
auto& mapping = iter->second;

if (mapping.MappingType == _D3D12_MAPPING_WRITE_WATCH)
{
// WriteWatch mappings.
_flush_mapping_watch_memcpys(mapping);

// TODO(Josh): Is this the right thing to do for aliased resources??? :<<<<<<
// I think this won't work for buffers
// Not enabling this for now... It'll probably work everywhere
// But what about the funny edge case with
// [buffer1 ]
// [buffer2 XXXXXX]
// and buffer 1 gets unmapped
// but XXXXX is in buffer 1's page boundaries but
// then it's unmapped after that watch will be destroyed
// AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
// THIS API SUCKS!!!!!!!!!!!!!!!!!!!!!!
//_reset_writewatch(mapping);
}
else if (mapping.MappingType == _D3D12_MAPPING_EXCEPTION)
{
// Exception mappings.
_flush_mapping_exception_memcpys(mapping);
assert(_getMapSize((ID3D12Resource*)key, subresource) == mapping.Size);

// No need to re-apply the guard as we are unmapping now.
}
else
{
os::log("apitrace: Unhandled mapping type\n");
assert(false);
}
if (mapping.MappingType == _D3D12_MAPPING_WRITE_WATCH)
{
// WriteWatch mappings.
_flush_mapping_watch_memcpys(mapping);

// TODO(Josh): Is this the right thing to do for aliased resources??? :<<<<<<
// I think this won't work for buffers
// Not enabling this for now... It'll probably work everywhere
// But what about the funny edge case with
// [buffer1 ]
// [buffer2 XXXXXX]
// and buffer 1 gets unmapped
// but XXXXX is in buffer 1's page boundaries but
// then it's unmapped after that watch will be destroyed
// AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
// THIS API SUCKS!!!!!!!!!!!!!!!!!!!!!!
//_reset_writewatch(mapping);
}
else if (mapping.MappingType == _D3D12_MAPPING_EXCEPTION)
{
// Exception mappings.
_flush_mapping_exception_memcpys(mapping);

resource->second.erase(iter);
// No need to re-apply the guard as we are unmapping now.
}
else
{
os::log("apitrace: Unhandled mapping type\n");
assert(false);
}

if (resource->second.empty())
g_D3D12AddressMappings.erase(resource);
resource->second.erase(iter);
}

static inline void
_unmap_resource(ID3D12Resource* pResource, UINT Subresource, bool forceUnmap = false)
_unmap_subresource(ID3D12Resource* pResource, UINT Subresource)
{
D3D12_HEAP_FLAGS flags = _get_heap_flags(pResource);

if (!(flags & D3D12_HEAP_FLAG_ALLOW_WRITE_WATCH))
return;

_unmap_resource(static_cast<SIZE_T>(reinterpret_cast<uintptr_t>(pResource)), Subresource, forceUnmap);
}

static inline void
_unmap_resource(ID3D12Heap* pResource, bool forceUnmap = false)
{
_unmap_resource(static_cast<SIZE_T>(reinterpret_cast<uintptr_t>(pResource)), 0, forceUnmap);
_unmap_subresource(static_cast<SIZE_T>(reinterpret_cast<uintptr_t>(pResource)), Subresource);
}

static inline void
Expand All @@ -411,15 +390,17 @@ _fully_unmap_resource(ID3D12Resource* pResource)
std::vector<UINT> subresources_to_unmap;
for (auto& subresource : resource->second)
subresources_to_unmap.push_back(subresource.first);

for (UINT subresource : subresources_to_unmap)
_unmap_resource(key, subresource, true);
_unmap_subresource(key, subresource);

g_D3D12AddressMappings.erase(resource);
}

static inline void
_fully_unmap_resource(ID3D12Heap* pResource)
{
_unmap_resource(static_cast<SIZE_T>(reinterpret_cast<uintptr_t>(pResource)), 0, true);
_unmap_subresource(static_cast<SIZE_T>(reinterpret_cast<uintptr_t>(pResource)), 0);
}

static inline void
Expand All @@ -438,7 +419,8 @@ _flush_mappings()
for (auto& element : resource.second)
{
auto& mapping = element.second;
assert(mapping.RefCount);

assert(_getMapSize((ID3D12Resource*)resource.first, resource.first) == mapping.Size);

if (mapping.MappingType == _D3D12_MAPPING_WRITE_WATCH)
{
Expand Down Expand Up @@ -487,7 +469,7 @@ _setup_seh()
bool wasWrite = static_cast<bool> (pException->ExceptionRecord->ExceptionInformation[0]);
uintptr_t address = static_cast<uintptr_t>(pException->ExceptionRecord->ExceptionInformation[1]);

auto lock = std::unique_lock<std::mutex>(g_D3D12AddressMappingsMutex);
auto lock = std::unique_lock<std::recursive_mutex>(g_D3D12AddressMappingsMutex);

for (auto& resource : g_D3D12AddressMappings) {
for (auto& element : resource.second)
Expand Down
27 changes: 26 additions & 1 deletion helpers/d3d12size.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ _calcSubresourceSize12(ID3D12Resource *pDstResource, UINT DstSubresource, const
return _calcDataSize(Desc.Format, Width, Height, SrcRowPitch, Depth, SrcDepthPitch);
}


static inline UINT64
_getMapSize(ID3D12Resource* pResource, UINT Subresource)
{
Expand All @@ -97,6 +96,32 @@ _getMapSize(ID3D12Resource* pResource, UINT Subresource)
return TotalBytes;
}

static inline UINT
_getLayerCount(D3D12_RESOURCE_DESC* pDesc)
{
return pDesc->Dimension != D3D12_RESOURCE_DIMENSION_TEXTURE3D ? pDesc->DepthOrArraySize : 1;
}

static inline UINT
_getSubresourceCount(ID3D12Resource* pResource)
{
D3D12_RESOURCE_DESC desc = pResource->GetDesc();

UINT planeCount = 1;
if (desc.Dimension != D3D12_RESOURCE_DIMENSION_BUFFER) {
com_ptr<ID3D12Device> pDevice;
pResource->GetDevice(IID_ID3D12Device, (void**)&pDevice);

D3D12_FEATURE_DATA_FORMAT_INFO formatInfo{};
formatInfo.Format = desc.Format;
pDevice->CheckFeatureSupport(D3D12_FEATURE_FORMAT_INFO, &formatInfo, sizeof(formatInfo));

planeCount = formatInfo.PlaneCount;
}

return _getLayerCount(&desc) * desc.MipLevels * planeCount;
}

static inline D3D12_PIPELINE_STATE_SUBOBJECT_TYPE
_getStateStreamType(char *pStream)
{
Expand Down
63 changes: 34 additions & 29 deletions wrappers/dxgitrace.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,14 @@ def implementWrapperInterfaceMethodBody(self, interface, base, method):
if method.name == 'GetDescriptorHandleIncrementSize':
print(' UINT _fake_result = _DescriptorIncrementSize;')

if method.name == 'Unmap':
result_name = ''

if method.name in ('GetCPUDescriptorHandleForHeapStart', 'GetGPUDescriptorHandleForHeapStart', 'GetGPUVirtualAddress', 'GetDescriptorHandleIncrementSize'):
result_name = '_fake_result'

if method.name in ('Map', 'Unmap', 'ExecuteCommandLists'):
print(' std::unique_lock<std::mutex> _ordering_lock{ g_D3D12AddressMappingsMutex };')
if method.name in ('Map', 'ExecuteCommandLists', 'CreateCommittedResource', 'CreateCommittedResource1', 'CreatePlacedResource', 'CreatePlacedResource1'):
print(' std::unique_lock<std::recursive_mutex> _ordering_lock{ g_D3D12AddressMappingsMutex };')

if method.name == 'ExecuteCommandLists':
print(' _flush_mappings();')
Expand Down Expand Up @@ -217,12 +220,12 @@ def implementWrapperInterfaceMethodBody(self, interface, base, method):
# If the current ref is 1, then the next one with be 0,
# therefore we need to unmap the resource here to
# avoid a dangling ptr.
print(' std::unique_lock<std::mutex> _ordering_lock;')
print(' std::unique_lock<std::recursive_mutex> _ordering_lock;')
print(' if (_current_ref == 1) {')
if interface in (d3d12.ID3D12Heap, d3d12.ID3D12Heap1):
print(' if (m_UserPointer != nullptr)')
print(' {')
print(' _ordering_lock = std::unique_lock<std::mutex>{ g_D3D12AddressMappingsMutex };')
print(' _ordering_lock = std::unique_lock<std::recursive_mutex>{ g_D3D12AddressMappingsMutex };')
print(' /* If Heap with a funny ptr, we should free here */')
print(' _fully_unmap_resource(m_pInstance);')
print(' }')
Expand Down Expand Up @@ -250,17 +253,14 @@ def implementWrapperInterfaceMethodBody(self, interface, base, method):
print(' MemoryShadow & _MapShadow = m_MapShadows[std::pair<%s, UINT>(pResource, Subresource)];' % resourceArg.type)
print(' Wrap%spResourceInstance = static_cast<Wrap%s>(%s);' % (resourceArg.type, resourceArg.type, resourceArg.name))

if method.name == 'Unmap':
if interface.name.startswith('ID3D12'):
print(' _unmap_resource(m_pInstance, Subresource);')
else:
print(' if (_MapDesc.Size && _MapDesc.pData) {')
print(' if (_shouldShadowMap(pResourceInstance)) {')
print(' _MapShadow.update(trace::fakeMemcpy);')
print(' } else {')
self.emit_memcpy('_MapDesc.pData', '_MapDesc.Size')
print(' }')
print(' }')
if method.name == 'Unmap' and not interface.name.startswith('ID3D12'):
print(' if (_MapDesc.Size && _MapDesc.pData) {')
print(' if (_shouldShadowMap(pResourceInstance)) {')
print(' _MapShadow.update(trace::fakeMemcpy);')
print(' } else {')
self.emit_memcpy('_MapDesc.pData', '_MapDesc.Size')
print(' }')
print(' }')

if interface.hasBase(d3d11.ID3D11VideoContext) and \
method.name == 'ReleaseDecoderBuffer':
Expand All @@ -278,7 +278,7 @@ def implementWrapperInterfaceMethodBody(self, interface, base, method):
if method.name == 'Map':
if interface.name.startswith('ID3D12'):
print(' if (SUCCEEDED(_result))')
print(' _map_resource(m_pInstance, Subresource, ppData ? *ppData : nullptr);')
print(' _map_subresource(m_pInstance, Subresource, ppData ? *ppData : nullptr);')
else:
# NOTE: recursive locks are explicitely forbidden
print(' if (SUCCEEDED(_result)) {')
Expand All @@ -301,7 +301,7 @@ def implementWrapperInterfaceMethodBody(self, interface, base, method):
print(' trace::fakeMalloc(pAddress, _d3d12_AllocationSize(pAddress));')
print(' trace::fakeMemcpy(pAddress, _d3d12_AllocationSize(pAddress));')
print(' auto *pHeap = *reinterpret_cast<WrapID3D12Heap**>(ppvHeap);')
print(' _map_resource(pHeap->m_pInstance, (void *)pAddress);')
print(' _map_heap(pHeap->m_pInstance, (void *)pAddress);')
print(' pHeap->m_UserPointer = pAddress;')
print(' }')

Expand All @@ -320,18 +320,23 @@ def implementWrapperInterfaceMethodBody(self, interface, base, method):
print(' _descriptor_heap_wrap->m_DescriptorSlab = g_D3D12DescriptorSlabs.RegisterSlab(_descriptor_heap_wrap->m_pInstance->GetCPUDescriptorHandleForHeapStart(), _descriptor_heap_wrap->m_pInstance->GetGPUDescriptorHandleForHeapStart(), m_pInstance->GetDescriptorHandleIncrementSize(pDescriptorHeapDesc->Type));')
print(' }')

if method.name in ('CreateCommittedResource', 'CreateCommittedResource1'):
# Create a fake GPU VA for buffers.
print(' if (SUCCEEDED(_result) && pResourceDesc->Dimension == D3D12_RESOURCE_DIMENSION_BUFFER) {')
print(' WrapID3D12Resource* _resource_wrap = (*reinterpret_cast<WrapID3D12Resource**>(ppvResource));')
print(' _resource_wrap->m_FakeAddress = g_D3D12AddressSlabs.RegisterSlab(_resource_wrap->m_pInstance->GetGPUVirtualAddress());')
print(' }')

if method.name in ('CreatePlacedResource', 'CreatePlacedResource1'):
if method.name in ('CreateCommittedResource', 'CreateCommittedResource1', 'CreatePlacedResource', 'CreatePlacedResource1'):
if method.name in ('CreatePlacedResource', 'CreatePlacedResource1'):
print(' const D3D12_RESOURCE_DESC *pResourceDesc = pDesc;')
# Create a fake GPU VA for buffers.
print(' if (SUCCEEDED(_result) && pDesc->Dimension == D3D12_RESOURCE_DIMENSION_BUFFER) {')
print(' WrapID3D12Resource* _resource_wrap = (*reinterpret_cast<WrapID3D12Resource**>(ppvResource));')
print(' _resource_wrap->m_FakeAddress = g_D3D12AddressSlabs.RegisterSlab(_resource_wrap->m_pInstance->GetGPUVirtualAddress());')
print(' if (SUCCEEDED(_result)) {')
print(' WrapID3D12Resource* _resource_wrap = *reinterpret_cast<WrapID3D12Resource**>(ppvResource);')
print(' if (pResourceDesc->Dimension == D3D12_RESOURCE_DIMENSION_BUFFER) {')
print(' _resource_wrap->m_FakeAddress = g_D3D12AddressSlabs.RegisterSlab(_resource_wrap->m_pInstance->GetGPUVirtualAddress());')
print(' }')
# Map ahead of time to avoid suballocation/zero-ing and stuff breaking our write watch
print(' if (_get_heap_flags(_resource_wrap->m_pInstance) & D3D12_HEAP_FLAG_ALLOW_WRITE_WATCH) {')
print(' UINT _subresource_count = _getSubresourceCount(_resource_wrap->m_pInstance);')
print(' for (UINT i = 0; i < _subresource_count; i++) {')
print(' void* _pData = NULL;')
print(' _resource_wrap->Map(i, NULL, &_pData);')
print(' }')
print(' }')
print(' }')

if method.name in ('CopyTileMappings', 'UpdateTileMappings'):
Expand Down Expand Up @@ -533,7 +538,7 @@ def invokeMethod(self, interface, base, method):
print('_D3D12_DESCRIPTOR_TRACER g_D3D12DescriptorSlabs;')
print('_D3D12_ADDRESS_SLAB_TRACER<D3D12_GPU_VIRTUAL_ADDRESS> g_D3D12AddressSlabs;')

print('std::mutex g_D3D12AddressMappingsMutex;')
print('std::recursive_mutex g_D3D12AddressMappingsMutex;')
print('std::map<SIZE_T, std::map<UINT, _D3D12_MAP_DESC>> g_D3D12AddressMappings;')

print('std::set<HANDLE> g_D3D12FenceEventSet;')
Expand Down

0 comments on commit 3208ff0

Please sign in to comment.