m138: [dawn][native] Ensure device lock is held for |CheckAndUpdateCompletedSerials|.

- Some backends do not implement a thread-safe
  ExecutionQueue::CheckAndUpdateCompletedSerials, as a result we need
  to make sure that we are still holding the device lock when calling it.

Bug: 420686217
Fixed: 423323683
Change-Id: I08bd6548e7e385bb4901072ffd536ee1b9b28500
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/244974
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Loko Kung <lokokung@google.com>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Auto-Submit: Loko Kung <lokokung@google.com>
(cherry picked from commit 0694294c3c9f72e6822cf44999abe6398c82b57f)
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/245954
Commit-Queue: Quyen Le <lehoangquyen@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

src/dawn/native/EventManager.cpp

diff --git a/src/dawn/native/EventManager.cpp b/src/dawn/native/EventManager.cpp
index 17717ac..4248a2a 100644
--- a/src/dawn/native/EventManager.cpp
+++ b/src/dawn/native/EventManager.cpp
@@ -178,21 +178,16 @@
                             DAWN_TRY_ASSIGN(waitResult,
                                             queue->WaitForQueueSerial(waitSerial, timeout));
                         }
+                        // Update completed serials.
+                        DAWN_TRY(queue->CheckPassedSerials());
                     }
                     return {};
                 }(),
                 "waiting for work in %s.", queue.Get());
         }
-
-        // Checking and updating serials cannot hold the device-wide lock because it may cause user
-        // callbacks to fire. As a result, we update the serials without the lock, and only
-        // reacquire the lock if there was an error.
-        auto maybeError = queue->CheckPassedSerials();
-        if (maybeError.IsError()) {
-            auto deviceLock(device->GetScopedLock());
-            [[maybe_unused]] bool error = device->ConsumedError(
-                std::move(maybeError), "updating passed serials in %s", queue.Get());
-        }
+        // TODO(crbug.com/421945313): Checking and updating serials cannot hold the device-wide lock
+        // because it may cause user callbacks to fire.
+        queue->UpdateCompletedSerial(queue->GetCompletedCommandSerial());
     }
 }
 

src/dawn/native/ExecutionQueue.cpp

diff --git a/src/dawn/native/ExecutionQueue.cpp b/src/dawn/native/ExecutionQueue.cpp
index 6fbb56a..1359724 100644
--- a/src/dawn/native/ExecutionQueue.cpp
+++ b/src/dawn/native/ExecutionQueue.cpp
@@ -51,7 +51,15 @@
     DAWN_ASSERT(completedSerial <=
                 ExecutionSerial(mLastSubmittedSerial.load(std::memory_order_acquire)));
 
-    UpdateCompletedSerial(completedSerial);
+    // Atomically set mCompletedSerial to completedSerial if completedSerial is larger.
+    uint64_t current = mCompletedSerial.load(std::memory_order_acquire);
+    while (uint64_t(completedSerial) > current &&
+           !mCompletedSerial.compare_exchange_weak(current, uint64_t(completedSerial),
+                                                   std::memory_order_acq_rel)) {
+    }
+    // TODO(crbug.com/421945313): We should call |UpdateCompletedSerial| here also, but since some
+    // backends rely on the device lock for safe use of |CheckAndUpdateCompletedSerials|, we
+    // separate that call out for now.
     return {};
 }
 

src/dawn/native/ExecutionQueue.h

diff --git a/src/dawn/native/ExecutionQueue.h b/src/dawn/native/ExecutionQueue.h
index c52908c..256a306 100644
--- a/src/dawn/native/ExecutionQueue.h
+++ b/src/dawn/native/ExecutionQueue.h
@@ -94,11 +94,11 @@
     // example, until the client has explictly issued a submission.
     enum class SubmitMode { Normal, Passive };
 
-  protected:
     // Currently, the queue has two paths for serial updating, one is via DeviceBase::Tick which
     // calls into the backend specific polling mechanisms implemented in
     // CheckAndUpdateCompletedSerials. Alternatively, the backend can actively call
     // UpdateCompletedSerial when a new serial is complete to make forward progress proactively.
+    // TODO(crbug.com/421945313): This shouldn't need to be public once we fix lock ordering.
     void UpdateCompletedSerial(ExecutionSerial completedSerial);
 
   private: