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

Commit 6e840175 authored by Marin Shalamanov's avatar Marin Shalamanov
Browse files

SF: Update the cached display modes in HWComposer on hotplug 

Currently configs in HWComposer are cached only once
per display and never updated on subseqent onHotplug
events. This may cause setActiveConfigsWithConstraints
to fail for a valid config.

This CL also removes the test HandleTransactionLockedTest::
processesHotplugConnectPrimaryDisplayWithExternalAlreadyConnected,
since it's testing an unsupported use case and its
incompatible with the tests we've added.

Bug: 159590486
Test: atest HWComposerConfigsTest
Change-Id: Ifb22c33ba5078bde35ae20a2f94a8630316da024
parent 408a34c8

services/surfaceflinger/DisplayHardware/HWComposer.cpp

+20 −26
Original line number Diff line number Diff line
@@ -74,6 +74,7 @@ 


namespace hal = android::hardware::graphics::composer::hal;



namespace android {

namespace {



using android::hardware::Return;
@@ -88,27 +89,26 @@ public: 
            mSequenceId(sequenceId),

            mVsyncSwitchingSupported(vsyncSwitchingSupported) {}



    android::hardware::Return<void> onHotplug(hal::HWDisplayId display,

                                              hal::Connection conn) override {

    Return<void> onHotplug(hal::HWDisplayId display, hal::Connection conn) override {

        mCallback->onHotplugReceived(mSequenceId, display, conn);

        return android::hardware::Void();

        return Void();

    }



    android::hardware::Return<void> onRefresh(hal::HWDisplayId display) override {

    Return<void> onRefresh(hal::HWDisplayId display) override {

        mCallback->onRefreshReceived(mSequenceId, display);

        return android::hardware::Void();

        return Void();

    }



    android::hardware::Return<void> onVsync(hal::HWDisplayId display, int64_t timestamp) override {

    Return<void> onVsync(hal::HWDisplayId display, int64_t timestamp) override {

        if (!mVsyncSwitchingSupported) {

            mCallback->onVsyncReceived(mSequenceId, display, timestamp, std::nullopt);

        } else {

            ALOGW("Unexpected onVsync callback on composer >= 2.4, ignoring.");

        }

        return android::hardware::Void();

        return Void();

    }



    android::hardware::Return<void> onVsync_2_4(hal::HWDisplayId display, int64_t timestamp,

    Return<void> onVsync_2_4(hal::HWDisplayId display, int64_t timestamp,

                             hal::VsyncPeriodNanos vsyncPeriodNanos) override {

        if (mVsyncSwitchingSupported) {

            mCallback->onVsyncReceived(mSequenceId, display, timestamp,
@@ -116,19 +116,19 @@ public: 
        } else {

            ALOGW("Unexpected onVsync_2_4 callback on composer <= 2.3, ignoring.");

        }

        return android::hardware::Void();

        return Void();

    }



    android::hardware::Return<void> onVsyncPeriodTimingChanged(

    Return<void> onVsyncPeriodTimingChanged(

            hal::HWDisplayId display,

            const hal::VsyncPeriodChangeTimeline& updatedTimeline) override {

        mCallback->onVsyncPeriodTimingChangedReceived(mSequenceId, display, updatedTimeline);

        return android::hardware::Void();

        return Void();

    }



    android::hardware::Return<void> onSeamlessPossible(hal::HWDisplayId display) override {

    Return<void> onSeamlessPossible(hal::HWDisplayId display) override {

        mCallback->onSeamlessPossible(mSequenceId, display);

        return android::hardware::Void();

        return Void();

    }



private:
@@ -139,8 +139,6 @@ private: 


} // namespace



namespace android {



HWComposer::~HWComposer() = default;



namespace impl {
@@ -295,6 +293,7 @@ void HWComposer::allocatePhysicalDisplay(hal::HWDisplayId hwcDisplayId, 
                                                  hal::DisplayType::PHYSICAL);

    newDisplay->setConnected(true);

    displayData.hwcDisplay = std::move(newDisplay);

    displayData.configs = displayData.hwcDisplay->getConfigs();

    mPhysicalDisplayIdMap[hwcDisplayId] = displayId;

}
@@ -335,14 +334,9 @@ std::vector<std::shared_ptr<const HWC2::Display::Config>> HWComposer::getConfigs 
        PhysicalDisplayId displayId) const {

    RETURN_IF_INVALID_DISPLAY(displayId, {});



    const auto& displayData = mDisplayData.at(displayId);

    auto configs = displayData.hwcDisplay->getConfigs();

    if (displayData.configMap.empty()) {

        for (size_t i = 0; i < configs.size(); ++i) {

            displayData.configMap[i] = configs[i];

        }

    }

    return configs;

    // We cache the configs when the DisplayData is created on hotplug. If the configs need to

    // change HWC will send a hotplug event which will recreate displayData.

    return mDisplayData.at(displayId).configs;

}



std::shared_ptr<const HWC2::Display::Config> HWComposer::getActiveConfig(
@@ -653,13 +647,13 @@ status_t HWComposer::setActiveConfigWithConstraints( 
    RETURN_IF_INVALID_DISPLAY(displayId, BAD_INDEX);



    auto& displayData = mDisplayData[displayId];

    if (displayData.configMap.count(configId) == 0) {

    if (configId >= displayData.configs.size()) {

        LOG_DISPLAY_ERROR(displayId, ("Invalid config " + std::to_string(configId)).c_str());

        return BAD_INDEX;

    }



    auto error =

            displayData.hwcDisplay->setActiveConfigWithConstraints(displayData.configMap[configId],

            displayData.hwcDisplay->setActiveConfigWithConstraints(displayData.configs[configId],

                                                                   constraints, outTimeline);

    RETURN_IF_HWC_ERROR(error, displayId, UNKNOWN_ERROR);

    return NO_ERROR;

services/surfaceflinger/DisplayHardware/HWComposer.h

+1 −4
Original line number Diff line number Diff line
@@ -183,7 +183,6 @@ public: 
    virtual nsecs_t getRefreshTimestamp(PhysicalDisplayId) const = 0;

    virtual bool isConnected(PhysicalDisplayId) const = 0;



    // Non-const because it can update configMap inside of mDisplayData

    virtual std::vector<std::shared_ptr<const HWC2::Display::Config>> getConfigs(

            PhysicalDisplayId) const = 0;
@@ -319,7 +318,6 @@ public: 
    nsecs_t getRefreshTimestamp(PhysicalDisplayId) const override;

    bool isConnected(PhysicalDisplayId) const override;



    // Non-const because it can update configMap inside of mDisplayData

    std::vector<std::shared_ptr<const HWC2::Display::Config>> getConfigs(

            PhysicalDisplayId) const override;
@@ -378,8 +376,7 @@ private: 
        std::unordered_map<HWC2::Layer*, sp<Fence>> releaseFences;

        buffer_handle_t outbufHandle = nullptr;

        sp<Fence> outbufAcquireFence = Fence::NO_FENCE;

        mutable std::unordered_map<int32_t,

                std::shared_ptr<const HWC2::Display::Config>> configMap;

        std::vector<std::shared_ptr<const HWC2::Display::Config>> configs;



        bool validateWasSkipped;

        hal::Error presentError;

services/surfaceflinger/Scheduler/RefreshRateConfigs.cpp

+1 −0
Original line number Diff line number Diff line
@@ -528,6 +528,7 @@ RefreshRateConfigs::RefreshRateConfigs( 
        HwcConfigIndexType currentConfigId)

      : mKnownFrameRates(constructKnownFrameRates(configs)) {

    LOG_ALWAYS_FATAL_IF(configs.empty());

    LOG_ALWAYS_FATAL_IF(currentConfigId.value() < 0);

    LOG_ALWAYS_FATAL_IF(currentConfigId.value() >= configs.size());



    for (auto configId = HwcConfigIndexType(0); configId.value() < configs.size(); configId++) {

services/surfaceflinger/SurfaceFlinger.h

+5 −19
Original line number Diff line number Diff line
@@ -865,26 +865,12 @@ private: 
    /*

     * H/W composer

     */



    // The current hardware composer interface.

    //

    // The following thread safety rules apply when accessing mHwc, either

    // directly or via getHwComposer():

    //

    // 1. When recreating mHwc, acquire mStateLock. Recreating mHwc must only be

    //    done on the main thread.

    //

    // 2. When accessing mHwc on the main thread, it's not necessary to acquire

    //    mStateLock.

    //

    // 3. When accessing mHwc on a thread other than the main thread, we always

    // The following thread safety rules apply when accessing HWComposer:

    // 1. When reading display state from HWComposer on the main thread, it's not necessary to

    //    acquire mStateLock.

    // 2. When accessing HWComposer on a thread other than the main thread, we always

    //    need to acquire mStateLock. This is because the main thread could be

    //    in the process of destroying the current mHwc instance.

    //

    // The above thread safety rules only apply to SurfaceFlinger.cpp. In

    // SurfaceFlinger_hwc1.cpp we create mHwc at surface flinger init and never

    // destroy it, so it's always safe to access mHwc from any thread without

    // acquiring mStateLock.

    //    in the process of writing display state, e.g. creating or destroying a display.

    HWComposer& getHwComposer() const;



    /*

services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h

+47 −28
Original line number Diff line number Diff line
@@ -364,7 +364,7 @@ struct HwcDisplayVariant { 
    static constexpr DisplayType HWC_DISPLAY_TYPE = hwcDisplayType;



    // The HWC active configuration id

    static constexpr int HWC_ACTIVE_CONFIG_ID = 2001;

    static constexpr hal::HWConfigId HWC_ACTIVE_CONFIG_ID = 2001;

    static constexpr PowerMode INIT_POWER_MODE = hal::PowerMode::ON;



    static void injectPendingHotplugEvent(DisplayTransactionTest* test, Connection connection) {
@@ -415,44 +415,58 @@ struct HwcDisplayVariant { 
                                                      ceDisplayArgs);

    }



    static void setupHwcHotplugCallExpectations(DisplayTransactionTest* test) {

        constexpr auto CONNECTION_TYPE =

                PhysicalDisplay::CONNECTION_TYPE == 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)));



        EXPECT_CALL(*test->mComposer, setClientTargetSlotCount(_))

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

    static void setupHwcGetConfigsCallExpectations(DisplayTransactionTest* test) {

        if (HWC_DISPLAY_TYPE == DisplayType::PHYSICAL) {

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

                .WillOnce(DoAll(SetArgPointee<1>(std::vector<unsigned>{HWC_ACTIVE_CONFIG_ID}),

                    .WillRepeatedly(DoAll(SetArgPointee<1>(std::vector<hal::HWConfigId>{

                                                  HWC_ACTIVE_CONFIG_ID}),

                                          Return(Error::NONE)));

            EXPECT_CALL(*test->mComposer,

                        getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,

                                            IComposerClient::Attribute::WIDTH, _))

                .WillOnce(DoAll(SetArgPointee<3>(DisplayVariant::WIDTH), Return(Error::NONE)));

                    .WillRepeatedly(

                            DoAll(SetArgPointee<3>(DisplayVariant::WIDTH), Return(Error::NONE)));

            EXPECT_CALL(*test->mComposer,

                        getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,

                                            IComposerClient::Attribute::HEIGHT, _))

                .WillOnce(DoAll(SetArgPointee<3>(DisplayVariant::HEIGHT), Return(Error::NONE)));

                    .WillRepeatedly(

                            DoAll(SetArgPointee<3>(DisplayVariant::HEIGHT), Return(Error::NONE)));

            EXPECT_CALL(*test->mComposer,

                        getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,

                                            IComposerClient::Attribute::VSYNC_PERIOD, _))

                .WillOnce(DoAll(SetArgPointee<3>(DEFAULT_REFRESH_RATE), Return(Error::NONE)));

                    .WillRepeatedly(

                            DoAll(SetArgPointee<3>(DEFAULT_REFRESH_RATE), Return(Error::NONE)));

            EXPECT_CALL(*test->mComposer,

                        getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,

                                            IComposerClient::Attribute::DPI_X, _))

                .WillOnce(DoAll(SetArgPointee<3>(DEFAULT_DPI), Return(Error::NONE)));

                    .WillRepeatedly(DoAll(SetArgPointee<3>(DEFAULT_DPI), Return(Error::NONE)));

            EXPECT_CALL(*test->mComposer,

                        getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,

                                            IComposerClient::Attribute::DPI_Y, _))

                .WillOnce(DoAll(SetArgPointee<3>(DEFAULT_DPI), Return(Error::NONE)));

                    .WillRepeatedly(DoAll(SetArgPointee<3>(DEFAULT_DPI), Return(Error::NONE)));

            EXPECT_CALL(*test->mComposer,

                        getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,

                                            IComposerClient::Attribute::CONFIG_GROUP, _))

                .WillOnce(DoAll(SetArgPointee<3>(-1), Return(Error::NONE)));

                    .WillRepeatedly(DoAll(SetArgPointee<3>(-1), Return(Error::NONE)));

        } else {

            EXPECT_CALL(*test->mComposer, getDisplayConfigs(_, _)).Times(0);

            EXPECT_CALL(*test->mComposer, getDisplayAttribute(_, _, _, _)).Times(0);

        }

    }



    static void setupHwcHotplugCallExpectations(DisplayTransactionTest* test) {

        constexpr auto CONNECTION_TYPE =

                PhysicalDisplay::CONNECTION_TYPE == 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)));



        EXPECT_CALL(*test->mComposer, setClientTargetSlotCount(_))

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



        setupHwcGetConfigsCallExpectations(test);



        if (PhysicalDisplay::HAS_IDENTIFICATION_DATA) {

            EXPECT_CALL(*test->mComposer, getDisplayIdentificationData(HWC_DISPLAY_ID, _, _))
@@ -559,6 +573,11 @@ struct NonHwcVirtualDisplayVariant 
                                                      ceDisplayArgs);

    }



    static void setupHwcGetConfigsCallExpectations(DisplayTransactionTest* test) {

        EXPECT_CALL(*test->mComposer, getDisplayConfigs(_, _)).Times(0);

        EXPECT_CALL(*test->mComposer, getDisplayAttribute(_, _, _, _)).Times(0);

    }



    static void setupHwcGetActiveConfigCallExpectations(DisplayTransactionTest* test) {

        EXPECT_CALL(*test->mComposer, getActiveConfig(_, _)).Times(0);

    }