Reland "SF: Set an initial mode [...] for external displays"

Rebased and modified from Iec154c488ef7af9b8e1d6386509e97c9ce85103b,
patch set 3. Does not include
I688b0c922747a80e881965a1dc243d11ba2c7438, which was reverted.

DM currently always picks 1080p@60 for an external display. More work
(tracked in b/318534874) needs to be done in SF in order to use that
mode initially. In the meantime, have SF pick the same mode on hotplug
so that it avoids extra mode switches.

Move "force" into DisplayModeRequest. If a request is already pending
this allows forcing the final request if a pending request forced it,
even if the new request does not. This matches how emitEvent is handled.

If a DisplayModeRequest is forced, don't early exit in
initiateDisplayModeChanges, which does not honor "force".

When loading display modes for an external display, look for 1080p@60
and treat it as the active mode. If the display does not have 1080p@60,
pick the mode closest to (but not exceeding) 1080p@60. DM will pick an
appropriate mode later. When adding that display, apply that active mode
so it will be used by SF.

Fixes: 305813445
Test: libsurfaceflinger_unittest
Test: YouTube is not frozen or choppy on external display hotplug.
Test: Remove the option of 1080p@60 and connect.
Change-Id: I6864d88b327e0f207947b44ce14b97f323170714

8 files changed, 178 insertions, 18 deletions

diff --git a/services/surfaceflinger/Display/DisplayModeRequest.h b/services/surfaceflinger/Display/DisplayModeRequest.h
index d07cdf55d2..c0e77bb5fc 100644
--- a/services/surfaceflinger/Display/DisplayModeRequest.h
+++ b/services/surfaceflinger/Display/DisplayModeRequest.h
@@ -27,6 +27,9 @@ struct DisplayModeRequest {
 
     // Whether to emit DisplayEventReceiver::DISPLAY_EVENT_MODE_CHANGE.
     bool emitEvent = false;
+
+    // Whether to force the request to be applied, even if the mode is unchanged.
+    bool force = false;
 };
 
 inline bool operator==(const DisplayModeRequest& lhs, const DisplayModeRequest& rhs) {
diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp
index 5f20cd9c87..45f08a4521 100644
--- a/services/surfaceflinger/DisplayDevice.cpp
+++ b/services/surfaceflinger/DisplayDevice.cpp
@@ -24,6 +24,7 @@
 
 #define ATRACE_TAG ATRACE_TAG_GRAPHICS
 
+#include <common/FlagManager.h>
 #include <compositionengine/CompositionEngine.h>
 #include <compositionengine/Display.h>
 #include <compositionengine/DisplayColorProfile.h>
@@ -214,6 +215,17 @@ void DisplayDevice::setActiveMode(DisplayModeId modeId, Fps vsyncRate, Fps rende
 bool DisplayDevice::initiateModeChange(display::DisplayModeRequest&& desiredMode,
                                        const hal::VsyncPeriodChangeConstraints& constraints,
                                        hal::VsyncPeriodChangeTimeline& outTimeline) {
+    // TODO(b/255635711): Flow the DisplayModeRequest through the desired/pending/active states. For
+    // now, `desiredMode` and `mDesiredModeOpt` are one and the same, but the latter is not cleared
+    // until the next `SF::initiateDisplayModeChanges`. However, the desired mode has been consumed
+    // at this point, so clear the `force` flag to prevent an endless loop of `initiateModeChange`.
+    if (FlagManager::getInstance().connected_display()) {
+        std::scoped_lock lock(mDesiredModeLock);
+        if (mDesiredModeOpt) {
+            mDesiredModeOpt->force = false;
+        }
+    }
+
     mPendingModeOpt = std::move(desiredMode);
     mIsModeSetPending = true;
 
@@ -517,8 +529,7 @@ void DisplayDevice::animateOverlay() {
     }
 }
 
-auto DisplayDevice::setDesiredMode(display::DisplayModeRequest&& desiredMode, bool force)
-        -> DesiredModeAction {
+auto DisplayDevice::setDesiredMode(display::DisplayModeRequest&& desiredMode) -> DesiredModeAction {
     ATRACE_CALL();
 
     const auto& desiredModePtr = desiredMode.mode.modePtr;
@@ -526,20 +537,26 @@ auto DisplayDevice::setDesiredMode(display::DisplayModeRequest&& desiredMode, bo
     LOG_ALWAYS_FATAL_IF(getPhysicalId() != desiredModePtr->getPhysicalDisplayId(),
                         "DisplayId mismatch");
 
-    ALOGV("%s(%s)", __func__, to_string(*desiredModePtr).c_str());
+    // TODO (b/318533819): Stringize DisplayModeRequest.
+    ALOGD("%s(%s, force=%s)", __func__, to_string(*desiredModePtr).c_str(),
+          desiredMode.force ? "true" : "false");
 
     std::scoped_lock lock(mDesiredModeLock);
     if (mDesiredModeOpt) {
         // A mode transition was already scheduled, so just override the desired mode.
         const bool emitEvent = mDesiredModeOpt->emitEvent;
+        const bool force = mDesiredModeOpt->force;
         mDesiredModeOpt = std::move(desiredMode);
         mDesiredModeOpt->emitEvent |= emitEvent;
+        if (FlagManager::getInstance().connected_display()) {
+            mDesiredModeOpt->force |= force;
+        }
         return DesiredModeAction::None;
     }
 
     // If the desired mode is already active...
     const auto activeMode = refreshRateSelector().getActiveMode();
-    if (!force && activeMode.modePtr->getId() == desiredModePtr->getId()) {
+    if (!desiredMode.force && activeMode.modePtr->getId() == desiredModePtr->getId()) {
         if (activeMode == desiredMode.mode) {
             return DesiredModeAction::None;
         }
diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h
index 4ab6321064..edd57cce91 100644
--- a/services/surfaceflinger/DisplayDevice.h
+++ b/services/surfaceflinger/DisplayDevice.h
@@ -189,8 +189,7 @@ public:
 
     enum class DesiredModeAction { None, InitiateDisplayModeSwitch, InitiateRenderRateSwitch };
 
-    DesiredModeAction setDesiredMode(display::DisplayModeRequest&&, bool force = false)
-            EXCLUDES(mDesiredModeLock);
+    DesiredModeAction setDesiredMode(display::DisplayModeRequest&&) EXCLUDES(mDesiredModeLock);
 
     using DisplayModeRequestOpt = ftl::Optional<display::DisplayModeRequest>;
 
diff --git a/services/surfaceflinger/DisplayHardware/HWC2.cpp b/services/surfaceflinger/DisplayHardware/HWC2.cpp
index 704ece516d..db66f5b549 100644
--- a/services/surfaceflinger/DisplayHardware/HWC2.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWC2.cpp
@@ -27,6 +27,7 @@
 #include "HWC2.h"
 
 #include <android/configuration.h>
+#include <common/FlagManager.h>
 #include <ui/Fence.h>
 #include <ui/FloatRect.h>
 #include <ui/GraphicBuffer.h>
@@ -416,7 +417,19 @@ Error Display::setActiveConfigWithConstraints(hal::HWConfigId configId,
                                               VsyncPeriodChangeTimeline* outTimeline) {
     ALOGV("[%" PRIu64 "] setActiveConfigWithConstraints", mId);
 
-    if (isVsyncPeriodSwitchSupported()) {
+    // FIXME (b/319505580): At least the first config set on an external display must be
+    // `setActiveConfig`, so skip over the block that calls `setActiveConfigWithConstraints`
+    // for simplicity.
+    ui::DisplayConnectionType type = ui::DisplayConnectionType::Internal;
+    const bool connected_display = FlagManager::getInstance().connected_display();
+    if (connected_display) {
+        if (auto err = getConnectionType(&type); err != Error::NONE) {
+            return err;
+        }
+    }
+
+    if (isVsyncPeriodSwitchSupported() &&
+        (!connected_display || type != ui::DisplayConnectionType::External)) {
         Hwc2::IComposerClient::VsyncPeriodChangeConstraints hwc2Constraints;
         hwc2Constraints.desiredTimeNanos = constraints.desiredTimeNanos;
         hwc2Constraints.seamlessRequired = constraints.seamlessRequired;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index d86de844ab..e1dbe0e081 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1230,8 +1230,10 @@ status_t SurfaceFlinger::getDisplayStats(const sp<IBinder>& displayToken,
     return NO_ERROR;
 }
 
-void SurfaceFlinger::setDesiredMode(display::DisplayModeRequest&& request, bool force) {
-    const auto displayId = request.mode.modePtr->getPhysicalDisplayId();
+void SurfaceFlinger::setDesiredMode(display::DisplayModeRequest&& desiredMode) {
+    const auto mode = desiredMode.mode;
+    const auto displayId = mode.modePtr->getPhysicalDisplayId();
+
     ATRACE_NAME(ftl::Concat(__func__, ' ', displayId.value).c_str());
 
     const auto display = getDisplayDeviceLocked(displayId);
@@ -1240,10 +1242,9 @@ void SurfaceFlinger::setDesiredMode(display::DisplayModeRequest&& request, bool
         return;
     }
 
-    const auto mode = request.mode;
-    const bool emitEvent = request.emitEvent;
+    const bool emitEvent = desiredMode.emitEvent;
 
-    switch (display->setDesiredMode(std::move(request), force)) {
+    switch (display->setDesiredMode(std::move(desiredMode))) {
         case DisplayDevice::DesiredModeAction::InitiateDisplayModeSwitch:
             // DisplayDevice::setDesiredMode updated the render rate, so inform Scheduler.
             mScheduler->setRenderRate(displayId,
@@ -1429,7 +1430,8 @@ void SurfaceFlinger::initiateDisplayModeChanges() {
               to_string(displayModePtrOpt->get()->getVsyncRate()).c_str(),
               to_string(display->getId()).c_str());
 
-        if (display->getActiveMode() == desiredModeOpt->mode) {
+        if ((!FlagManager::getInstance().connected_display() || !desiredModeOpt->force) &&
+            display->getActiveMode() == desiredModeOpt->mode) {
             applyActiveMode(display);
             continue;
         }
@@ -3295,13 +3297,88 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes(
     std::vector<HWComposer::HWCDisplayMode> hwcModes;
     std::optional<hal::HWConfigId> activeModeHwcIdOpt;
 
+    const bool isExternalDisplay = FlagManager::getInstance().connected_display() &&
+            getHwComposer().getDisplayConnectionType(displayId) ==
+                    ui::DisplayConnectionType::External;
+
     int attempt = 0;
     constexpr int kMaxAttempts = 3;
     do {
         hwcModes = getHwComposer().getModes(displayId,
                                             scheduler::RefreshRateSelector::kMinSupportedFrameRate
                                                     .getPeriodNsecs());
-        activeModeHwcIdOpt = getHwComposer().getActiveMode(displayId).value_opt();
+        const auto activeModeHwcIdExp = getHwComposer().getActiveMode(displayId);
+        activeModeHwcIdOpt = activeModeHwcIdExp.value_opt();
+
+        if (isExternalDisplay &&
+            activeModeHwcIdExp.has_error([](status_t error) { return error == NO_INIT; })) {
+            constexpr nsecs_t k59HzVsyncPeriod = 16949153;
+            constexpr nsecs_t k60HzVsyncPeriod = 16666667;
+
+            // DM sets the initial mode for an external display to 1080p@60, but
+            // this comes after SF creates its own state (including the
+            // DisplayDevice). For now, pick the same mode in order to avoid
+            // inconsistent state and unnecessary mode switching.
+            // TODO (b/318534874): Let DM decide the initial mode.
+            //
+            // Try to find 1920x1080 @ 60 Hz
+            if (const auto iter = std::find_if(hwcModes.begin(), hwcModes.end(),
+                                               [](const auto& mode) {
+                                                   return mode.width == 1920 &&
+                                                           mode.height == 1080 &&
+                                                           mode.vsyncPeriod == k60HzVsyncPeriod;
+                                               });
+                iter != hwcModes.end()) {
+                activeModeHwcIdOpt = iter->hwcId;
+                break;
+            }
+
+            // Try to find 1920x1080 @ 59-60 Hz
+            if (const auto iter = std::find_if(hwcModes.begin(), hwcModes.end(),
+                                               [](const auto& mode) {
+                                                   return mode.width == 1920 &&
+                                                           mode.height == 1080 &&
+                                                           mode.vsyncPeriod >= k60HzVsyncPeriod &&
+                                                           mode.vsyncPeriod <= k59HzVsyncPeriod;
+                                               });
+                iter != hwcModes.end()) {
+                activeModeHwcIdOpt = iter->hwcId;
+                break;
+            }
+
+            // The display does not support 1080p@60, and this is the last attempt to pick a display
+            // mode. Prefer 60 Hz if available, with the closest resolution to 1080p.
+            if (attempt + 1 == kMaxAttempts) {
+                std::vector<HWComposer::HWCDisplayMode> hwcModeOpts;
+
+                for (const auto& mode : hwcModes) {
+                    if (mode.width <= 1920 && mode.height <= 1080 &&
+                        mode.vsyncPeriod >= k60HzVsyncPeriod &&
+                        mode.vsyncPeriod <= k59HzVsyncPeriod) {
+                        hwcModeOpts.push_back(mode);
+                    }
+                }
+
+                if (const auto iter = std::max_element(hwcModeOpts.begin(), hwcModeOpts.end(),
+                                                       [](const auto& a, const auto& b) {
+                                                           const auto aSize = a.width * a.height;
+                                                           const auto bSize = b.width * b.height;
+                                                           if (aSize < bSize)
+                                                               return true;
+                                                           else if (aSize == bSize)
+                                                               return a.vsyncPeriod > b.vsyncPeriod;
+                                                           else
+                                                               return false;
+                                                       });
+                    iter != hwcModeOpts.end()) {
+                    activeModeHwcIdOpt = iter->hwcId;
+                    break;
+                }
+
+                // hwcModeOpts was empty, use hwcModes[0] as the last resort
+                activeModeHwcIdOpt = hwcModes[0].hwcId;
+            }
+        }
 
         const auto isActiveMode = [activeModeHwcIdOpt](const HWComposer::HWCDisplayMode& mode) {
             return mode.hwcId == activeModeHwcIdOpt;
@@ -3362,6 +3439,10 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes(
                 return pair.second->getHwcId() == activeModeHwcIdOpt;
             })->second;
 
+    if (isExternalDisplay) {
+        ALOGI("External display %s initial mode: {%s}", to_string(displayId).c_str(),
+              to_string(*activeMode).c_str());
+    }
     return {modes, activeMode};
 }
 
@@ -3666,6 +3747,27 @@ void SurfaceFlinger::processDisplayAdded(const wp<IBinder>& displayToken,
     }
 
     mDisplays.try_emplace(displayToken, std::move(display));
+
+    // For an external display, loadDisplayModes already attempted to select the same mode
+    // as DM, but SF still needs to be updated to match.
+    // TODO (b/318534874): Let DM decide the initial mode.
+    if (const auto& physical = state.physical;
+        mScheduler && physical && FlagManager::getInstance().connected_display()) {
+        const bool isInternalDisplay = mPhysicalDisplays.get(physical->id)
+                                               .transform(&PhysicalDisplay::isInternal)
+                                               .value_or(false);
+
+        if (!isInternalDisplay) {
+            auto activeModePtr = physical->activeMode;
+            const auto fps = activeModePtr->getPeakFps();
+
+            setDesiredMode(
+                    {.mode = scheduler::FrameRateMode{fps,
+                                                      ftl::as_non_null(std::move(activeModePtr))},
+                     .emitEvent = false,
+                     .force = true});
+        }
+    }
 }
 
 void SurfaceFlinger::processDisplayRemoved(const wp<IBinder>& displayToken) {
@@ -8342,7 +8444,7 @@ status_t SurfaceFlinger::applyRefreshRateSelectorPolicy(
         return INVALID_OPERATION;
     }
 
-    setDesiredMode({std::move(preferredMode), .emitEvent = true}, force);
+    setDesiredMode({std::move(preferredMode), .emitEvent = true, .force = force});
 
     // Update the frameRateOverride list as the display render rate might have changed
     if (mScheduler->updateFrameRateOverrides(scheduler::GlobalSignals{}, preferredFps)) {
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index c8e2a4d70a..4ed8ec6e09 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -719,7 +719,7 @@ private:
     // Show hdr sdr ratio overlay
     bool mHdrSdrRatioOverlay = false;
 
-    void setDesiredMode(display::DisplayModeRequest&&, bool force = false) REQUIRES(mStateLock);
+    void setDesiredMode(display::DisplayModeRequest&&) REQUIRES(mStateLock);
 
     status_t setActiveModeFromBackdoor(const sp<display::DisplayToken>&, DisplayModeId, Fps minFps,
                                        Fps maxFps);
diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
index 387d2f26e6..f26336a655 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
@@ -461,9 +461,11 @@ struct HwcDisplayVariant {
                     ? IComposerClient::DisplayConnectionType::INTERNAL
                     : IComposerClient::DisplayConnectionType::EXTERNAL;
 
+            using ::testing::AtLeast;
             EXPECT_CALL(*test->mComposer, getDisplayConnectionType(HWC_DISPLAY_ID, _))
-                    .WillOnce(DoAll(SetArgPointee<1>(CONNECTION_TYPE),
-                                    Return(hal::V2_4::Error::NONE)));
+                    .Times(AtLeast(1))
+                    .WillRepeatedly(DoAll(SetArgPointee<1>(CONNECTION_TYPE),
+                                          Return(hal::V2_4::Error::NONE)));
         }
 
         EXPECT_CALL(*test->mComposer, setClientTargetSlotCount(_))
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
index 8b16a8a480..82b4ad0b4f 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
@@ -21,9 +21,13 @@
 #include "mock/DisplayHardware/MockDisplayMode.h"
 #include "mock/MockDisplayModeSpecs.h"
 
+#include <com_android_graphics_surfaceflinger_flags.h>
+#include <common/test/FlagUtils.h>
 #include <ftl/fake_guard.h>
 #include <scheduler/Fps.h>
 
+using namespace com::android::graphics::surfaceflinger;
+
 namespace android {
 namespace {
 
@@ -360,6 +364,13 @@ MATCHER_P(ModeSettledTo, modeId, "") {
 }
 
 TEST_F(DisplayModeSwitchingTest, innerXorOuterDisplay) {
+    SET_FLAG_FOR_TEST(flags::connected_display, true);
+
+    // For the inner display, this is handled by setupHwcHotplugCallExpectations.
+    EXPECT_CALL(*mComposer, getDisplayConnectionType(kOuterDisplayHwcId, _))
+            .WillOnce(DoAll(SetArgPointee<1>(IComposerClient::DisplayConnectionType::INTERNAL),
+                            Return(hal::V2_4::Error::NONE)));
+
     const auto [innerDisplay, outerDisplay] = injectOuterDisplay();
 
     EXPECT_TRUE(innerDisplay->isPoweredOn());
@@ -429,6 +440,12 @@ TEST_F(DisplayModeSwitchingTest, innerXorOuterDisplay) {
 }
 
 TEST_F(DisplayModeSwitchingTest, innerAndOuterDisplay) {
+    SET_FLAG_FOR_TEST(flags::connected_display, true);
+
+    // For the inner display, this is handled by setupHwcHotplugCallExpectations.
+    EXPECT_CALL(*mComposer, getDisplayConnectionType(kOuterDisplayHwcId, _))
+            .WillOnce(DoAll(SetArgPointee<1>(IComposerClient::DisplayConnectionType::INTERNAL),
+                            Return(hal::V2_4::Error::NONE)));
     const auto [innerDisplay, outerDisplay] = injectOuterDisplay();
 
     EXPECT_TRUE(innerDisplay->isPoweredOn());
@@ -512,6 +529,13 @@ TEST_F(DisplayModeSwitchingTest, powerOffDuringModeSet) {
 }
 
 TEST_F(DisplayModeSwitchingTest, powerOffDuringConcurrentModeSet) {
+    SET_FLAG_FOR_TEST(flags::connected_display, true);
+
+    // For the inner display, this is handled by setupHwcHotplugCallExpectations.
+    EXPECT_CALL(*mComposer, getDisplayConnectionType(kOuterDisplayHwcId, _))
+            .WillOnce(DoAll(SetArgPointee<1>(IComposerClient::DisplayConnectionType::INTERNAL),
+                            Return(hal::V2_4::Error::NONE)));
+
     const auto [innerDisplay, outerDisplay] = injectOuterDisplay();
 
     EXPECT_TRUE(innerDisplay->isPoweredOn());