Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit bab5128d authored by Dominik Laskowski's avatar Dominik Laskowski
Browse files

SF: Reject hotplug if display modes failed to load 

Do not abort if the repeated attempts to query the display modes and
active mode fail, which could happen if HWC disconnected the display
but the disconnect event has not yet been processed.

Bug: 241286153
Bug: 235488695
Test: HotplugTest.rejectsHotplugIfFailedToLoadDisplayModes
Change-Id: I0997a502dc88a28f993f51a8ef23aeac17694b90
parent e2c5b0ae

services/surfaceflinger/SurfaceFlinger.cpp

+15 −6
Original line number Diff line number Diff line
@@ -2599,10 +2599,9 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes( 
    do {

        hwcModes = getHwComposer().getModes(displayId);

        activeModeHwcId = getHwComposer().getActiveMode(displayId);

        LOG_ALWAYS_FATAL_IF(!activeModeHwcId, "HWC returned no active mode");



        const auto isActiveMode = [activeModeHwcId](const HWComposer::HWCDisplayMode& mode) {

            return mode.hwcId == *activeModeHwcId;

            return mode.hwcId == activeModeHwcId;

        };



        if (std::any_of(hwcModes.begin(), hwcModes.end(), isActiveMode)) {
@@ -2610,10 +2609,14 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes( 
        }

    } while (++attempt < kMaxAttempts);



    LOG_ALWAYS_FATAL_IF(attempt == kMaxAttempts,

                        "After %d attempts HWC still returns an active mode which is not"

                        " supported. Active mode ID = %" PRIu64 ". Supported modes = %s",

                        kMaxAttempts, *activeModeHwcId, base::Join(hwcModes, ", ").c_str());

    if (attempt == kMaxAttempts) {

        const std::string activeMode =

                activeModeHwcId ? std::to_string(*activeModeHwcId) : "unknown"s;

        ALOGE("HWC failed to report an active mode that is supported: activeModeHwcId=%s, "

              "hwcModes={%s}",

              activeMode.c_str(), base::Join(hwcModes, ", ").c_str());

        return {};

    }



    DisplayModes oldModes;

    if (const auto token = getPhysicalDisplayTokenLocked(displayId)) {
@@ -2671,6 +2674,12 @@ void SurfaceFlinger::processDisplayHotplugEventsLocked() { 


        if (event.connection == hal::Connection::CONNECTED) {

            auto [supportedModes, activeMode] = loadDisplayModes(displayId);

            if (!activeMode) {

                // TODO(b/241286153): Report hotplug failure to the framework.

                ALOGE("Failed to hotplug display %s", to_string(displayId).c_str());

                getHwComposer().disconnectDisplay(displayId);

                continue;

            }



            if (!token) {

                ALOGV("Creating display %s", to_string(displayId).c_str());

services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp

+6 −1
Original line number Diff line number Diff line
@@ -168,7 +168,12 @@ sp<DisplayDevice> DisplayTransactionTest::injectDefaultInternalDisplay( 
}



bool DisplayTransactionTest::hasPhysicalHwcDisplay(HWDisplayId hwcDisplayId) const {

    return mFlinger.hwcPhysicalDisplayIdMap().count(hwcDisplayId) == 1;

    const auto& map = mFlinger.hwcPhysicalDisplayIdMap();



    const auto it = map.find(hwcDisplayId);

    if (it == map.end()) return false;



    return mFlinger.hwcDisplayData().count(it->second) == 1;

}



bool DisplayTransactionTest::hasTransactionFlagSet(int32_t flag) const {

services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h

+11 −7
Original line number Diff line number Diff line
@@ -434,14 +434,18 @@ struct HwcDisplayVariant { 
        }

    }



    template <bool kFailedHotplug = false>

    static void setupHwcHotplugCallExpectations(DisplayTransactionTest* test) {

        if constexpr (!kFailedHotplug) {

            constexpr auto CONNECTION_TYPE =

                    PhysicalDisplay::CONNECTION_TYPE == ui::DisplayConnectionType::Internal

                    ? IComposerClient::DisplayConnectionType::INTERNAL

                    : IComposerClient::DisplayConnectionType::EXTERNAL;



            EXPECT_CALL(*test->mComposer, getDisplayConnectionType(HWC_DISPLAY_ID, _))

                .WillOnce(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(_))

                .WillOnce(Return(hal::Error::NONE));

services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp

+31 −2
Original line number Diff line number Diff line
@@ -20,7 +20,6 @@ 
#include "DisplayTransactionTestHelpers.h"



namespace android {

namespace {



class HotplugTest : public DisplayTransactionTest {};
@@ -107,5 +106,35 @@ TEST_F(HotplugTest, processesEnqueuedEventsIfCalledOnMainThread) { 
    EXPECT_TRUE(mFlinger.mutablePendingHotplugEvents().empty());

}



} // namespace

TEST_F(HotplugTest, rejectsHotplugIfFailedToLoadDisplayModes) {

    // Inject a primary display.

    PrimaryDisplayVariant::injectHwcDisplay(this);



    using ExternalDisplay = ExternalDisplayVariant;

    constexpr bool kFailedHotplug = true;

    ExternalDisplay::setupHwcHotplugCallExpectations<kFailedHotplug>(this);



    // Simulate a connect event that fails to load display modes due to HWC already having

    // disconnected the display but SF yet having to process the queued disconnect event.

    EXPECT_CALL(*mComposer, getActiveConfig(ExternalDisplay::HWC_DISPLAY_ID, _))

            .WillRepeatedly(Return(Error::BAD_DISPLAY));



    // TODO(b/241286146): Remove this unnecessary call.

    EXPECT_CALL(*mComposer,

                setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))

            .WillOnce(Return(Error::NONE));



    ExternalDisplay::injectPendingHotplugEvent(this, Connection::CONNECTED);

    mFlinger.processDisplayHotplugEvents();



    // The hotplug should be rejected, so no HWComposer::DisplayData should be created.

    EXPECT_FALSE(hasPhysicalHwcDisplay(ExternalDisplay::HWC_DISPLAY_ID));



    // Disconnecting a display that does not exist should be a no-op.

    ExternalDisplay::injectPendingHotplugEvent(this, Connection::DISCONNECTED);

    mFlinger.processDisplayHotplugEvents();



    EXPECT_FALSE(hasPhysicalHwcDisplay(ExternalDisplay::HWC_DISPLAY_ID));

}



} // namespace android

services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h

+6 −0
Original line number Diff line number Diff line
@@ -371,6 +371,10 @@ public: 
        mFlinger->onComposerHalHotplug(hwcDisplayId, connection);

    }



    void processDisplayHotplugEvents() {

        FTL_FAKE_GUARD(mFlinger->mStateLock, mFlinger->processDisplayHotplugEventsLocked());

    }



    auto setDisplayStateLocked(const DisplayState& s) {

        Mutex::Autolock lock(mFlinger->mStateLock);

        return mFlinger->setDisplayStateLocked(s);
@@ -500,7 +504,9 @@ public: 
    const auto& currentState() const { return mFlinger->mCurrentState; }

    const auto& drawingState() const { return mFlinger->mDrawingState; }

    const auto& transactionFlags() const { return mFlinger->mTransactionFlags; }



    const auto& hwcPhysicalDisplayIdMap() const { return getHwComposer().mPhysicalDisplayIdMap; }

    const auto& hwcDisplayData() const { return getHwComposer().mDisplayData; }



    auto& mutableHasWideColorDisplay() { return SurfaceFlinger::hasWideColorDisplay; }