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

Commit bff87588 authored by Gil Dekel's avatar Gil Dekel
Browse files

HWComposer: Display ID conflict resolution logic 

Currently, there are no safeguards against duplicate display IDs when
creating displays of any kind (physical or virtual). In rare cases where
duplicates occur, they cause critical failures in display management.

This patch introduces logic in HWComposer that resolve duplicate display
IDs upon creation of physical displays by seeding their first 8 LSB with
the display's port ID.

Flag: com.android.graphics.surfaceflinger.flags.stable_edid_ids
Bug: 394404483
Bug: 415036566
Test: HWComposerTest && DisplayIdentification_test
Test: adb shell service call SurfaceFlinger 1037
Change-Id: I94ebda3f69f83c2b8ae0f3af17094b6b9b21f64b
parent 5c1e4599

services/surfaceflinger/Display/DisplayIdentification.cpp

+10 −0
Original line number Diff line number Diff line
@@ -432,6 +432,16 @@ std::optional<DisplayIdentificationInfo> parseDisplayIdentificationData( 
    };

}



PhysicalDisplayId resolveDisplayIdCollision(PhysicalDisplayId id, uint8_t port) {

    const uint8_t lowByte = static_cast<uint8_t>(id.value) == port ? ~port : port;

    const uint64_t newIdValue = (id.value & ~0xFFULL) | lowByte;



    ALOGI("Display ID %" PRIu64 " --> resolved to %" PRIu64 " using %" PRIu8 " as suffix.",

          id.value, newIdValue, lowByte);



    return PhysicalDisplayId::fromValue(newIdValue);

}



PhysicalDisplayId getVirtualDisplayId(uint32_t id) {

    return PhysicalDisplayId::fromEdid(0, kVirtualEdidManufacturerId, id);

}

services/surfaceflinger/Display/DisplayIdentification.h

+2 −0
Original line number Diff line number Diff line
@@ -92,6 +92,8 @@ std::optional<PnpId> getPnpId(uint16_t manufacturerId); 
std::optional<DisplayIdentificationInfo> parseDisplayIdentificationData(

        uint8_t port, const DisplayIdentificationData&, android::ScreenPartStatus screenPartStatus);



PhysicalDisplayId resolveDisplayIdCollision(PhysicalDisplayId id, uint8_t port);



PhysicalDisplayId getVirtualDisplayId(uint32_t id);



// Generates a consistent, stable, and hashed display ID that is based on the

services/surfaceflinger/DisplayHardware/HWComposer.cpp

+27 −1
Original line number Diff line number Diff line
@@ -234,6 +234,13 @@ void HWComposer::allocatePhysicalDisplay(hal::HWDisplayId hwcDisplayId, Physical 
             "Attaching display %" PRIu64 " to an already active port %" PRIu8 ".", hwcDisplayId,

             port);



    if (FlagManager::getInstance().stable_edid_ids()) {

        LOG_ALWAYS_FATAL_IF(hasDisplayWithId(displayId),

                            "Cannot attach display to HAL display %" PRIu64

                            " with a duplicate display ID %" PRIu64 ".",

                            hwcDisplayId, displayId.value);

    }



    mPhysicalDisplayIdMap[hwcDisplayId] = displayId;



    if (!mPrimaryHwcDisplayId) {
@@ -1235,8 +1242,12 @@ std::optional<display::DisplayIdentificationInfo> HWComposer::onHotplugConnect( 
        info = [this, hwcDisplayId, &port, &data, &screenPartStatus, hasDisplayIdentificationData] {

            const bool isPrimary = !mPrimaryHwcDisplayId;

            if (mHasMultiDisplaySupport) {

                if (const auto info =

                if (auto info =

                            display::parseDisplayIdentificationData(port, data, screenPartStatus)) {

                    if (FlagManager::getInstance().stable_edid_ids() &&

                        hasDisplayWithId(info->id)) {

                        info->id = display::resolveDisplayIdCollision(info->id, info->port);

                    }

                    return *info;

                }

                ALOGE("Failed to parse identification data for display %" PRIu64, hwcDisplayId);
@@ -1255,6 +1266,15 @@ std::optional<display::DisplayIdentificationInfo> HWComposer::onHotplugConnect( 
            };

        }();



        // Fail the hotplug if the display ID conflict could not be resolved to avoid display

        // mixups.

        if (FlagManager::getInstance().stable_edid_ids() && hasDisplayWithId(info->id)) {

            ALOGE("Ignoring connection of display %" PRIu64

                  ". Failed to resolve Display ID collision for duplicate display ID %" PRIu64 ".",

                  hwcDisplayId, info->id.value);

            return {};

        }



        mComposer->onHotplugConnect(hwcDisplayId);

    }
@@ -1369,6 +1389,12 @@ void HWComposer::loadLayerMetadataSupport() { 
    }

}



bool HWComposer::hasDisplayWithId(PhysicalDisplayId displayId) const {

    return ftl::find_if(mPhysicalDisplayIdMap,

                        [displayId](const auto& pair) { return pair.second == displayId; })

            .has_value();

}



} // namespace impl

} // namespace android

services/surfaceflinger/DisplayHardware/HWComposer.h

+1 −0
Original line number Diff line number Diff line
@@ -592,6 +592,7 @@ private: 
    void loadLayerMetadataSupport();

    void loadOverlayProperties();

    void loadHdrConversionCapabilities();

    bool hasDisplayWithId(PhysicalDisplayId displayId) const;



    std::unordered_map<HalDisplayId, DisplayData> mDisplayData;

    ui::PhysicalDisplaySet<uint8_t> mActivePorts;

services/surfaceflinger/tests/unittests/DisplayIdentification_test.cpp

+20 −0
Original line number Diff line number Diff line
@@ -419,6 +419,26 @@ TEST(DisplayIdentificationTest, parseDisplayIdentificationData) { 
    EXPECT_EQ(4633127902230889474, tertiaryInfo->id.value);

}



TEST(DisplayIdentificationTest, resolveDisplayIdCollision) {

    // 1280000 is a value such that when ORd with uint8_t decimal values, it'll show it clearly in

    // its 3 LSB digits. So 1280000 | 12 == 1280012.

    const auto id = PhysicalDisplayId::fromValue(1280000u);

    constexpr uint8_t kPort = 1;



    // ID should be resolved by adding port 1 to the LSBs of the ID, resulting in 1280001.

    const auto resolvedId = resolveDisplayIdCollision(id, kPort);

    EXPECT_NE(resolvedId, id);

    EXPECT_EQ(1280001, resolvedId.value);



    // Since the 8 LSBs of resolvedId already match the given port 1, it should be resolved by

    // inverting the port value. The inversion of unit8_t 1 is 11111110b, which is 254 - so the

    // expected ID is 1280254u.

    const auto resolvedIdWithInvertedPort = resolveDisplayIdCollision(resolvedId, kPort);

    EXPECT_NE(resolvedIdWithInvertedPort, resolvedId);

    EXPECT_NE(resolvedIdWithInvertedPort, id);

    EXPECT_EQ(1280254u, resolvedIdWithInvertedPort.value);

}



TEST(DisplayIdentificationTest, generateEdidDisplayId) {

    const auto firstExternalDisplayEdidOpt = parseEdid(getExternalEdid());

    ASSERT_TRUE(firstExternalDisplayEdidOpt);