SF: Split Scheduler::setRefreshRateSelector

Add helper functions to bind/unbind the idle timer.

Bug: 255635821
Test: Build (-Wthread-safety)
Change-Id: I68cd1274e2b0591652a259b7f60d0a370883e512

5 files changed, 43 insertions, 24 deletions

diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 0e1b775a8c..6e34a68fff 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -94,36 +94,24 @@ void Scheduler::startTimers() {
     }
 }
 
-void Scheduler::setRefreshRateSelector(RefreshRateSelectorPtr selectorPtr) {
-    // The current RefreshRateSelector instance may outlive this call, so unbind its idle timer.
-    {
-        // mRefreshRateSelectorLock is not locked here to avoid the deadlock
-        // as the callback can attempt to acquire the lock before stopIdleTimer can finish
-        // the execution. It's safe to FakeGuard as main thread is the only thread that
-        // writes to the mRefreshRateSelector.
-        ftl::FakeGuard guard(mRefreshRateSelectorLock);
-        if (mRefreshRateSelector) {
-            mRefreshRateSelector->stopIdleTimer();
-            mRefreshRateSelector->clearIdleTimerCallbacks();
-        }
+void Scheduler::setRefreshRateSelector(RefreshRateSelectorPtr newSelectorPtr) {
+    // No need to lock for reads on kMainThreadContext.
+    if (const auto& selectorPtr = FTL_FAKE_GUARD(mRefreshRateSelectorLock, mRefreshRateSelector)) {
+        unbindIdleTimer(*selectorPtr);
     }
+
     {
-        // Clear state that depends on the current instance.
+        // Clear state that depends on the current RefreshRateSelector.
         std::scoped_lock lock(mPolicyLock);
         mPolicy = {};
     }
 
     std::scoped_lock lock(mRefreshRateSelectorLock);
-    mRefreshRateSelector = std::move(selectorPtr);
-    if (!mRefreshRateSelector) return;
+    mRefreshRateSelector = std::move(newSelectorPtr);
 
-    mRefreshRateSelector->setIdleTimerCallbacks(
-            {.platform = {.onReset = [this] { idleTimerCallback(TimerState::Reset); },
-                          .onExpired = [this] { idleTimerCallback(TimerState::Expired); }},
-             .kernel = {.onReset = [this] { kernelIdleTimerCallback(TimerState::Reset); },
-                        .onExpired = [this] { kernelIdleTimerCallback(TimerState::Expired); }}});
-
-    mRefreshRateSelector->startIdleTimer();
+    if (mRefreshRateSelector) {
+        bindIdleTimer(*mRefreshRateSelector);
+    }
 }
 
 void Scheduler::registerDisplay(PhysicalDisplayId displayId, RefreshRateSelectorPtr selectorPtr) {
@@ -546,6 +534,21 @@ void Scheduler::setDisplayPowerMode(hal::PowerMode powerMode) {
     mLayerHistory.clear();
 }
 
+void Scheduler::bindIdleTimer(RefreshRateSelector& selector) {
+    selector.setIdleTimerCallbacks(
+            {.platform = {.onReset = [this] { idleTimerCallback(TimerState::Reset); },
+                          .onExpired = [this] { idleTimerCallback(TimerState::Expired); }},
+             .kernel = {.onReset = [this] { kernelIdleTimerCallback(TimerState::Reset); },
+                        .onExpired = [this] { kernelIdleTimerCallback(TimerState::Expired); }}});
+
+    selector.startIdleTimer();
+}
+
+void Scheduler::unbindIdleTimer(RefreshRateSelector& selector) {
+    selector.stopIdleTimer();
+    selector.clearIdleTimerCallbacks();
+}
+
 void Scheduler::kernelIdleTimerCallback(TimerState state) {
     ATRACE_INT("ExpiredKernelIdleTimer", static_cast<int>(state));
 
diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 901cf74558..797ec96d0b 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -108,7 +108,8 @@ public:
     void startTimers();
 
     using RefreshRateSelectorPtr = std::shared_ptr<RefreshRateSelector>;
-    void setRefreshRateSelector(RefreshRateSelectorPtr) EXCLUDES(mRefreshRateSelectorLock);
+    void setRefreshRateSelector(RefreshRateSelectorPtr) REQUIRES(kMainThreadContext)
+            EXCLUDES(mRefreshRateSelectorLock);
 
     void registerDisplay(PhysicalDisplayId, RefreshRateSelectorPtr);
     void unregisterDisplay(PhysicalDisplayId);
@@ -253,6 +254,13 @@ private:
     sp<EventThreadConnection> createConnectionInternal(
             EventThread*, EventRegistrationFlags eventRegistration = {});
 
+    void bindIdleTimer(RefreshRateSelector&) REQUIRES(kMainThreadContext, mRefreshRateSelectorLock);
+
+    // Blocks until the timer thread exits. `mRefreshRateSelectorLock` must not be locked by the
+    // caller on the main thread to avoid deadlock, since the timer thread locks it before exit.
+    static void unbindIdleTimer(RefreshRateSelector&) REQUIRES(kMainThreadContext)
+            EXCLUDES(mRefreshRateSelectorLock);
+
     // Update feature state machine to given state when corresponding timer resets or expires.
     void kernelIdleTimerCallback(TimerState) EXCLUDES(mRefreshRateSelectorLock);
     void idleTimerCallback(TimerState);
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 53066024d0..864245ad48 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2941,6 +2941,8 @@ void SurfaceFlinger::processDisplayAdded(const wp<IBinder>& displayToken,
 
         // Display modes are reloaded on hotplug reconnect.
         if (display->isPrimary()) {
+            // TODO(b/241285876): Annotate `processDisplayAdded` instead.
+            ftl::FakeGuard guard(kMainThreadContext);
             mScheduler->setRefreshRateSelector(selectorPtr);
         }
 
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index d2907626e0..897f4a3701 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -694,7 +694,7 @@ private:
     void commitInputWindowCommands() REQUIRES(mStateLock);
     void updateCursorAsync();
 
-    void initScheduler(const sp<const DisplayDevice>&) REQUIRES(mStateLock);
+    void initScheduler(const sp<const DisplayDevice>&) REQUIRES(kMainThreadContext, mStateLock);
     void updatePhaseConfiguration(const Fps&) REQUIRES(mStateLock);
     void setVsyncConfig(const VsyncModulator::VsyncConfig&, nsecs_t vsyncPeriod);
 
diff --git a/services/surfaceflinger/tests/unittests/TestableScheduler.h b/services/surfaceflinger/tests/unittests/TestableScheduler.h
index 95c99150b3..ba214d534f 100644
--- a/services/surfaceflinger/tests/unittests/TestableScheduler.h
+++ b/services/surfaceflinger/tests/unittests/TestableScheduler.h
@@ -17,6 +17,7 @@
 #pragma once
 
 #include <Scheduler/Scheduler.h>
+#include <ftl/fake_guard.h>
 #include <gmock/gmock.h>
 #include <gui/ISurfaceComposer.h>
 
@@ -69,6 +70,11 @@ public:
     auto refreshRateSelector() { return holdRefreshRateSelector(); }
     bool hasRefreshRateSelectors() const { return !mRefreshRateSelectors.empty(); }
 
+    void setRefreshRateSelector(RefreshRateSelectorPtr selectorPtr) {
+        ftl::FakeGuard guard(kMainThreadContext);
+        return Scheduler::setRefreshRateSelector(std::move(selectorPtr));
+    }
+
     auto& mutableLayerHistory() { return mLayerHistory; }
 
     size_t layerHistorySize() NO_THREAD_SAFETY_ANALYSIS {