Merge "Avoid deadlock in EventThread::onNewVsyncSchedule" into udc-dev am: e6cfeaad1f
Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/native/+/22125708
Change-Id: I07fd24acef2e3aa12fed3c3906465ac54c9f465c
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index f18dfdc..7f8d394 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -109,7 +109,6 @@
void Scheduler::setPacesetterDisplay(std::optional<PhysicalDisplayId> pacesetterIdOpt) {
demotePacesetterDisplay();
- std::scoped_lock lock(mDisplayLock);
promotePacesetterDisplay(pacesetterIdOpt);
}
@@ -123,26 +122,34 @@
std::shared_ptr<VsyncSchedule> vsyncSchedule) {
demotePacesetterDisplay();
- std::scoped_lock lock(mDisplayLock);
- mRefreshRateSelectors.emplace_or_replace(displayId, std::move(selectorPtr));
- mVsyncSchedules.emplace_or_replace(displayId, std::move(vsyncSchedule));
+ std::shared_ptr<VsyncSchedule> pacesetterVsyncSchedule;
+ {
+ std::scoped_lock lock(mDisplayLock);
+ mRefreshRateSelectors.emplace_or_replace(displayId, std::move(selectorPtr));
+ mVsyncSchedules.emplace_or_replace(displayId, std::move(vsyncSchedule));
- promotePacesetterDisplay();
+ pacesetterVsyncSchedule = promotePacesetterDisplayLocked();
+ }
+ applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
}
void Scheduler::unregisterDisplay(PhysicalDisplayId displayId) {
demotePacesetterDisplay();
- std::scoped_lock lock(mDisplayLock);
- mRefreshRateSelectors.erase(displayId);
- mVsyncSchedules.erase(displayId);
+ std::shared_ptr<VsyncSchedule> pacesetterVsyncSchedule;
+ {
+ std::scoped_lock lock(mDisplayLock);
+ mRefreshRateSelectors.erase(displayId);
+ mVsyncSchedules.erase(displayId);
- // Do not allow removing the final display. Code in the scheduler expects
- // there to be at least one display. (This may be relaxed in the future with
- // headless virtual display.)
- LOG_ALWAYS_FATAL_IF(mRefreshRateSelectors.empty(), "Cannot unregister all displays!");
+ // Do not allow removing the final display. Code in the scheduler expects
+ // there to be at least one display. (This may be relaxed in the future with
+ // headless virtual display.)
+ LOG_ALWAYS_FATAL_IF(mRefreshRateSelectors.empty(), "Cannot unregister all displays!");
- promotePacesetterDisplay();
+ pacesetterVsyncSchedule = promotePacesetterDisplayLocked();
+ }
+ applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
}
void Scheduler::run() {
@@ -678,6 +685,18 @@
}
void Scheduler::promotePacesetterDisplay(std::optional<PhysicalDisplayId> pacesetterIdOpt) {
+ std::shared_ptr<VsyncSchedule> pacesetterVsyncSchedule;
+
+ {
+ std::scoped_lock lock(mDisplayLock);
+ pacesetterVsyncSchedule = promotePacesetterDisplayLocked(pacesetterIdOpt);
+ }
+
+ applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
+}
+
+std::shared_ptr<VsyncSchedule> Scheduler::promotePacesetterDisplayLocked(
+ std::optional<PhysicalDisplayId> pacesetterIdOpt) {
// TODO(b/241286431): Choose the pacesetter display.
mPacesetterDisplayId = pacesetterIdOpt.value_or(mRefreshRateSelectors.begin()->first);
ALOGI("Display %s is the pacesetter", to_string(*mPacesetterDisplayId).c_str());
@@ -697,14 +716,22 @@
vsyncSchedule->startPeriodTransition(mSchedulerCallback, refreshRate.getPeriod(),
true /* force */);
}
+ return vsyncSchedule;
+}
+void Scheduler::applyNewVsyncSchedule(std::shared_ptr<VsyncSchedule> vsyncSchedule) {
onNewVsyncSchedule(vsyncSchedule->getDispatch());
+ std::vector<android::EventThread*> threads;
{
std::lock_guard<std::mutex> lock(mConnectionsLock);
+ threads.reserve(mConnections.size());
for (auto& [_, connection] : mConnections) {
- connection.thread->onNewVsyncSchedule(vsyncSchedule);
+ threads.push_back(connection.thread.get());
}
}
+ for (auto* thread : threads) {
+ thread->onNewVsyncSchedule(vsyncSchedule);
+ }
}
void Scheduler::demotePacesetterDisplay() {
diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 74547d5..3423652 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -321,7 +321,18 @@
// Chooses a pacesetter among the registered displays, unless `pacesetterIdOpt` is specified.
// The new `mPacesetterDisplayId` is never `std::nullopt`.
void promotePacesetterDisplay(std::optional<PhysicalDisplayId> pacesetterIdOpt = std::nullopt)
+ REQUIRES(kMainThreadContext) EXCLUDES(mDisplayLock);
+
+ // Changes to the displays (e.g. registering and unregistering) must be made
+ // while mDisplayLock is locked, and the new pacesetter then must be promoted while
+ // mDisplayLock is still locked. However, a new pacesetter means that
+ // MessageQueue and EventThread need to use the new pacesetter's
+ // VsyncSchedule, and this must happen while mDisplayLock is *not* locked,
+ // or else we may deadlock with EventThread.
+ std::shared_ptr<VsyncSchedule> promotePacesetterDisplayLocked(
+ std::optional<PhysicalDisplayId> pacesetterIdOpt = std::nullopt)
REQUIRES(kMainThreadContext, mDisplayLock);
+ void applyNewVsyncSchedule(std::shared_ptr<VsyncSchedule>) EXCLUDES(mDisplayLock);
// Blocks until the pacesetter's idle timer thread exits. `mDisplayLock` must not be locked by
// the caller on the main thread to avoid deadlock, since the timer thread locks it before exit.