Avoid deadlock in EventThread::onNewVsyncSchedule
EventThread::onNewVsyncSchedule attempts to lock mMutex, but it is
currently called while Scheduler::mDisplayLock is held. But
shouldConsumeEvent is called while mMutex is locked, and
shouldConsumeEvent's call to mThrottleVsyncCallback results calls
Scheduler::isVsyncValid, which then calls multiple methods that require
locking mDisplayLock, resulting in deadlock.
MessageQueue::onNewVsyncSchedule runs into a similar problem. It is also
called while mDisplayLock is held.
Split up the calls to onNewVsyncSchedule into a separate method, which
is called after mDisplayLock is released. Create
promotePacesetterDisplayLocked, which returns the new pacesetter's
VsyncSchedule so it can be applied afterwards. This avoids the deadlock.
In addition, handle the connections the same as other calls: lock
mConnectionsLock, store a pointer to the EventThread, and call the
method after releasing the lock. This is safe because Scheduler never
deletes its EventThreads (until its own teardown).
Bug: 272943830
Test: manual (see b/272234280)
Change-Id: Ieb05fc3abcf4fc20f04ea5d3595da70155de2df5
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 6e33272..3e12db6 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() {
@@ -679,6 +686,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());
@@ -698,14 +717,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.