Revert "[native][vulkan] Remove device guard for CheckAndUpdateCompletedSerials." This reverts commit b4ea001870c1e05e4588470e6c8de7f6d17c8845. Reason for revert: Possible cause of dawn OOM in Skia tests Failure Link: <LINK TO FAILURE> Original change's description: > [native][vulkan] Remove device guard for CheckAndUpdateCompletedSerials. > > - Unifies the handling in ExecutionQueue for WaitIsThreadSafe toggle to > create and hold the device guard appropriately for this case also. I > think it might be possible to eventually remove the need to eagerly > update the serial value, but right now it seems like it's necessary > for the different backends' fence handing. > > Bug: 496272707 > Change-Id: Iba0708e4ee292e905131fa2ab0d796e9a6392aa9 > Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/352175 > Commit-Queue: Corentin Wallez <cwallez@chromium.org> > Reviewed-by: Corentin Wallez <cwallez@chromium.org> > Auto-Submit: Loko Kung <lokokung@google.com> # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 496272707 Change-Id: I3d3a5b71b64063f4945bd7142b30bbca3d0cf7cf Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/353615 Reviewed-by: Ran Wang <wangra@google.com> Reviewed-by: Brandon Jones <bajones@chromium.org> Reviewed-by: Ken Russell <kbr@google.com> Auto-Submit: Greg Daniel <egdaniel@google.com> Commit-Queue: Brandon Jones <bajones@chromium.org>
diff --git a/src/dawn/native/ExecutionQueue.cpp b/src/dawn/native/ExecutionQueue.cpp index fd6efa9..8aca9b6 100644 --- a/src/dawn/native/ExecutionQueue.cpp +++ b/src/dawn/native/ExecutionQueue.cpp
@@ -29,7 +29,6 @@ #include <algorithm> #include <atomic> -#include <optional> #include <utility> #include <vector> @@ -117,33 +116,46 @@ } } - // If the backend does not support thread-safe waits, we need to acquire the device lock first. - // Note that the main blocking backend is D3D11 which is using the value of |mCompletedSerial| - // within its implementation of |CheckAndUpdateCompletedSerials|. - std::optional<DeviceGuard> deviceGuard; - if (!GetDevice()->IsToggleEnabled(Toggle::WaitIsThreadSafe)) { - deviceGuard.emplace(GetDevice()->GetGuard()); + // We currently have two differing implementations for this function depending on whether the + // backend supports thread safe waits. Note that while currently only the Metal backend + // explicitly enables thread safe wait, the main blocking backend is D3D11 which is using the + // value of |mCompletedSerial| within it's implementation of |CheckAndUpdateCompletedSerials|. + if (GetDevice()->IsToggleEnabled(Toggle::WaitIsThreadSafe)) { + if (timeout > Nanoseconds(0u)) { + // We should never need to wait for queue serials after the device has been + // Disconnected. + DAWN_ASSERT(GetDevice()->GetState() != DeviceBase::State::Disconnected); + + // Wait on the serial if it hasn't passed yet. + ExecutionSerial completedSerial = kWaitSerialTimeout; + DAWN_TRY_ASSIGN(completedSerial, WaitForQueueSerialImpl(waitSerial, timeout)); + UpdateCompletedSerialTo(QueuePriority::UserVisible, completedSerial); + return {}; + } + return UpdateCompletedSerial(QueuePriority::UserVisible); + } else { + // Otherwise, we need to acquire the device lock first. + auto deviceGuard = GetDevice()->GetGuard(); + if (timeout > Nanoseconds(0u)) { + // We should never need to wait for queue serials after the device has been + // Disconnected. + DAWN_ASSERT(GetDevice()->GetState() != DeviceBase::State::Disconnected); + + // Wait on the serial if it hasn't passed yet. + ExecutionSerial completedSerial = kWaitSerialTimeout; + DAWN_TRY_ASSIGN(completedSerial, WaitForQueueSerialImpl(waitSerial, timeout)); + + // It's critical to update the completed serial right away. If fences are processed + // by another thread before CheckAndUpdateCompletedSerials() runs on the current + // thread, the fence list will be empty, preventing the current thread from + // determining the true latest serial. Preemptively updating mCompletedSerial + // ensures CheckAndUpdateCompletedSerials() returns an accurate value, preventing + // stale data. + mCompletedSerial.Use( + [&](auto old) { *old = std::max(*old, static_cast<uint64_t>(completedSerial)); }); + } + return UpdateCompletedSerial(QueuePriority::UserVisible); } - - if (timeout > Nanoseconds(0u)) { - // We should never need to wait for queue serials after the device has been - // Disconnected. - DAWN_ASSERT(GetDevice()->GetState() != DeviceBase::State::Disconnected); - - // Wait on the serial if it hasn't passed yet. - ExecutionSerial completedSerial = kWaitSerialTimeout; - DAWN_TRY_ASSIGN(completedSerial, WaitForQueueSerialImpl(waitSerial, timeout)); - - // It's critical to update the completed serial right away. If fences are processed - // by another thread before CheckAndUpdateCompletedSerials() runs on the current - // thread, the fence list will be empty, preventing the current thread from - // determining the true latest serial. Preemptively updating mCompletedSerial - // ensures CheckAndUpdateCompletedSerials() returns an accurate value, preventing - // stale data. - mCompletedSerial.Use( - [&](auto old) { *old = std::max(*old, static_cast<uint64_t>(completedSerial)); }); - } - return UpdateCompletedSerial(QueuePriority::UserVisible); } void ExecutionQueueBase::WaitForIdleForDestruction() {
diff --git a/src/dawn/native/vulkan/QueueVk.cpp b/src/dawn/native/vulkan/QueueVk.cpp index 68e730c..e396d87 100644 --- a/src/dawn/native/vulkan/QueueVk.cpp +++ b/src/dawn/native/vulkan/QueueVk.cpp
@@ -130,6 +130,9 @@ } ResultOrError<ExecutionSerial> Queue::CheckAndUpdateCompletedSerials() { + // TODO(crbug.com/40643114): Revisit whether this lock is needed for this backend. + auto deviceGuard = GetDevice()->GetGuard(); + Device* device = ToBackend(GetDevice()); return mFencesInFlight.Use([&](auto fencesInFlight) -> ResultOrError<ExecutionSerial> { ExecutionSerial fenceSerial(0u);