From 3194b4a31f651921dc211f5e2d3822f6d04641fc Mon Sep 17 00:00:00 2001 From: Sunny Sachanandani Date: Wed, 28 Sep 2022 00:58:46 +0000 Subject: [PATCH] d3d12: Signal fence after PIX Present Calling ID3D12SharingContract::Present issues GPU work on the command queue which needs to be synchronized with resource deallocation. This seems to work now perhaps due to the keyed mutex semantics keeping the D3D11 texture alive for longer than necessary. With fences, this missing synchronization causes the validation layers to complain about early deallocation of the ID3D12Resource. Moving the Present to SynchronizeImportTextureBeforeUse ensures that it happens before NextSerial and hence the signal fence that's recorded will include any GPU work issued by Present. Also, resource deallocation will happen after this work. However, this has the side-effect of PIX seeing more frames, once per ExecuteCommandLists, but it could be argued that's more accurate and useful. Bug: dawn:1544 Change-Id: I1b417049045a812837f67072d7f09ac47bc18125 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/103841 Reviewed-by: Austin Eng Kokoro: Kokoro Commit-Queue: Sunny Sachanandani --- src/dawn/native/d3d12/TextureD3D12.cpp | 47 ++++++++++++-------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/dawn/native/d3d12/TextureD3D12.cpp b/src/dawn/native/d3d12/TextureD3D12.cpp index f17e995cdb..30b852d45f 100644 --- a/src/dawn/native/d3d12/TextureD3D12.cpp +++ b/src/dawn/native/d3d12/TextureD3D12.cpp @@ -656,53 +656,37 @@ Texture::~Texture() {} void Texture::DestroyImpl() { TextureBase::DestroyImpl(); - Device* device = ToBackend(GetDevice()); + ToBackend(GetDevice())->DeallocateMemory(mResourceAllocation); - // In PIX's D3D12-only mode, there is no way to determine frame boundaries - // for WebGPU since Dawn does not manage DXGI swap chains. Without assistance, - // PIX will wait forever for a present that never happens. - // If we know we're dealing with a swapbuffer texture, inform PIX we've - // "presented" the texture so it can determine frame boundaries and use its - // contents for the UI. - if (mSwapChainTexture) { - ID3D12SharingContract* d3dSharingContract = device->GetSharingContract(); - if (d3dSharingContract != nullptr) { - d3dSharingContract->Present(mResourceAllocation.GetD3D12Resource(), 0, 0); - } - } - - device->DeallocateMemory(mResourceAllocation); - - // Now that we've deallocated the memory, the texture is no longer a swap chain texture. - // We can set mSwapChainTexture to false to avoid passing a nullptr to - // ID3D12SharingContract::Present. + // Set mSwapChainTexture to false to prevent ever calling ID3D12SharingContract::Present again. mSwapChainTexture = false; - // Now that the texture has been destroyed. It should release the refptr of the d3d11on12 - // resource and the fence. + // Now that the texture has been destroyed, it should release the d3d11on12 resource refptr. mD3D11on12Resource = nullptr; } ResultOrError Texture::EndAccess() { ASSERT(mD3D11on12Resource == nullptr); + Device* device = ToBackend(GetDevice()); + // Synchronize if texture access wasn't synchronized already due to ExecuteCommandLists. if (!mSignalFenceValue.has_value()) { // Needed to ensure that command allocator doesn't get destroyed before pending commands // are submitted due to calling NextSerial(). No-op if there are no pending commands. - DAWN_TRY(ToBackend(GetDevice())->ExecutePendingCommandContext()); + DAWN_TRY(device->ExecutePendingCommandContext()); // If there were pending commands that used this texture mSignalFenceValue will be set, // but if it's still not set, generate a signal fence after waiting on wait fences. if (!mSignalFenceValue.has_value()) { DAWN_TRY(SynchronizeImportedTextureBeforeUse()); DAWN_TRY(SynchronizeImportedTextureAfterUse()); } - DAWN_TRY(ToBackend(GetDevice())->NextSerial()); + DAWN_TRY(device->NextSerial()); ASSERT(mSignalFenceValue.has_value()); } ExecutionSerial ret = mSignalFenceValue.value(); - ASSERT(ret <= GetDevice()->GetLastSubmittedCommandSerial()); + ASSERT(ret <= device->GetLastSubmittedCommandSerial()); // Explicitly call reset() since std::move() on optional doesn't make it std::nullopt. mSignalFenceValue.reset(); return ret; @@ -757,11 +741,24 @@ MaybeError Texture::SynchronizeImportedTextureBeforeUse() { } MaybeError Texture::SynchronizeImportedTextureAfterUse() { + // In PIX's D3D12-only mode, there is no way to determine frame boundaries + // for WebGPU since Dawn does not manage DXGI swap chains. Without assistance, + // PIX will wait forever for a present that never happens. + // If we know we're dealing with a swapbuffer texture, inform PIX we've + // "presented" the texture so it can determine frame boundaries and use its + // contents for the UI. + Device* device = ToBackend(GetDevice()); + if (mSwapChainTexture) { + ID3D12SharingContract* d3dSharingContract = device->GetSharingContract(); + if (d3dSharingContract != nullptr) { + d3dSharingContract->Present(mResourceAllocation.GetD3D12Resource(), 0, 0); + } + } if (mD3D11on12Resource != nullptr) { DAWN_TRY(mD3D11on12Resource->ReleaseKeyedMutex()); } else { // NextSerial() will be called after this - this is also checked in EndAccess(). - mSignalFenceValue = ToBackend(GetDevice())->GetPendingCommandSerial(); + mSignalFenceValue = device->GetPendingCommandSerial(); } return {}; }