diff options
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 <ftl/small_map.h> #include <ftl/small_vector.h> +#include <ftl/unit.h> namespace android::ui { @@ -30,6 +31,8 @@ using DisplayMap = ftl::SmallMap<Key, Value, kDisplayCapacity>; constexpr size_t kPhysicalDisplayCapacity = 3; template <typename Key, typename Value> using PhysicalDisplayMap = ftl::SmallMap<Key, Value, kPhysicalDisplayCapacity>; +template <typename Key> +using PhysicalDisplaySet = ftl::SmallMap<Key, ftl::Unit, kPhysicalDisplayCapacity>; template <typename T> using DisplayVector = ftl::SmallVector<T, kDisplayCapacity>; 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<ui::Size> physicalSize) { + uint8_t port, std::optional<ui::Size> 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<hal::HWDisplayId> 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<DisplayIdentificationInfo> HWComposer::onHotplugConnect( mHasMultiDisplaySupport ? "generalized" : "legacy"); } - if (shouldIgnoreHotplugConnect(hwcDisplayId, hasDisplayIdentificationData)) { + if (shouldIgnoreHotplugConnect(hwcDisplayId, port, hasDisplayIdentificationData)) { return {}; } @@ -1202,7 +1217,7 @@ std::optional<DisplayIdentificationInfo> 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 <ftl/expected.h> #include <ftl/future.h> #include <ui/DisplayIdentification.h> +#include <ui/DisplayMap.h> #include <ui/FenceTime.h> #include <ui/PictureProfileHandle.h> @@ -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<ui::Size> 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<ui::Size> physicalSize) override; // Attempts to create a new layer on this display @@ -525,6 +526,7 @@ private: struct DisplayData { std::unique_ptr<HWC2::Display> hwcDisplay; + std::optional<uint8_t> port; // Set on hotplug for physical displays sp<Fence> lastPresentFence = Fence::NO_FENCE; // signals when the last set op retires nsecs_t lastPresentTimestamp = 0; @@ -542,7 +544,8 @@ private: std::optional<DisplayIdentificationInfo> onHotplugConnect(hal::HWDisplayId); std::optional<DisplayIdentificationInfo> 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<HalDisplayId, DisplayData> mDisplayData; + ui::PhysicalDisplaySet<uint8_t> mActivePorts; std::unique_ptr<android::Hwc2::Composer> mComposer; std::unordered_set<aidl::android::hardware::graphics::composer3::Capability> 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<DisplayModeId> 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> deviceProductInfo; if (getHwComposer().updatesDeviceProductInfoOnHotplugReconnect()) { @@ -3729,14 +3730,14 @@ std::optional<DisplayModeId> 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<DisplayModeId> 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<IBinder>& 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 <ui::DisplayConnectionType connectionType, bool hasIdentificationData, bool secure> +template <ui::DisplayConnectionType connectionType, bool hasIdentificationData, bool secure, + HWDisplayId hwDisplayId = 1002> 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<SecondaryDisplay<ui::DisplayConnectionType::External, - /*hasIdentificationData=*/true, kNonSecure>, - 1920, 1280>; +template <HWDisplayId hwDisplayId = 1002> +using ExternalDisplayWithIdentificationVariant = PhysicalDisplayVariant< + SecondaryDisplay<ui::DisplayConnectionType::External, + /*hasIdentificationData=*/true, kNonSecure, hwDisplayId>, + 1920, 1280>; using ExternalDisplayVariant = PhysicalDisplayVariant<SecondaryDisplay<ui::DisplayConnectionType::External, /*hasIdentificationData=*/false, kSecure>, 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<kHwDisplayId>; + + // 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<ui::Size>), (override)); + (hal::HWDisplayId, PhysicalDisplayId, uint8_t port, std::optional<ui::Size>), + (override)); MOCK_METHOD(std::shared_ptr<HWC2::Layer>, createLayer, (HalDisplayId), (override)); MOCK_METHOD(status_t, getDeviceCompositionChanges, |