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

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

SF: Handle BAD_CONFIG for getActiveConfig 

Propagate the error as NO_INIT, which SF::loadDisplayModes will handle
in a future CL.

Also, fix confusion between hal::HWDisplayId and hal::HWConfigId in SF::
loadDisplayModes.

Bug: 305813445
Test: HWComposerTest.getActiveMode
Change-Id: I2f1510ec3d883977fd77c01315e2406788f4ac90
parent 9bb429aa

services/surfaceflinger/CompositionEngine/tests/MockHWComposer.h

+1 −1
Original line number Diff line number Diff line
@@ -97,7 +97,7 @@ public: 
    MOCK_CONST_METHOD1(isConnected, bool(PhysicalDisplayId));

    MOCK_CONST_METHOD2(getModes,

                       std::vector<HWComposer::HWCDisplayMode>(PhysicalDisplayId, int32_t));

    MOCK_CONST_METHOD1(getActiveMode, std::optional<hal::HWConfigId>(PhysicalDisplayId));

    MOCK_CONST_METHOD1(getActiveMode, ftl::Expected<hal::HWConfigId, status_t>(PhysicalDisplayId));

    MOCK_CONST_METHOD1(getColorModes, std::vector<ui::ColorMode>(PhysicalDisplayId));

    MOCK_METHOD3(setActiveColorMode, status_t(PhysicalDisplayId, ui::ColorMode, ui::RenderIntent));

    MOCK_CONST_METHOD0(isUsingVrComposer, bool());

services/surfaceflinger/DisplayHardware/HWComposer.cpp

+7 −3
Original line number Diff line number Diff line
@@ -343,14 +343,18 @@ std::vector<HWComposer::HWCDisplayMode> HWComposer::getModesFromLegacyDisplayCon 
    return modes;

}



std::optional<hal::HWConfigId> HWComposer::getActiveMode(PhysicalDisplayId displayId) const {

    RETURN_IF_INVALID_DISPLAY(displayId, std::nullopt);

ftl::Expected<hal::HWConfigId, status_t> HWComposer::getActiveMode(

        PhysicalDisplayId displayId) const {

    RETURN_IF_INVALID_DISPLAY(displayId, ftl::Unexpected(BAD_INDEX));

    const auto hwcId = *fromPhysicalDisplayId(displayId);



    hal::HWConfigId configId;

    const auto error = static_cast<hal::Error>(mComposer->getActiveConfig(hwcId, &configId));

    if (error == hal::Error::BAD_CONFIG) {

        return ftl::Unexpected(NO_INIT);

    }



    RETURN_IF_HWC_ERROR_FOR("getActiveConfig", error, displayId, std::nullopt);

    RETURN_IF_HWC_ERROR_FOR("getActiveConfig", error, displayId, ftl::Unexpected(UNKNOWN_ERROR));

    return configId;

}

services/surfaceflinger/DisplayHardware/HWComposer.h

+3 −2
Original line number Diff line number Diff line
@@ -25,6 +25,7 @@ 
#include <vector>



#include <android-base/thread_annotations.h>

#include <ftl/expected.h>

#include <ftl/future.h>

#include <ui/DisplayIdentification.h>

#include <ui/FenceTime.h>
@@ -237,7 +238,7 @@ public: 
    virtual std::vector<HWCDisplayMode> getModes(PhysicalDisplayId,

                                                 int32_t maxFrameIntervalNs) const = 0;



    virtual std::optional<hal::HWConfigId> getActiveMode(PhysicalDisplayId) const = 0;

    virtual ftl::Expected<hal::HWConfigId, status_t> getActiveMode(PhysicalDisplayId) const = 0;



    virtual std::vector<ui::ColorMode> getColorModes(PhysicalDisplayId) const = 0;
@@ -424,7 +425,7 @@ public: 
    std::vector<HWCDisplayMode> getModes(PhysicalDisplayId,

                                         int32_t maxFrameIntervalNs) const override;



    std::optional<hal::HWConfigId> getActiveMode(PhysicalDisplayId) const override;

    ftl::Expected<hal::HWConfigId, status_t> getActiveMode(PhysicalDisplayId) const override;



    std::vector<ui::ColorMode> getColorModes(PhysicalDisplayId) const override;

services/surfaceflinger/SurfaceFlinger.cpp

+7 −7
Original line number Diff line number Diff line
@@ -3293,7 +3293,7 @@ void SurfaceFlinger::commitTransactions() { 
std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes(

        PhysicalDisplayId displayId) const {

    std::vector<HWComposer::HWCDisplayMode> hwcModes;

    std::optional<hal::HWDisplayId> activeModeHwcId;

    std::optional<hal::HWConfigId> activeModeHwcIdOpt;



    int attempt = 0;

    constexpr int kMaxAttempts = 3;
@@ -3301,10 +3301,10 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes( 
        hwcModes = getHwComposer().getModes(displayId,

                                            scheduler::RefreshRateSelector::kMinSupportedFrameRate

                                                    .getPeriodNsecs());

        activeModeHwcId = getHwComposer().getActiveMode(displayId);

        activeModeHwcIdOpt = getHwComposer().getActiveMode(displayId).value_opt();



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

            return mode.hwcId == activeModeHwcId;

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

            return mode.hwcId == activeModeHwcIdOpt;

        };



        if (std::any_of(hwcModes.begin(), hwcModes.end(), isActiveMode)) {
@@ -3314,7 +3314,7 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes( 


    if (attempt == kMaxAttempts) {

        const std::string activeMode =

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

                activeModeHwcIdOpt ? std::to_string(*activeModeHwcIdOpt) : "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());
@@ -3358,8 +3358,8 @@ std::pair<DisplayModes, DisplayModePtr> SurfaceFlinger::loadDisplayModes( 
    // Keep IDs if modes have not changed.

    const auto& modes = sameModes ? oldModes : newModes;

    const DisplayModePtr activeMode =

            std::find_if(modes.begin(), modes.end(), [activeModeHwcId](const auto& pair) {

                return pair.second->getHwcId() == activeModeHwcId;

            std::find_if(modes.begin(), modes.end(), [activeModeHwcIdOpt](const auto& pair) {

                return pair.second->getHwcId() == activeModeHwcIdOpt;

            })->second;



    return {modes, activeMode};

services/surfaceflinger/tests/unittests/HWComposerTest.cpp

+9 −3
Original line number Diff line number Diff line
@@ -104,7 +104,7 @@ TEST_F(HWComposerTest, isHeadless) { 


TEST_F(HWComposerTest, getActiveMode) {

    // Unknown display.

    EXPECT_EQ(mHwc.getActiveMode(PhysicalDisplayId::fromPort(0)), std::nullopt);

    EXPECT_EQ(mHwc.getActiveMode(PhysicalDisplayId::fromPort(0)), ftl::Unexpected(BAD_INDEX));



    constexpr hal::HWDisplayId kHwcDisplayId = 2;

    expectHotplugConnect(kHwcDisplayId);
@@ -117,14 +117,20 @@ TEST_F(HWComposerTest, getActiveMode) { 
        EXPECT_CALL(*mHal, getActiveConfig(kHwcDisplayId, _))

                .WillOnce(Return(HalError::BAD_DISPLAY));



        EXPECT_EQ(mHwc.getActiveMode(info->id), std::nullopt);

        EXPECT_EQ(mHwc.getActiveMode(info->id), ftl::Unexpected(UNKNOWN_ERROR));

    }

    {

        EXPECT_CALL(*mHal, getActiveConfig(kHwcDisplayId, _))

                .WillOnce(Return(HalError::BAD_CONFIG));



        EXPECT_EQ(mHwc.getActiveMode(info->id), ftl::Unexpected(NO_INIT));

    }

    {

        constexpr hal::HWConfigId kConfigId = 42;

        EXPECT_CALL(*mHal, getActiveConfig(kHwcDisplayId, _))

                .WillOnce(DoAll(SetArgPointee<1>(kConfigId), Return(HalError::NONE)));



        EXPECT_EQ(mHwc.getActiveMode(info->id), kConfigId);

        EXPECT_EQ(mHwc.getActiveMode(info->id).value_opt(), kConfigId);

    }

}