Revert "Only call onNewVsyncSchedule if the pacesetter changes"
This reverts commit d649463aa27ae1aea7101afc4684cc9422d4808a.
Reason for revert: Speculative fix for a regression in SF performance.
Bug: 278987666
Test: APC
Change-Id: I5bf393a2eef4dc7254ccf931a0fab3c67eb315ad
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 8ddcfa1..3e12db6 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -130,7 +130,7 @@
pacesetterVsyncSchedule = promotePacesetterDisplayLocked();
}
- applyNewVsyncScheduleIfNonNull(std::move(pacesetterVsyncSchedule));
+ applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
}
void Scheduler::unregisterDisplay(PhysicalDisplayId displayId) {
@@ -149,7 +149,7 @@
pacesetterVsyncSchedule = promotePacesetterDisplayLocked();
}
- applyNewVsyncScheduleIfNonNull(std::move(pacesetterVsyncSchedule));
+ applyNewVsyncSchedule(std::move(pacesetterVsyncSchedule));
}
void Scheduler::run() {
@@ -693,17 +693,16 @@
pacesetterVsyncSchedule = promotePacesetterDisplayLocked(pacesetterIdOpt);
}
- applyNewVsyncScheduleIfNonNull(std::move(pacesetterVsyncSchedule));
+ applyNewVsyncSchedule(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 newVsyncSchedule = getVsyncScheduleLocked(*mPacesetterDisplayId);
+ auto vsyncSchedule = getVsyncScheduleLocked(*mPacesetterDisplayId);
if (const auto pacesetterPtr = pacesetterSelectorPtrLocked()) {
pacesetterPtr->setIdleTimerCallbacks(
{.platform = {.onReset = [this] { idleTimerCallback(TimerState::Reset); },
@@ -714,28 +713,15 @@
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();
- newVsyncSchedule->startPeriodTransition(mSchedulerCallback, refreshRate.getPeriod(),
- true /* force */);
+ vsyncSchedule->startPeriodTransition(mSchedulerCallback, refreshRate.getPeriod(),
+ true /* force */);
}
- if (oldPacesetterDisplayIdOpt == mPacesetterDisplayId) {
- return nullptr;
- }
- return newVsyncSchedule;
+ return vsyncSchedule;
}
-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());
+void Scheduler::applyNewVsyncSchedule(std::shared_ptr<VsyncSchedule> vsyncSchedule) {
+ onNewVsyncSchedule(vsyncSchedule->getDispatch());
std::vector<android::EventThread*> threads;
{
std::lock_guard<std::mutex> lock(mConnectionsLock);
@@ -745,7 +731,7 @@
}
}
for (auto* thread : threads) {
- thread->onNewVsyncSchedule(pacesetterSchedulePtr);
+ thread->onNewVsyncSchedule(vsyncSchedule);
}
}
diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 720a1cb..3423652 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -329,12 +329,10 @@
// 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 applyNewVsyncScheduleIfNonNull(std::shared_ptr<VsyncSchedule>) EXCLUDES(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.
diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
index 0c43831..dc76b4c 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -384,23 +384,4 @@
}
}
-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