[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
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>
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());
}
}
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 {};
}
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:
