Only call onNewVsyncSchedule if the pacesetter changes
When re-calculating the pacesetter display, we may end up with the same
display, and therefore the same VsyncSchedule. In that case, we don't
need to apply the new one.
Bug: 276367387
Test: atest libsurfaceflinger_unittest:SchedulerTest
Change-Id: I76750ffdba6c3d790a98b65aabbc8b13eab3b4ae
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 3e12db6..8ddcfa1 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -130,7 +130,7 @@
pacesetterVsyncSchedule = promotePacesetterDisplayLocked();
}
- applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
+ applyNewVsyncScheduleIfNonNull(std::move(pacesetterVsyncSchedule));
}
void Scheduler::unregisterDisplay(PhysicalDisplayId displayId) {
@@ -149,7 +149,7 @@
pacesetterVsyncSchedule = promotePacesetterDisplayLocked();
}
- applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
+ applyNewVsyncScheduleIfNonNull(std::move(pacesetterVsyncSchedule));
}
void Scheduler::run() {
@@ -693,16 +693,17 @@
pacesetterVsyncSchedule = promotePacesetterDisplayLocked(pacesetterIdOpt);
}
- applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
+ applyNewVsyncScheduleIfNonNull(std::move(pacesetterVsyncSchedule));
}
std::shared_ptr<VsyncSchedule> Scheduler::promotePacesetterDisplayLocked(
std::optional<PhysicalDisplayId> pacesetterIdOpt) {
// TODO(b/241286431): Choose the pacesetter display.
+ const auto oldPacesetterDisplayIdOpt = mPacesetterDisplayId;
mPacesetterDisplayId = pacesetterIdOpt.value_or(mRefreshRateSelectors.begin()->first);
ALOGI("Display %s is the pacesetter", to_string(*mPacesetterDisplayId).c_str());
- auto vsyncSchedule = getVsyncScheduleLocked(*mPacesetterDisplayId);
+ auto newVsyncSchedule = getVsyncScheduleLocked(*mPacesetterDisplayId);
if (const auto pacesetterPtr = pacesetterSelectorPtrLocked()) {
pacesetterPtr->setIdleTimerCallbacks(
{.platform = {.onReset = [this] { idleTimerCallback(TimerState::Reset); },
@@ -713,15 +714,28 @@
pacesetterPtr->startIdleTimer();
+ // Track the new period, which may have changed due to switching to a
+ // new pacesetter or due to a hotplug event. In the former case, this
+ // is important so that VSYNC modulation does not get stuck in the
+ // initiated state if a transition started on the old pacesetter.
const Fps refreshRate = pacesetterPtr->getActiveMode().modePtr->getFps();
- vsyncSchedule->startPeriodTransition(mSchedulerCallback, refreshRate.getPeriod(),
- true /* force */);
+ newVsyncSchedule->startPeriodTransition(mSchedulerCallback, refreshRate.getPeriod(),
+ true /* force */);
}
- return vsyncSchedule;
+ if (oldPacesetterDisplayIdOpt == mPacesetterDisplayId) {
+ return nullptr;
+ }
+ return newVsyncSchedule;
}
-void Scheduler::applyNewVsyncSchedule(std::shared_ptr<VsyncSchedule> vsyncSchedule) {
- onNewVsyncSchedule(vsyncSchedule->getDispatch());
+void Scheduler::applyNewVsyncScheduleIfNonNull(
+ std::shared_ptr<VsyncSchedule> pacesetterSchedulePtr) {
+ if (!pacesetterSchedulePtr) {
+ // The pacesetter has not changed, so there is no new VsyncSchedule to
+ // apply.
+ return;
+ }
+ onNewVsyncSchedule(pacesetterSchedulePtr->getDispatch());
std::vector<android::EventThread*> threads;
{
std::lock_guard<std::mutex> lock(mConnectionsLock);
@@ -731,7 +745,7 @@
}
}
for (auto* thread : threads) {
- thread->onNewVsyncSchedule(vsyncSchedule);
+ thread->onNewVsyncSchedule(pacesetterSchedulePtr);
}
}
diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 3423652..720a1cb 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -329,10 +329,12 @@
// 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.
+ // Returns the new pacesetter's VsyncSchedule, or null if the pacesetter is
+ // unchanged.
std::shared_ptr<VsyncSchedule> promotePacesetterDisplayLocked(
std::optional<PhysicalDisplayId> pacesetterIdOpt = std::nullopt)
REQUIRES(kMainThreadContext, mDisplayLock);
- void applyNewVsyncSchedule(std::shared_ptr<VsyncSchedule>) EXCLUDES(mDisplayLock);
+ void applyNewVsyncScheduleIfNonNull(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.
diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
index dc76b4c..0c43831 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -384,4 +384,23 @@
}
}
+TEST_F(SchedulerTest, changingPacesetterChangesVsyncSchedule) {
+ // Add a second display so we can change the pacesetter.
+ mScheduler->registerDisplay(kDisplayId2,
+ std::make_shared<RefreshRateSelector>(kDisplay2Modes,
+ kDisplay2Mode60->getId()));
+ // Ensure that the pacesetter is the one we expect.
+ mScheduler->setPacesetterDisplay(kDisplayId1);
+
+ // Switching to the other will call onNewVsyncSchedule.
+ EXPECT_CALL(*mEventThread, onNewVsyncSchedule(mScheduler->getVsyncSchedule(kDisplayId2)))
+ .Times(1);
+ mScheduler->setPacesetterDisplay(kDisplayId2);
+}
+
+TEST_F(SchedulerTest, promotingSamePacesetterDoesNotChangeVsyncSchedule) {
+ EXPECT_CALL(*mEventThread, onNewVsyncSchedule(_)).Times(0);
+ mScheduler->setPacesetterDisplay(kDisplayId1);
+}
+
} // namespace android::scheduler