From 161feafea8274542313c38f3022581c8131eb68f Mon Sep 17 00:00:00 2001 From: Gil Dekel Date: Mon, 10 Feb 2025 18:59:29 -0500 Subject: SF: Reject hotplugs on invalid or duplicate ports When a HWC returns an invalid or duplicate port that collides with an existing active port, the end result is display identification confusion in higher layers of the stack. This is especially bad when the confusion is with the internal/primary display and causes it to malfunction. Reject hotplugs in which the reported port from HWC is already active. Flag: EXEMPT bugfix Bug: 383430671 Test: Display{Id | Identification}Test && libsurfaceflinger_unittest Change-Id: Id3569ef1d973f4ace51d14c7e3cc9aef17630b22 --- libs/ui/include/ui/DisplayMap.h | 3 + services/surfaceflinger/DisplayDevice.h | 1 + .../surfaceflinger/DisplayHardware/HWComposer.cpp | 23 ++++++-- .../surfaceflinger/DisplayHardware/HWComposer.h | 10 +++- services/surfaceflinger/SurfaceFlinger.cpp | 12 ++-- .../unittests/DisplayTransactionTestHelpers.h | 14 +++-- .../tests/unittests/SurfaceFlinger_HotplugTest.cpp | 65 +++++++++++++++++++--- ...ceFlinger_SetupNewDisplayDeviceInternalTest.cpp | 4 +- .../mock/DisplayHardware/MockHWComposer.h | 3 +- 9 files changed, 107 insertions(+), 28 deletions(-) diff --git a/libs/ui/include/ui/DisplayMap.h b/libs/ui/include/ui/DisplayMap.h index 65d2b8fa31..834a3042d1 100644 --- a/libs/ui/include/ui/DisplayMap.h +++ b/libs/ui/include/ui/DisplayMap.h @@ -18,6 +18,7 @@ #include #include +#include namespace android::ui { @@ -30,6 +31,8 @@ using DisplayMap = ftl::SmallMap; constexpr size_t kPhysicalDisplayCapacity = 3; template using PhysicalDisplayMap = ftl::SmallMap; +template +using PhysicalDisplaySet = ftl::SmallMap; template using DisplayVector = ftl::SmallVector; diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h index af2b48fc07..23b36375ce 100644 --- a/services/surfaceflinger/DisplayDevice.h +++ b/services/surfaceflinger/DisplayDevice.h @@ -260,6 +260,7 @@ struct DisplayDeviceState { struct Physical { PhysicalDisplayId id; hardware::graphics::composer::hal::HWDisplayId hwcDisplayId; + uint8_t port; DisplayModePtr activeMode; bool operator==(const Physical& other) const { diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp index c47943f6c2..db41b9b5fc 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp +++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp @@ -225,7 +225,11 @@ bool HWComposer::allocateVirtualDisplay(HalVirtualDisplayId displayId, ui::Size } void HWComposer::allocatePhysicalDisplay(hal::HWDisplayId hwcDisplayId, PhysicalDisplayId displayId, - std::optional physicalSize) { + uint8_t port, std::optional physicalSize) { + LOG_ALWAYS_FATAL_IF(!mActivePorts.try_emplace(port).second, + "Cannot attach display %" PRIu64 " to an already active port %" PRIu8 ".", + hwcDisplayId, port); + mPhysicalDisplayIdMap[hwcDisplayId] = displayId; if (!mPrimaryHwcDisplayId) { @@ -239,6 +243,7 @@ void HWComposer::allocatePhysicalDisplay(hal::HWDisplayId hwcDisplayId, Physical newDisplay->setConnected(true); newDisplay->setPhysicalSizeInMm(physicalSize); displayData.hwcDisplay = std::move(newDisplay); + displayData.port = port; } int32_t HWComposer::getAttribute(hal::HWDisplayId hwcDisplayId, hal::HWConfigId configId, @@ -758,6 +763,9 @@ void HWComposer::disconnectDisplay(HalDisplayId displayId) { const auto hwcDisplayId = displayData.hwcDisplay->getId(); mPhysicalDisplayIdMap.erase(hwcDisplayId); + if (const auto port = displayData.port) { + mActivePorts.erase(port.value()); + } mDisplayData.erase(displayId); // Reset the primary display ID if we're disconnecting it. @@ -1123,8 +1131,15 @@ std::optional HWComposer::fromPhysicalDisplayId( return {}; } -bool HWComposer::shouldIgnoreHotplugConnect(hal::HWDisplayId hwcDisplayId, +bool HWComposer::shouldIgnoreHotplugConnect(hal::HWDisplayId hwcDisplayId, uint8_t port, bool hasDisplayIdentificationData) const { + if (mActivePorts.contains(port)) { + ALOGE("Ignoring connection of display %" PRIu64 ". Port %" PRIu8 + " is already in active use.", + hwcDisplayId, port); + return true; + } + if (mHasMultiDisplaySupport && !hasDisplayIdentificationData) { ALOGE("Ignoring connection of display %" PRIu64 " without identification data", hwcDisplayId); @@ -1170,7 +1185,7 @@ std::optional HWComposer::onHotplugConnect( mHasMultiDisplaySupport ? "generalized" : "legacy"); } - if (shouldIgnoreHotplugConnect(hwcDisplayId, hasDisplayIdentificationData)) { + if (shouldIgnoreHotplugConnect(hwcDisplayId, port, hasDisplayIdentificationData)) { return {}; } @@ -1202,7 +1217,7 @@ std::optional HWComposer::onHotplugConnect( if (info->preferredDetailedTimingDescriptor) { size = info->preferredDetailedTimingDescriptor->physicalSizeInMm; } - allocatePhysicalDisplay(hwcDisplayId, info->id, size); + allocatePhysicalDisplay(hwcDisplayId, info->id, info->port, size); } return info; } diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h index d60f6ffb7c..2c0aa3d6d5 100644 --- a/services/surfaceflinger/DisplayHardware/HWComposer.h +++ b/services/surfaceflinger/DisplayHardware/HWComposer.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -144,7 +145,7 @@ public: // supported by the HWC can be queried in advance, but allocation may fail for other reasons. virtual bool allocateVirtualDisplay(HalVirtualDisplayId, ui::Size, ui::PixelFormat*) = 0; - virtual void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId, + virtual void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId, uint8_t port, std::optional physicalSize) = 0; // Attempts to create a new layer on this display @@ -362,7 +363,7 @@ public: bool allocateVirtualDisplay(HalVirtualDisplayId, ui::Size, ui::PixelFormat*) override; // Called from SurfaceFlinger, when the state for a new physical display needs to be recreated. - void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId, + void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId, uint8_t port, std::optional physicalSize) override; // Attempts to create a new layer on this display @@ -525,6 +526,7 @@ private: struct DisplayData { std::unique_ptr hwcDisplay; + std::optional port; // Set on hotplug for physical displays sp lastPresentFence = Fence::NO_FENCE; // signals when the last set op retires nsecs_t lastPresentTimestamp = 0; @@ -542,7 +544,8 @@ private: std::optional onHotplugConnect(hal::HWDisplayId); std::optional onHotplugDisconnect(hal::HWDisplayId); - bool shouldIgnoreHotplugConnect(hal::HWDisplayId, bool hasDisplayIdentificationData) const; + bool shouldIgnoreHotplugConnect(hal::HWDisplayId, uint8_t port, + bool hasDisplayIdentificationData) const; aidl::android::hardware::graphics::composer3::DisplayConfiguration::Dpi getEstimatedDotsPerInchFromSize(uint64_t hwcDisplayId, const HWCDisplayMode& hwcMode) const; @@ -564,6 +567,7 @@ private: void loadHdrConversionCapabilities(); std::unordered_map mDisplayData; + ui::PhysicalDisplaySet mActivePorts; std::unique_ptr mComposer; std::unordered_set mCapabilities; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 6384fcc483..0d0246ff04 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3718,6 +3718,7 @@ std::optional SurfaceFlinger::processHotplugConnect(PhysicalDispl if (const auto displayOpt = mPhysicalDisplays.get(displayId)) { const auto& display = displayOpt->get(); const auto& snapshot = display.snapshot(); + const uint8_t port = snapshot.port(); std::optional deviceProductInfo; if (getHwComposer().updatesDeviceProductInfoOnHotplugReconnect()) { @@ -3729,14 +3730,14 @@ std::optional SurfaceFlinger::processHotplugConnect(PhysicalDispl // Use the cached port via snapshot because we are updating an existing // display on reconnect. const auto it = - mPhysicalDisplays.try_replace(displayId, display.token(), displayId, - snapshot.port(), snapshot.connectionType(), - std::move(displayModes), std::move(colorModes), - std::move(deviceProductInfo)); + mPhysicalDisplays.try_replace(displayId, display.token(), displayId, port, + snapshot.connectionType(), std::move(displayModes), + std::move(colorModes), std::move(deviceProductInfo)); auto& state = mCurrentState.displays.editValueFor(it->second.token()); state.sequenceId = DisplayDeviceState{}.sequenceId; // Generate new sequenceId. state.physical->activeMode = std::move(activeMode); + state.physical->port = port; ALOGI("Reconnecting %s", displayString); return activeModeId; } @@ -3752,6 +3753,7 @@ std::optional SurfaceFlinger::processHotplugConnect(PhysicalDispl DisplayDeviceState state; state.physical = {.id = displayId, .hwcDisplayId = hwcDisplayId, + .port = info.port, .activeMode = std::move(activeMode)}; if (mIsHdcpViaNegVsync) { state.isSecure = connectionType == ui::DisplayConnectionType::Internal; @@ -4067,7 +4069,7 @@ void SurfaceFlinger::processDisplayChanged(const wp& displayToken, if (const auto& physical = currentState.physical) { getHwComposer().allocatePhysicalDisplay(physical->hwcDisplayId, physical->id, - /*physicalSize=*/std::nullopt); + physical->port, /*physicalSize=*/std::nullopt); } processDisplayAdded(displayToken, currentState); diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h index 81bfc977db..7f9296f3ae 100644 --- a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h +++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h @@ -533,13 +533,14 @@ struct PrimaryDisplay { static constexpr auto GET_IDENTIFICATION_DATA = getInternalEdid; }; -template +template struct SecondaryDisplay { static constexpr auto CONNECTION_TYPE = connectionType; static constexpr Primary PRIMARY = Primary::FALSE; static constexpr bool SECURE = secure; static constexpr uint8_t PORT = 254; - static constexpr HWDisplayId HWC_DISPLAY_ID = 1002; + static constexpr HWDisplayId HWC_DISPLAY_ID = hwDisplayId; static constexpr bool HAS_IDENTIFICATION_DATA = hasIdentificationData; static constexpr auto GET_IDENTIFICATION_DATA = connectionType == ui::DisplayConnectionType::Internal ? getInternalEdid @@ -571,10 +572,11 @@ using OuterDisplayNonSecureVariant = /*hasIdentificationData=*/true, kNonSecure>, 1080, 2092>; -using ExternalDisplayWithIdentificationVariant = - PhysicalDisplayVariant, - 1920, 1280>; +template +using ExternalDisplayWithIdentificationVariant = PhysicalDisplayVariant< + SecondaryDisplay, + 1920, 1280>; using ExternalDisplayVariant = PhysicalDisplayVariant, diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp index 2d986c6997..b0cda0f270 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp @@ -66,7 +66,7 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithIdentificationData) { PrimaryDisplay::setupHwcGetActiveConfigCallExpectations(this); PrimaryDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected); - // TODO(b/241286146): Remove this unnecessary call. + // TODO: b/241286146 - Remove this unnecessary call. EXPECT_CALL(*mComposer, setVsyncEnabled(PrimaryDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) .WillOnce(Return(Error::NONE)); @@ -77,12 +77,12 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithIdentificationData) { mFlinger.configure(); // Configure an external display with identification info. - using ExternalDisplay = ExternalDisplayWithIdentificationVariant; + using ExternalDisplay = ExternalDisplayWithIdentificationVariant<>; ExternalDisplay::setupHwcHotplugCallExpectations(this); ExternalDisplay::setupHwcGetActiveConfigCallExpectations(this); ExternalDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected); - // TODO(b/241286146): Remove this unnecessary call. + // TODO: b/241286146 - Remove this unnecessary call. EXPECT_CALL(*mComposer, setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) .WillOnce(Return(Error::NONE)); @@ -125,7 +125,7 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithoutIdentificationData) PrimaryDisplay::setupHwcGetActiveConfigCallExpectations(this); PrimaryDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected); - // TODO(b/241286146): Remove this unnecessary call. + // TODO: b/241286146 - Remove this unnecessary call. EXPECT_CALL(*mComposer, setVsyncEnabled(PrimaryDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) .WillOnce(Return(Error::NONE)); @@ -136,12 +136,12 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithoutIdentificationData) mFlinger.configure(); // Configure an external display with identification info. - using ExternalDisplay = ExternalDisplayWithIdentificationVariant; + using ExternalDisplay = ExternalDisplayWithIdentificationVariant<>; ExternalDisplay::setupHwcHotplugCallExpectations(this); ExternalDisplay::setupHwcGetActiveConfigCallExpectations(this); ExternalDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected); - // TODO(b/241286146): Remove this unnecessary call. + // TODO: b/241286146 - Remove this unnecessary call. EXPECT_CALL(*mComposer, setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) .WillOnce(Return(Error::NONE)); @@ -198,7 +198,7 @@ TEST_F(HotplugTest, ignoresDuplicateDisconnection) { ExternalDisplay::setupHwcHotplugCallExpectations(this); ExternalDisplay::setupHwcGetActiveConfigCallExpectations(this); - // TODO(b/241286146): Remove this unnecessary call. + // TODO: b/241286146 - Remove this unnecessary call. EXPECT_CALL(*mComposer, setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) .WillOnce(Return(Error::NONE)); @@ -242,7 +242,7 @@ TEST_F(HotplugTest, rejectsHotplugIfFailedToLoadDisplayModes) { EXPECT_CALL(*mComposer, getActiveConfig(ExternalDisplay::HWC_DISPLAY_ID, _)) .WillRepeatedly(Return(Error::BAD_DISPLAY)); - // TODO(b/241286146): Remove this unnecessary call. + // TODO: b/241286146 - Remove this unnecessary call. EXPECT_CALL(*mComposer, setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) .WillOnce(Return(Error::NONE)); @@ -262,4 +262,53 @@ TEST_F(HotplugTest, rejectsHotplugIfFailedToLoadDisplayModes) { EXPECT_FALSE(hasPhysicalHwcDisplay(ExternalDisplay::HWC_DISPLAY_ID)); } +TEST_F(HotplugTest, rejectsHotplugOnActivePortsDuplicate) { + SET_FLAG_FOR_TEST(flags::connected_display, true); + + // Inject a primary display. + PrimaryDisplayVariant::injectHwcDisplay(this); + + // Second display should come up properly. + using SecondDisplay = ExternalDisplayWithIdentificationVariant<>; + SecondDisplay::setupHwcHotplugCallExpectations(this); + SecondDisplay::setupHwcGetActiveConfigCallExpectations(this); + + // TODO: b/241286146 - Remove this unnecessary call. + EXPECT_CALL(*mComposer, + setVsyncEnabled(SecondDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE)) + .WillOnce(Return(Error::NONE)); + + EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1); + + SecondDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected); + mFlinger.configure(); + + EXPECT_TRUE(hasPhysicalHwcDisplay(SecondDisplay::HWC_DISPLAY_ID)); + + // Third display will return the same port ID as the second, and the hotplug + // should fail. + constexpr HWDisplayId kHwDisplayId = 1234; + using DuplicatePortDisplay = ExternalDisplayWithIdentificationVariant; + + // We expect display identification to be fetched correctly, since EDID and + // port are available and successfully retrieved from HAL. + EXPECT_CALL(*mComposer, + getDisplayIdentificationData(DuplicatePortDisplay::HWC_DISPLAY_ID, _, _)) + .WillOnce(DoAll(SetArgPointee<1>(*DuplicatePortDisplay::PORT::value), + SetArgPointee<2>(getExternalEedid()), Return(Error::NONE))); + + DuplicatePortDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected); + mFlinger.configure(); + + // The hotplug should be rejected due to an attempt to connect a display to an already active + // port. No HWComposer::DisplayData should be created. + EXPECT_FALSE(hasPhysicalHwcDisplay(DuplicatePortDisplay::HWC_DISPLAY_ID)); + + // Disconnecting a display that was not successfully configured should be a no-op. + DuplicatePortDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Disconnected); + mFlinger.configure(); + + EXPECT_FALSE(hasPhysicalHwcDisplay(DuplicatePortDisplay::HWC_DISPLAY_ID)); +} + } // namespace android diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp index 6951eaf093..cd554ea1ec 100644 --- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp +++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp @@ -241,7 +241,8 @@ void SetupNewDisplayDeviceInternalTest::setupNewDisplayDeviceInternalTest() { ASSERT_TRUE(hwcDisplayId); const auto port = Case::Display::PORT::value; ASSERT_TRUE(port); - mFlinger.getHwComposer().allocatePhysicalDisplay(*hwcDisplayId, *displayId, std::nullopt); + mFlinger.getHwComposer().allocatePhysicalDisplay(*hwcDisplayId, *displayId, *port, + std::nullopt); DisplayModePtr activeMode = DisplayMode::Builder(Case::Display::HWC_ACTIVE_CONFIG_ID) .setResolution(Case::Display::RESOLUTION) .setVsyncPeriod(DEFAULT_VSYNC_PERIOD) @@ -252,6 +253,7 @@ void SetupNewDisplayDeviceInternalTest::setupNewDisplayDeviceInternalTest() { state.physical = {.id = *displayId, .hwcDisplayId = *hwcDisplayId, + .port = *port, .activeMode = activeMode}; ui::ColorModes colorModes; diff --git a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWComposer.h b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWComposer.h index 3fa4093aa6..01d078bbf1 100644 --- a/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWComposer.h +++ b/services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWComposer.h @@ -44,7 +44,8 @@ public: MOCK_METHOD(bool, allocateVirtualDisplay, (HalVirtualDisplayId, ui::Size, ui::PixelFormat*), (override)); MOCK_METHOD(void, allocatePhysicalDisplay, - (hal::HWDisplayId, PhysicalDisplayId, std::optional), (override)); + (hal::HWDisplayId, PhysicalDisplayId, uint8_t port, std::optional), + (override)); MOCK_METHOD(std::shared_ptr, createLayer, (HalDisplayId), (override)); MOCK_METHOD(status_t, getDeviceCompositionChanges, -- cgit v1.2.3-59-g8ed1b