commit 27cbed7f9eb1330963dd60bc35d2742d6136107a
author Ady Abraham <adyabr@google.com> Fri Apr 10 12:56:59 2020 -0700
committer Ady Abraham <adyabr@google.com> Mon Apr 13 17:58:33 2020 +0000
tree 330a3068e693afb6d9517737bf9c1d3d3ec4a836
parent 7c24a5510f030b6fba2d112b6d8236a20d7217e8

SurfaceFlinger: check pending fence when set active config is pending

ag/9653007 moved performSetActiveConfig() after the check for whether
previous frame was missed or not. This creates a logical loop with the
inactivity timer logic as the timer is reset while we try to change
the active config.

Bug: 152321942
Test: trigger refresh rate and observe refresh rate
Change-Id: I081ef8123ed46b979eae804a47c4d06491df877c

services/surfaceflinger/SurfaceFlinger.cpp

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index adcccd2..97ec987 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1044,53 +1044,44 @@
     mVSyncModulator->setPhaseOffsets(mPhaseConfiguration->getCurrentOffsets());
 }
 
-bool SurfaceFlinger::performSetActiveConfig() {
+void SurfaceFlinger::performSetActiveConfig() {
     ATRACE_CALL();
     ALOGV("performSetActiveConfig");
-    if (mCheckPendingFence) {
-        if (previousFramePending()) {
-            // fence has not signaled yet. wait for the next invalidate
-            mEventQueue->invalidate();
-            return true;
-        }
-
-        // We received the present fence from the HWC, so we assume it successfully updated
-        // the config, hence we update SF.
-        mCheckPendingFence = false;
-        setActiveConfigInternal();
-    }
-
     // Store the local variable to release the lock.
-    ActiveConfigInfo desiredActiveConfig;
-    {
+    const auto desiredActiveConfig = [&]() -> std::optional<ActiveConfigInfo> {
         std::lock_guard<std::mutex> lock(mActiveConfigLock);
-        if (!mDesiredActiveConfigChanged) {
-            return false;
+        if (mDesiredActiveConfigChanged) {
+            return mDesiredActiveConfig;
         }
-        desiredActiveConfig = mDesiredActiveConfig;
+        return std::nullopt;
+    }();
+
+    if (!desiredActiveConfig) {
+        // No desired active config pending to be applied
+        return;
     }
 
     auto& refreshRate =
-            mRefreshRateConfigs->getRefreshRateFromConfigId(desiredActiveConfig.configId);
+            mRefreshRateConfigs->getRefreshRateFromConfigId(desiredActiveConfig->configId);
     ALOGV("performSetActiveConfig changing active config to %d(%s)",
           refreshRate.getConfigId().value(), refreshRate.getName().c_str());
     const auto display = getDefaultDisplayDeviceLocked();
-    if (!display || display->getActiveConfig() == desiredActiveConfig.configId) {
+    if (!display || display->getActiveConfig() == desiredActiveConfig->configId) {
         // display is not valid or we are already in the requested mode
         // on both cases there is nothing left to do
         desiredActiveConfigChangeDone();
-        return false;
+        return;
     }
 
     // Desired active config was set, it is different than the config currently in use, however
     // allowed configs might have change by the time we process the refresh.
     // Make sure the desired config is still allowed
-    if (!isDisplayConfigAllowed(desiredActiveConfig.configId)) {
+    if (!isDisplayConfigAllowed(desiredActiveConfig->configId)) {
         desiredActiveConfigChangeDone();
-        return false;
+        return;
     }
 
-    mUpcomingActiveConfig = desiredActiveConfig;
+    mUpcomingActiveConfig = *desiredActiveConfig;
     const auto displayId = display->getId();
     LOG_ALWAYS_FATAL_IF(!displayId);
 
@@ -1110,13 +1101,12 @@
         // setActiveConfigWithConstraints may fail if a hotplug event is just about
         // to be sent. We just log the error in this case.
         ALOGW("setActiveConfigWithConstraints failed: %d", status);
-        return false;
+        return;
     }
 
     mScheduler->onNewVsyncPeriodChangeTimeline(outTimeline);
     // Scheduler will submit an empty frame to HWC if needed.
-    mCheckPendingFence = true;
-    return false;
+    mSetActiveConfigPending = true;
 }
 
 status_t SurfaceFlinger::getDisplayColorModes(const sp<IBinder>& displayToken,
@@ -1901,6 +1891,20 @@
                 mGpuFrameMissedCount++;
             }
 
+            // If we are in the middle of a config change and the fence hasn't
+            // fired yet just wait for the next invalidate
+            if (mSetActiveConfigPending) {
+                if (framePending) {
+                    mEventQueue->invalidate();
+                    break;
+                }
+
+                // We received the present fence from the HWC, so we assume it successfully updated
+                // the config, hence we update SF.
+                mSetActiveConfigPending = false;
+                setActiveConfigInternal();
+            }
+
             if (framePending && mPropagateBackpressure) {
                 if ((hwcFrameMissed && !gpuFrameMissed) ||
                     mPropagateBackpressureClientComposition) {
@@ -1970,9 +1974,7 @@
                 mScheduler->chooseRefreshRateForContent();
             }
 
-            if (performSetActiveConfig()) {
-                break;
-            }
+            performSetActiveConfig();
 
             updateCursorAsync();
             updateInputFlinger();