Merge "Revert^2 "SF: Set an initial mode [...] for external displays"" into main

Author: Dominik Laskowski

services/surfaceflinger/Display/DisplayModeRequest.h

@@ -27,9 +27,6 @@ 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) {

services/surfaceflinger/DisplayDevice.cpp

@@ -24,7 +24,6 @@
 
 #define ATRACE_TAG ATRACE_TAG_GRAPHICS
 
-#include <common/FlagManager.h>
 #include <compositionengine/CompositionEngine.h>
 #include <compositionengine/Display.h>
 #include <compositionengine/DisplayColorProfile.h>
@@ -215,17 +214,6 @@ 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;
 
@@ -529,34 +517,29 @@ void DisplayDevice::animateOverlay() {
     }
 }
 
-auto DisplayDevice::setDesiredMode(display::DisplayModeRequest&& desiredMode) -> DesiredModeAction {
+auto DisplayDevice::setDesiredMode(display::DisplayModeRequest&& desiredMode, bool force)
+        -> DesiredModeAction {
     ATRACE_CALL();
 
     const auto& desiredModePtr = desiredMode.mode.modePtr;
 
     LOG_ALWAYS_FATAL_IF(getPhysicalId() != desiredModePtr->getPhysicalDisplayId(),
                         "DisplayId mismatch");
 
-    // TODO (b/318533819): Stringize DisplayModeRequest.
-    ALOGD("%s(%s, force=%s)", __func__, to_string(*desiredModePtr).c_str(),
-          desiredMode.force ? "true" : "false");
+    ALOGV("%s(%s)", __func__, to_string(*desiredModePtr).c_str());
 
     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 (!desiredMode.force && activeMode.modePtr->getId() == desiredModePtr->getId()) {
+    if (!force && activeMode.modePtr->getId() == desiredModePtr->getId()) {
         if (activeMode == desiredMode.mode) {
             return DesiredModeAction::None;
         }

services/surfaceflinger/DisplayDevice.h

@@ -189,7 +189,8 @@ class DisplayDevice : public RefBase {
 
     enum class DesiredModeAction { None, InitiateDisplayModeSwitch, InitiateRenderRateSwitch };
 
-    DesiredModeAction setDesiredMode(display::DisplayModeRequest&&) EXCLUDES(mDesiredModeLock);
+    DesiredModeAction setDesiredMode(display::DisplayModeRequest&&, bool force = false)
+            EXCLUDES(mDesiredModeLock);
 
     using DisplayModeRequestOpt = ftl::Optional<display::DisplayModeRequest>;
 

services/surfaceflinger/DisplayHardware/HWC2.cpp

@@ -27,7 +27,6 @@
 #include "HWC2.h"
 
 #include <android/configuration.h>
-#include <common/FlagManager.h>
 #include <ui/Fence.h>
 #include <ui/FloatRect.h>
 #include <ui/GraphicBuffer.h>
@@ -417,19 +416,7 @@ Error Display::setActiveConfigWithConstraints(hal::HWConfigId configId,
                                               VsyncPeriodChangeTimeline* outTimeline) {
     ALOGV("[%" PRIu64 "] setActiveConfigWithConstraints", mId);
 
-    // 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)) {
+    if (isVsyncPeriodSwitchSupported()) {
         Hwc2::IComposerClient::VsyncPeriodChangeConstraints hwc2Constraints;
         hwc2Constraints.desiredTimeNanos = constraints.desiredTimeNanos;
         hwc2Constraints.seamlessRequired = constraints.seamlessRequired;

services/surfaceflinger/SurfaceFlinger.cpp

@@ -1230,10 +1230,8 @@ status_t SurfaceFlinger::getDisplayStats(const sp<IBinder>& displayToken,
     return NO_ERROR;
 }
 
-void SurfaceFlinger::setDesiredMode(display::DisplayModeRequest&& desiredMode) {
-    const auto mode = desiredMode.mode;
-    const auto displayId = mode.modePtr->getPhysicalDisplayId();
-
+void SurfaceFlinger::setDesiredMode(display::DisplayModeRequest&& request, bool force) {
+    const auto displayId = request.mode.modePtr->getPhysicalDisplayId();
     ATRACE_NAME(ftl::Concat(__func__, ' ', displayId.value).c_str());
 
     const auto display = getDisplayDeviceLocked(displayId);
@@ -1242,9 +1240,10 @@ void SurfaceFlinger::setDesiredMode(display::DisplayModeRequest&& desiredMode) {
         return;
     }
 
-    const bool emitEvent = desiredMode.emitEvent;
+    const auto mode = request.mode;
+    const bool emitEvent = request.emitEvent;
 
-    switch (display->setDesiredMode(std::move(desiredMode))) {
+    switch (display->setDesiredMode(std::move(request), force)) {
         case DisplayDevice::DesiredModeAction::InitiateDisplayModeSwitch:
             // DisplayDevice::setDesiredMode updated the render rate, so inform Scheduler.
             mScheduler->setRenderRate(displayId,
@@ -1430,8 +1429,7 @@ void SurfaceFlinger::initiateDisplayModeChanges() {
               to_string(displayModePtrOpt->get()->getVsyncRate()).c_str(),
               to_string(display->getId()).c_str());
 
-        if ((!FlagManager::getInstance().connected_display() || !desiredModeOpt->force) &&
-            display->getActiveMode() == desiredModeOpt->mode) {
+        if (display->getActiveMode() == desiredModeOpt->mode) {
             applyActiveMode(display);
             continue;
         }
@@ -3286,88 +3284,13 @@ 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());
-        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;
-            }
-        }
+        activeModeHwcIdOpt = getHwComposer().getActiveMode(displayId).value_opt();
 
         const auto isActiveMode = [activeModeHwcIdOpt](const HWComposer::HWCDisplayMode& mode) {
             return mode.hwcId == activeModeHwcIdOpt;
@@ -3428,10 +3351,6 @@ 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};
 }
 
@@ -3740,27 +3659,6 @@ 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) {
@@ -8485,7 +8383,7 @@ status_t SurfaceFlinger::applyRefreshRateSelectorPolicy(
         return INVALID_OPERATION;
     }
 
-    setDesiredMode({std::move(preferredMode), .emitEvent = true, .force = force});
+    setDesiredMode({std::move(preferredMode), .emitEvent = true}, force);
 
     // Update the frameRateOverride list as the display render rate might have changed
     if (mScheduler->updateFrameRateOverrides(scheduler::GlobalSignals{}, preferredFps)) {

services/surfaceflinger/SurfaceFlinger.h

@@ -716,7 +716,7 @@ class SurfaceFlinger : public BnSurfaceComposer,
     // Show hdr sdr ratio overlay
     bool mHdrSdrRatioOverlay = false;
 
-    void setDesiredMode(display::DisplayModeRequest&&) REQUIRES(mStateLock);
+    void setDesiredMode(display::DisplayModeRequest&&, bool force = false) REQUIRES(mStateLock);
 
     status_t setActiveModeFromBackdoor(const sp<display::DisplayToken>&, DisplayModeId, Fps minFps,
                                        Fps maxFps);

services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h

@@ -461,11 +461,9 @@ struct HwcDisplayVariant {
                     ? IComposerClient::DisplayConnectionType::INTERNAL
                     : IComposerClient::DisplayConnectionType::EXTERNAL;
 
-            using ::testing::AtLeast;
             EXPECT_CALL(*test->mComposer, getDisplayConnectionType(HWC_DISPLAY_ID, _))
-                    .Times(AtLeast(1))
-                    .WillRepeatedly(DoAll(SetArgPointee<1>(CONNECTION_TYPE),
-                                          Return(hal::V2_4::Error::NONE)));
+                    .WillOnce(DoAll(SetArgPointee<1>(CONNECTION_TYPE),
+                                    Return(hal::V2_4::Error::NONE)));
         }
 
         EXPECT_CALL(*test->mComposer, setClientTargetSlotCount(_))

services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp

@@ -21,13 +21,9 @@
 #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;
-
 #define EXPECT_SET_ACTIVE_CONFIG(displayId, modeId)                                 \
     EXPECT_CALL(*mComposer,                                                         \
                 setActiveConfigWithConstraints(displayId,                           \
@@ -353,13 +349,6 @@ 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());
@@ -423,12 +412,6 @@ 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());
@@ -502,13 +485,6 @@ 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());