commit 59db956b74a09712b3289316c40eae19c4cf2279
author Dominik Laskowski <domlaskowski@google.com> Thu Oct 27 16:18:53 2022 -0400
committer Dominik Laskowski <domlaskowski@google.com> Fri Nov 04 14:07:55 2022 -0400
tree 780fe826f284bd4a39dd1957de92388430267dc6
parent 8ecd08e78a1474285c38a38cf6db4426c5738dba

SF: Split Scheduler::setRefreshRateSelector

Add helper functions to bind/unbind the idle timer.

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

services/surfaceflinger/Scheduler/Scheduler.cpp

diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 0e1b775..6e34a68 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -94,36 +94,24 @@
     }
 }
 
-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 @@
     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));
 

services/surfaceflinger/Scheduler/Scheduler.h

diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 901cf74..797ec96 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -108,7 +108,8 @@
     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 @@
     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);