commit 6cbe0154290f124cf26ecc9627f4998f38c97f7c
author Sunny Sachanandani <sunnyps@chromium.org> Mon Jul 18 22:04:52 2022 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Jul 18 22:04:52 2022 +0000
tree 78dfa90e9523f023e087496855816a06970bbb2f
parent 4a92a3c904d6a4ce477b077281ce178305f551c1

d3d12: Fix fence signal race when no command lists are submitted

Signaling a fence in Dawn without waiting can cause a race between Dawn
and Chromium over who signals the fence. Since D3D12 fences are allowed
to be rewound this can cause future waits to hang indefinitely if Dawn
loses the race and signals a lower value than what Chromium expects.

The signal without wait case happens when Dawn submits no command lists
e.g. when CopyExternalImageToTexture uses a zero-sized dimension. This
is the reason for the test failures in https://crrev.com/c/3700811 e.g.
https://ci.chromium.org/ui/p/chromium/builders/try/dawn-win10-x64-deps-rel/24617/overview

To fix the race, it's sufficient to wait before signaling if not already
waited which is what this CL does. This CL also makes the fence stay
alive until pending signals are done - this wasn't the root cause of the
hang described above, but it's still a good idea for robustness, and
also recommended based on discussion around past Chromium XR fence use.

Bug: dawn:576
Change-Id: I8c1d1a19fdb022ae28d26f6723c2f2bfc9c1c3c7
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/96321
Auto-Submit: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>

src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp

diff --git a/src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp b/src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp
index b7838b2..1fdf7d6 100644
--- a/src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp
+++ b/src/dawn/native/d3d12/ExternalImageDXGIImpl.cpp
@@ -62,7 +62,12 @@
 void ExternalImageDXGIImpl::Destroy() {
     if (IsInList()) {
         RemoveFromList();
+
+        // Keep fence alive until any pending signal calls are done on the GPU.
+        mBackendDevice->ConsumedError(mBackendDevice->NextSerial());
+        mBackendDevice->ReferenceUntilUnused(mD3D12Fence.Get());
         mBackendDevice = nullptr;
+
         mD3D12Resource.Reset();
         mD3D12Fence.Reset();
         mD3D11on12ResourceCache.reset();

src/dawn/native/d3d12/TextureD3D12.cpp

diff --git a/src/dawn/native/d3d12/TextureD3D12.cpp b/src/dawn/native/d3d12/TextureD3D12.cpp
index d547920..cd17ada1 100644
--- a/src/dawn/native/d3d12/TextureD3D12.cpp
+++ b/src/dawn/native/d3d12/TextureD3D12.cpp
@@ -675,7 +675,15 @@
 
     // Signal the fence on destroy after all uses of the texture.
     if (mD3D12Fence != nullptr && mFenceSignalValue != 0) {
-        device->GetCommandQueue()->Signal(mD3D12Fence.Get(), mFenceSignalValue);
+        // Enqueue a fence wait if we haven't already; otherwise the fence signal will be racy.
+        if (mFenceWaitValue != UINT64_MAX) {
+            device->ConsumedError(
+                CheckHRESULT(device->GetCommandQueue()->Wait(mD3D12Fence.Get(), mFenceWaitValue),
+                             "D3D12 fence wait"));
+        }
+        device->ConsumedError(
+            CheckHRESULT(device->GetCommandQueue()->Signal(mD3D12Fence.Get(), mFenceSignalValue),
+                         "D3D12 fence signal"));
     }
 
     // Now that the texture has been destroyed. It should release the refptr of the d3d11on12