SF: Fix display mode transitions for multi-display

SF does not yet support concurrent modeset on multiple displays, as the
scheduler/modeset/VSYNC state machines are tied to the internal display
that is powered on, a.k.a. the active display.

When SF detects a change in the DM policy for a display, it initiates a
transition to the new display mode, which includes syncing to its VSYNC.
However, the per-display calls to setDesiredDisplayModeSpecs that start
this process occur after the setPowerMode calls to turn off/on the old/
new active display, respectively. Before this CL, a change in policy on
the now inactive (powered-off) display triggered a partial display mode
transition (that would start resync but abort before HWC modeset), such
that SF wound up internally inconsistent and out of sync with HWC.

Fix this by deferring the applyRefreshRateConfigsPolicy of the inactive
display until it becomes active. Later, in onActiveDisplayChangedLocked,
the Scheduler::Policy is cleared by Scheduler::setRefreshRateConfigs, so
ensure that Scheduler::getPreferredDisplayMode subsequently initializes
Scheduler::Policy::mode to the chosen mode for the newly active display.
Otherwise, applyRefreshRateConfigsPolicy falls back to its default mode.

Bug: 260092798
Test: No intermittent jank on outer/inner displays after fold/unfold.
Test: DisplayModeSwitchingTest.multiDisplay
Change-Id: Iebe1a6bb4749630333ef954955ac33807c95dd9f
Merged-In: Iebe1a6bb4749630333ef954955ac33807c95dd9f

7 files changed, 168 insertions, 23 deletions

diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp
index 26fbd55d6e..86ad4ef169 100644
--- a/services/surfaceflinger/DisplayDevice.cpp
+++ b/services/surfaceflinger/DisplayDevice.cpp
@@ -511,7 +511,7 @@ void DisplayDevice::animateRefreshRateOverlay() {
     }
 }
 
-bool DisplayDevice::setDesiredActiveMode(const ActiveModeInfo& info) {
+bool DisplayDevice::setDesiredActiveMode(const ActiveModeInfo& info, bool force) {
     ATRACE_CALL();
 
     LOG_ALWAYS_FATAL_IF(!info.mode, "desired mode not provided");
@@ -529,7 +529,7 @@ bool DisplayDevice::setDesiredActiveMode(const ActiveModeInfo& info) {
     }
 
     // Check if we are already at the desired mode
-    if (getActiveMode()->getId() == info.mode->getId()) {
+    if (!force && getActiveMode()->getId() == info.mode->getId()) {
         return false;
     }
 
diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h
index fc24a9ce49..f14bef3d11 100644
--- a/services/surfaceflinger/DisplayDevice.h
+++ b/services/surfaceflinger/DisplayDevice.h
@@ -204,7 +204,7 @@ public:
         }
     };
 
-    bool setDesiredActiveMode(const ActiveModeInfo&) EXCLUDES(mActiveModeLock);
+    bool setDesiredActiveMode(const ActiveModeInfo&, bool force = false) EXCLUDES(mActiveModeLock);
     std::optional<ActiveModeInfo> getDesiredActiveMode() const EXCLUDES(mActiveModeLock);
     void clearDesiredActiveModeState() EXCLUDES(mActiveModeLock);
     ActiveModeInfo getUpcomingActiveMode() const REQUIRES(kMainThreadContext) {
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 727cb0817e..8b1a5d9720 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -727,9 +727,7 @@ auto Scheduler::chooseDisplayMode() -> std::pair<DisplayModePtr, GlobalSignals>
 DisplayModePtr Scheduler::getPreferredDisplayMode() {
     std::lock_guard<std::mutex> lock(mPolicyLock);
     // Make sure the stored mode is up to date.
-    if (mPolicy.mode) {
-        mPolicy.mode = chooseDisplayMode().first;
-    }
+    mPolicy.mode = chooseDisplayMode().first;
     return mPolicy.mode;
 }
 
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 18807346fa..f6880ce1f3 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1070,7 +1070,7 @@ status_t SurfaceFlinger::getDisplayStats(const sp<IBinder>&, DisplayStatInfo* st
     return NO_ERROR;
 }
 
-void SurfaceFlinger::setDesiredActiveMode(const ActiveModeInfo& info) {
+void SurfaceFlinger::setDesiredActiveMode(const ActiveModeInfo& info, bool force) {
     ATRACE_CALL();
 
     if (!info.mode) {
@@ -1083,7 +1083,7 @@ void SurfaceFlinger::setDesiredActiveMode(const ActiveModeInfo& info) {
         return;
     }
 
-    if (display->setDesiredActiveMode(info)) {
+    if (display->setDesiredActiveMode(info, force)) {
         scheduleComposite(FrameHint::kNone);
 
         // Start receiving vsync samples now, so that we can detect a period
@@ -3372,8 +3372,14 @@ void SurfaceFlinger::requestDisplayMode(DisplayModePtr mode, DisplayModeEvent ev
     }
     ATRACE_CALL();
 
+    if (display->isInternal() && !isDisplayActiveLocked(display)) {
+        ALOGV("%s(%s): Inactive display", __func__, to_string(display->getId()).c_str());
+        return;
+    }
+
     if (!display->refreshRateConfigs().isModeAllowed(mode->getId())) {
-        ALOGV("Skipping disallowed mode %d", mode->getId().value());
+        ALOGV("%s(%s): Disallowed mode %d", __func__, to_string(display->getId()).c_str(),
+              mode->getId().value());
         return;
     }
 
@@ -6942,10 +6948,17 @@ status_t SurfaceFlinger::setDesiredDisplayModeSpecsInternal(
         return NO_ERROR;
     }
 
+    if (display->isInternal() && !isDisplayActiveLocked(display)) {
+        // The policy will be be applied when the display becomes active.
+        ALOGV("%s(%s): Inactive display", __func__, to_string(display->getId()).c_str());
+        return NO_ERROR;
+    }
+
     return applyRefreshRateConfigsPolicy(display);
 }
 
-status_t SurfaceFlinger::applyRefreshRateConfigsPolicy(const sp<DisplayDevice>& display) {
+status_t SurfaceFlinger::applyRefreshRateConfigsPolicy(const sp<DisplayDevice>& display,
+                                                       bool force) {
     const scheduler::RefreshRateConfigs::Policy currentPolicy =
             display->refreshRateConfigs().getCurrentPolicy();
     ALOGV("Setting desired display mode specs: %s", currentPolicy.toString().c_str());
@@ -6975,7 +6988,7 @@ status_t SurfaceFlinger::applyRefreshRateConfigsPolicy(const sp<DisplayDevice>&
     if (display->refreshRateConfigs().isModeAllowed(preferredDisplayMode->getId())) {
         ALOGV("switching to Scheduler preferred display mode %d",
               preferredDisplayMode->getId().value());
-        setDesiredActiveMode({preferredDisplayMode, DisplayModeEvent::Changed});
+        setDesiredActiveMode({preferredDisplayMode, DisplayModeEvent::Changed}, force);
     } else {
         LOG_ALWAYS_FATAL("Desired display mode not allowed: %d",
                          preferredDisplayMode->getId().value());
@@ -7304,14 +7317,23 @@ void SurfaceFlinger::onActiveDisplaySizeChanged(const sp<DisplayDevice>& activeD
 void SurfaceFlinger::onActiveDisplayChangedLocked(const sp<DisplayDevice>& activeDisplay) {
     ATRACE_CALL();
 
+    // During boot, SF powers on the primary display, which is the first display to be active. In
+    // that case, there is no need to force setDesiredActiveMode, because DM is about to send its
+    // policy via setDesiredDisplayModeSpecs.
+    bool forceApplyPolicy = false;
+
     if (const auto display = getDisplayDeviceLocked(mActiveDisplayToken)) {
         display->getCompositionDisplay()->setLayerCachingTexturePoolEnabled(false);
+        forceApplyPolicy = true;
     }
 
     if (!activeDisplay) {
         ALOGE("%s: activeDisplay is null", __func__);
         return;
     }
+
+    ALOGI("Active display is %s", to_string(activeDisplay->getPhysicalId()).c_str());
+
     mActiveDisplayToken = activeDisplay->getDisplayToken();
     activeDisplay->getCompositionDisplay()->setLayerCachingTexturePoolEnabled(true);
     updateInternalDisplayVsyncLocked(activeDisplay);
@@ -7324,7 +7346,7 @@ void SurfaceFlinger::onActiveDisplayChangedLocked(const sp<DisplayDevice>& activ
     // case, its preferred mode has not been propagated to HWC (via setDesiredActiveMode). In either
     // case, the Scheduler's cachedModeChangedParams must be initialized to the newly active mode,
     // and the kernel idle timer of the newly active display must be toggled.
-    applyRefreshRateConfigsPolicy(activeDisplay);
+    applyRefreshRateConfigsPolicy(activeDisplay, forceApplyPolicy);
 }
 
 status_t SurfaceFlinger::addWindowInfosListener(
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 64c0ae9d0c..51d721626e 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -712,7 +712,7 @@ private:
     // Called on the main thread in response to initializeDisplays()
     void onInitializeDisplays() REQUIRES(mStateLock);
     // Sets the desired active mode bit. It obtains the lock, and sets mDesiredActiveMode.
-    void setDesiredActiveMode(const ActiveModeInfo& info) REQUIRES(mStateLock);
+    void setDesiredActiveMode(const ActiveModeInfo& info, bool force = false) REQUIRES(mStateLock);
     status_t setActiveModeFromBackdoor(const sp<IBinder>& displayToken, int id);
     // Sets the active mode and a new refresh rate in SF.
     void updateInternalStateWithChangedMode() REQUIRES(mStateLock);
@@ -735,7 +735,8 @@ private:
             const std::optional<scheduler::RefreshRateConfigs::Policy>& policy, bool overridePolicy)
             EXCLUDES(mStateLock);
 
-    status_t applyRefreshRateConfigsPolicy(const sp<DisplayDevice>&) REQUIRES(mStateLock);
+    status_t applyRefreshRateConfigsPolicy(const sp<DisplayDevice>&, bool force = false)
+            REQUIRES(mStateLock);
 
     void commitTransactions() EXCLUDES(mStateLock);
     void commitTransactionsLocked(uint32_t transactionFlags) REQUIRES(mStateLock);
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
index 5872a472cd..b58add8f21 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
@@ -14,11 +14,11 @@
  * limitations under the License.
  */
 
-#include "mock/MockEventThread.h"
 #undef LOG_TAG
 #define LOG_TAG "LibSurfaceFlingerUnittests"
 
 #include "DisplayTransactionTestHelpers.h"
+#include "mock/DisplayHardware/MockDisplayMode.h"
 
 #include <scheduler/Fps.h>
 
@@ -42,14 +42,9 @@ public:
 
         mFlinger.onComposerHalHotplug(PrimaryDisplayVariant::HWC_DISPLAY_ID, Connection::CONNECTED);
 
-        {
-            DisplayModes modes = makeModes(kMode60, kMode90, kMode120, kMode90_4K);
-            auto configs = std::make_shared<scheduler::RefreshRateConfigs>(modes, kModeId60);
-
-            mDisplay = PrimaryDisplayVariant::makeFakeExistingDisplayInjector(this)
-                               .setDisplayModes(std::move(modes), kModeId60, std::move(configs))
-                               .inject();
-        }
+        mDisplay = PrimaryDisplayVariant::makeFakeExistingDisplayInjector(this)
+                           .setDisplayModes(kModes, kModeId60)
+                           .inject();
 
         setupScheduler(mDisplay->holdRefreshRateConfigs());
 
@@ -77,6 +72,8 @@ protected:
     static constexpr ui::Size kResolution4K{3840, 2160};
     static inline const DisplayModePtr kMode90_4K =
             createDisplayMode(kModeId90_4K, 90_Hz, 3, kResolution4K);
+
+    static inline const DisplayModes kModes = makeModes(kMode60, kMode90, kMode120, kMode90_4K);
 };
 
 void DisplayModeSwitchingTest::setupScheduler(
@@ -265,5 +262,112 @@ TEST_F(DisplayModeSwitchingTest, changeResolution_OnActiveDisplay_WithoutRefresh
     ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId90_4K);
 }
 
+TEST_F(DisplayModeSwitchingTest, multiDisplay) {
+    constexpr HWDisplayId kInnerDisplayHwcId = PrimaryDisplayVariant::HWC_DISPLAY_ID;
+    constexpr HWDisplayId kOuterDisplayHwcId = kInnerDisplayHwcId + 1;
+
+    constexpr PhysicalDisplayId kOuterDisplayId = PhysicalDisplayId::fromPort(254u);
+
+    constexpr bool kIsPrimary = false;
+    TestableSurfaceFlinger::FakeHwcDisplayInjector(kOuterDisplayId, hal::DisplayType::PHYSICAL,
+                                                   kIsPrimary)
+            .setHwcDisplayId(kOuterDisplayHwcId)
+            .inject(&mFlinger, mComposer);
+
+    const auto outerDisplay = mFakeDisplayInjector.injectInternalDisplay(
+            [&](FakeDisplayDeviceInjector& injector) {
+                injector.setDisplayModes(mock::cloneForDisplay(kOuterDisplayId, kModes),
+                                         kModeId120);
+            },
+            {.displayId = kOuterDisplayId,
+             .hwcDisplayId = kOuterDisplayHwcId,
+             .isPrimary = kIsPrimary});
+
+    const auto& innerDisplay = mDisplay;
+
+    EXPECT_FALSE(innerDisplay->getDesiredActiveMode());
+    EXPECT_FALSE(outerDisplay->getDesiredActiveMode());
+
+    EXPECT_EQ(innerDisplay->getActiveMode()->getId(), kModeId60);
+    EXPECT_EQ(outerDisplay->getActiveMode()->getId(), kModeId120);
+
+    mFlinger.onActiveDisplayChanged(innerDisplay);
+
+    EXPECT_EQ(NO_ERROR,
+              mFlinger.setDesiredDisplayModeSpecs(innerDisplay->getDisplayToken().promote(),
+                                                  kModeId90.value(), false, 0.f, 120.f, 0.f,
+                                                  120.f));
+
+    EXPECT_EQ(NO_ERROR,
+              mFlinger.setDesiredDisplayModeSpecs(outerDisplay->getDisplayToken().promote(),
+                                                  kModeId60.value(), false, 0.f, 120.f, 0.f,
+                                                  120.f));
+
+    // Transition on the inner display.
+    ASSERT_TRUE(innerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(innerDisplay->getDesiredActiveMode()->mode->getId(), kModeId90);
+
+    // No transition on the outer display.
+    EXPECT_FALSE(outerDisplay->getDesiredActiveMode());
+
+    const VsyncPeriodChangeTimeline timeline{.refreshRequired = true};
+    EXPECT_CALL(*mComposer,
+                setActiveConfigWithConstraints(kInnerDisplayHwcId,
+                                               hal::HWConfigId(kModeId90.value()), _, _))
+            .WillOnce(DoAll(SetArgPointee<3>(timeline), Return(Error::NONE)));
+
+    mFlinger.commit();
+
+    // Transition on the inner display.
+    ASSERT_TRUE(innerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(innerDisplay->getDesiredActiveMode()->mode->getId(), kModeId90);
+
+    // No transition on the outer display.
+    EXPECT_FALSE(outerDisplay->getDesiredActiveMode());
+
+    mFlinger.commit();
+
+    // Transition on the inner display.
+    EXPECT_FALSE(innerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(innerDisplay->getActiveMode()->getId(), kModeId90);
+
+    // No transition on the outer display.
+    EXPECT_FALSE(outerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(outerDisplay->getActiveMode()->getId(), kModeId120);
+
+    mFlinger.onActiveDisplayChanged(outerDisplay);
+
+    // No transition on the inner display.
+    EXPECT_FALSE(innerDisplay->getDesiredActiveMode());
+
+    // Transition on the outer display.
+    ASSERT_TRUE(outerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(outerDisplay->getDesiredActiveMode()->mode->getId(), kModeId60);
+
+    EXPECT_CALL(*mComposer,
+                setActiveConfigWithConstraints(kOuterDisplayHwcId,
+                                               hal::HWConfigId(kModeId60.value()), _, _))
+            .WillOnce(DoAll(SetArgPointee<3>(timeline), Return(Error::NONE)));
+
+    mFlinger.commit();
+
+    // No transition on the inner display.
+    EXPECT_FALSE(innerDisplay->getDesiredActiveMode());
+
+    // Transition on the outer display.
+    ASSERT_TRUE(outerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(outerDisplay->getDesiredActiveMode()->mode->getId(), kModeId60);
+
+    mFlinger.commit();
+
+    // No transition on the inner display.
+    EXPECT_FALSE(innerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(innerDisplay->getActiveMode()->getId(), kModeId90);
+
+    // Transition on the outer display.
+    EXPECT_FALSE(outerDisplay->getDesiredActiveMode());
+    EXPECT_EQ(outerDisplay->getActiveMode()->getId(), kModeId60);
+}
+
 } // namespace
 } // namespace android
diff --git a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockDisplayMode.h b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockDisplayMode.h
index a83ecbca26..6809580998 100644
--- a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockDisplayMode.h
+++ b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockDisplayMode.h
@@ -33,4 +33,24 @@ inline DisplayModePtr createDisplayMode(
             .build();
 }
 
+inline DisplayModePtr cloneForDisplay(PhysicalDisplayId displayId, const DisplayModePtr& modePtr) {
+    return DisplayMode::Builder(modePtr->getHwcId())
+            .setId(modePtr->getId())
+            .setPhysicalDisplayId(displayId)
+            .setVsyncPeriod(modePtr->getVsyncPeriod())
+            .setGroup(modePtr->getGroup())
+            .setResolution(modePtr->getResolution())
+            .build();
+}
+
+inline DisplayModes cloneForDisplay(PhysicalDisplayId displayId, const DisplayModes& modes) {
+    DisplayModes clones;
+
+    for (const auto& [id, modePtr] : modes) {
+        clones.try_emplace(id, cloneForDisplay(displayId, modePtr));
+    }
+
+    return clones;
+}
+
 } // namespace android::mock