Skip to content

Commit d2b6fcd

Browse files
stagadishAndroid (Google) Code Review
stagadish
authored and
Android (Google) Code Review
committed
Merge "SF: Reject hotplugs on invalid or duplicate ports" into main
2 parents d3147f9 + 161feaf commit d2b6fcd

9 files changed

Lines changed: 107 additions & 28 deletions

File tree

‎libs/ui/include/ui/DisplayMap.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#include <ftl/small_map.h>
2020
#include <ftl/small_vector.h>
21+
#include <ftl/unit.h>
2122

2223
namespace android::ui {
2324

@@ -30,6 +31,8 @@ using DisplayMap = ftl::SmallMap<Key, Value, kDisplayCapacity>;
3031
constexpr size_t kPhysicalDisplayCapacity = 3;
3132
template <typename Key, typename Value>
3233
using PhysicalDisplayMap = ftl::SmallMap<Key, Value, kPhysicalDisplayCapacity>;
34+
template <typename Key>
35+
using PhysicalDisplaySet = ftl::SmallMap<Key, ftl::Unit, kPhysicalDisplayCapacity>;
3336

3437
template <typename T>
3538
using DisplayVector = ftl::SmallVector<T, kDisplayCapacity>;

‎services/surfaceflinger/DisplayDevice.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,7 @@ struct DisplayDeviceState {
260260
struct Physical {
261261
PhysicalDisplayId id;
262262
hardware::graphics::composer::hal::HWDisplayId hwcDisplayId;
263+
uint8_t port;
263264
DisplayModePtr activeMode;
264265

265266
bool operator==(const Physical& other) const {

‎services/surfaceflinger/DisplayHardware/HWComposer.cpp‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,11 @@ bool HWComposer::allocateVirtualDisplay(HalVirtualDisplayId displayId, ui::Size
225225
}
226226

227227
void HWComposer::allocatePhysicalDisplay(hal::HWDisplayId hwcDisplayId, PhysicalDisplayId displayId,
228-
std::optional<ui::Size> physicalSize) {
228+
uint8_t port, std::optional<ui::Size> physicalSize) {
229+
LOG_ALWAYS_FATAL_IF(!mActivePorts.try_emplace(port).second,
230+
"Cannot attach display %" PRIu64 " to an already active port %" PRIu8 ".",
231+
hwcDisplayId, port);
232+
229233
mPhysicalDisplayIdMap[hwcDisplayId] = displayId;
230234

231235
if (!mPrimaryHwcDisplayId) {
@@ -239,6 +243,7 @@ void HWComposer::allocatePhysicalDisplay(hal::HWDisplayId hwcDisplayId, Physical
239243
newDisplay->setConnected(true);
240244
newDisplay->setPhysicalSizeInMm(physicalSize);
241245
displayData.hwcDisplay = std::move(newDisplay);
246+
displayData.port = port;
242247
}
243248

244249
int32_t HWComposer::getAttribute(hal::HWDisplayId hwcDisplayId, hal::HWConfigId configId,
@@ -758,6 +763,9 @@ void HWComposer::disconnectDisplay(HalDisplayId displayId) {
758763
const auto hwcDisplayId = displayData.hwcDisplay->getId();
759764

760765
mPhysicalDisplayIdMap.erase(hwcDisplayId);
766+
if (const auto port = displayData.port) {
767+
mActivePorts.erase(port.value());
768+
}
761769
mDisplayData.erase(displayId);
762770

763771
// Reset the primary display ID if we're disconnecting it.
@@ -1123,8 +1131,15 @@ std::optional<hal::HWDisplayId> HWComposer::fromPhysicalDisplayId(
11231131
return {};
11241132
}
11251133

1126-
bool HWComposer::shouldIgnoreHotplugConnect(hal::HWDisplayId hwcDisplayId,
1134+
bool HWComposer::shouldIgnoreHotplugConnect(hal::HWDisplayId hwcDisplayId, uint8_t port,
11271135
bool hasDisplayIdentificationData) const {
1136+
if (mActivePorts.contains(port)) {
1137+
ALOGE("Ignoring connection of display %" PRIu64 ". Port %" PRIu8
1138+
" is already in active use.",
1139+
hwcDisplayId, port);
1140+
return true;
1141+
}
1142+
11281143
if (mHasMultiDisplaySupport && !hasDisplayIdentificationData) {
11291144
ALOGE("Ignoring connection of display %" PRIu64 " without identification data",
11301145
hwcDisplayId);
@@ -1170,7 +1185,7 @@ std::optional<DisplayIdentificationInfo> HWComposer::onHotplugConnect(
11701185
mHasMultiDisplaySupport ? "generalized" : "legacy");
11711186
}
11721187

1173-
if (shouldIgnoreHotplugConnect(hwcDisplayId, hasDisplayIdentificationData)) {
1188+
if (shouldIgnoreHotplugConnect(hwcDisplayId, port, hasDisplayIdentificationData)) {
11741189
return {};
11751190
}
11761191

@@ -1202,7 +1217,7 @@ std::optional<DisplayIdentificationInfo> HWComposer::onHotplugConnect(
12021217
if (info->preferredDetailedTimingDescriptor) {
12031218
size = info->preferredDetailedTimingDescriptor->physicalSizeInMm;
12041219
}
1205-
allocatePhysicalDisplay(hwcDisplayId, info->id, size);
1220+
allocatePhysicalDisplay(hwcDisplayId, info->id, info->port, size);
12061221
}
12071222
return info;
12081223
}

‎services/surfaceflinger/DisplayHardware/HWComposer.h‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include <ftl/expected.h>
2929
#include <ftl/future.h>
3030
#include <ui/DisplayIdentification.h>
31+
#include <ui/DisplayMap.h>
3132
#include <ui/FenceTime.h>
3233
#include <ui/PictureProfileHandle.h>
3334

@@ -144,7 +145,7 @@ class HWComposer {
144145
// supported by the HWC can be queried in advance, but allocation may fail for other reasons.
145146
virtual bool allocateVirtualDisplay(HalVirtualDisplayId, ui::Size, ui::PixelFormat*) = 0;
146147

147-
virtual void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId,
148+
virtual void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId, uint8_t port,
148149
std::optional<ui::Size> physicalSize) = 0;
149150

150151
// Attempts to create a new layer on this display
@@ -362,7 +363,7 @@ class HWComposer final : public android::HWComposer {
362363
bool allocateVirtualDisplay(HalVirtualDisplayId, ui::Size, ui::PixelFormat*) override;
363364

364365
// Called from SurfaceFlinger, when the state for a new physical display needs to be recreated.
365-
void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId,
366+
void allocatePhysicalDisplay(hal::HWDisplayId, PhysicalDisplayId, uint8_t port,
366367
std::optional<ui::Size> physicalSize) override;
367368

368369
// Attempts to create a new layer on this display
@@ -525,6 +526,7 @@ class HWComposer final : public android::HWComposer {
525526

526527
struct DisplayData {
527528
std::unique_ptr<HWC2::Display> hwcDisplay;
529+
std::optional<uint8_t> port; // Set on hotplug for physical displays
528530

529531
sp<Fence> lastPresentFence = Fence::NO_FENCE; // signals when the last set op retires
530532
nsecs_t lastPresentTimestamp = 0;
@@ -542,7 +544,8 @@ class HWComposer final : public android::HWComposer {
542544

543545
std::optional<DisplayIdentificationInfo> onHotplugConnect(hal::HWDisplayId);
544546
std::optional<DisplayIdentificationInfo> onHotplugDisconnect(hal::HWDisplayId);
545-
bool shouldIgnoreHotplugConnect(hal::HWDisplayId, bool hasDisplayIdentificationData) const;
547+
bool shouldIgnoreHotplugConnect(hal::HWDisplayId, uint8_t port,
548+
bool hasDisplayIdentificationData) const;
546549

547550
aidl::android::hardware::graphics::composer3::DisplayConfiguration::Dpi
548551
getEstimatedDotsPerInchFromSize(uint64_t hwcDisplayId, const HWCDisplayMode& hwcMode) const;
@@ -564,6 +567,7 @@ class HWComposer final : public android::HWComposer {
564567
void loadHdrConversionCapabilities();
565568

566569
std::unordered_map<HalDisplayId, DisplayData> mDisplayData;
570+
ui::PhysicalDisplaySet<uint8_t> mActivePorts;
567571

568572
std::unique_ptr<android::Hwc2::Composer> mComposer;
569573
std::unordered_set<aidl::android::hardware::graphics::composer3::Capability> mCapabilities;

‎services/surfaceflinger/SurfaceFlinger.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3753,6 +3753,7 @@ std::optional<DisplayModeId> SurfaceFlinger::processHotplugConnect(PhysicalDispl
37533753
if (const auto displayOpt = mPhysicalDisplays.get(displayId)) {
37543754
const auto& display = displayOpt->get();
37553755
const auto& snapshot = display.snapshot();
3756+
const uint8_t port = snapshot.port();
37563757

37573758
std::optional<DeviceProductInfo> deviceProductInfo;
37583759
if (getHwComposer().updatesDeviceProductInfoOnHotplugReconnect()) {
@@ -3764,14 +3765,14 @@ std::optional<DisplayModeId> SurfaceFlinger::processHotplugConnect(PhysicalDispl
37643765
// Use the cached port via snapshot because we are updating an existing
37653766
// display on reconnect.
37663767
const auto it =
3767-
mPhysicalDisplays.try_replace(displayId, display.token(), displayId,
3768-
snapshot.port(), snapshot.connectionType(),
3769-
std::move(displayModes), std::move(colorModes),
3770-
std::move(deviceProductInfo));
3768+
mPhysicalDisplays.try_replace(displayId, display.token(), displayId, port,
3769+
snapshot.connectionType(), std::move(displayModes),
3770+
std::move(colorModes), std::move(deviceProductInfo));
37713771

37723772
auto& state = mCurrentState.displays.editValueFor(it->second.token());
37733773
state.sequenceId = DisplayDeviceState{}.sequenceId; // Generate new sequenceId.
37743774
state.physical->activeMode = std::move(activeMode);
3775+
state.physical->port = port;
37753776
ALOGI("Reconnecting %s", displayString);
37763777
return activeModeId;
37773778
}
@@ -3787,6 +3788,7 @@ std::optional<DisplayModeId> SurfaceFlinger::processHotplugConnect(PhysicalDispl
37873788
DisplayDeviceState state;
37883789
state.physical = {.id = displayId,
37893790
.hwcDisplayId = hwcDisplayId,
3791+
.port = info.port,
37903792
.activeMode = std::move(activeMode)};
37913793
if (mIsHdcpViaNegVsync) {
37923794
state.isSecure = connectionType == ui::DisplayConnectionType::Internal;
@@ -4102,7 +4104,7 @@ void SurfaceFlinger::processDisplayChanged(const wp<IBinder>& displayToken,
41024104

41034105
if (const auto& physical = currentState.physical) {
41044106
getHwComposer().allocatePhysicalDisplay(physical->hwcDisplayId, physical->id,
4105-
/*physicalSize=*/std::nullopt);
4107+
physical->port, /*physicalSize=*/std::nullopt);
41064108
}
41074109

41084110
processDisplayAdded(displayToken, currentState);

‎services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -533,13 +533,14 @@ struct PrimaryDisplay {
533533
static constexpr auto GET_IDENTIFICATION_DATA = getInternalEdid;
534534
};
535535

536-
template <ui::DisplayConnectionType connectionType, bool hasIdentificationData, bool secure>
536+
template <ui::DisplayConnectionType connectionType, bool hasIdentificationData, bool secure,
537+
HWDisplayId hwDisplayId = 1002>
537538
struct SecondaryDisplay {
538539
static constexpr auto CONNECTION_TYPE = connectionType;
539540
static constexpr Primary PRIMARY = Primary::FALSE;
540541
static constexpr bool SECURE = secure;
541542
static constexpr uint8_t PORT = 254;
542-
static constexpr HWDisplayId HWC_DISPLAY_ID = 1002;
543+
static constexpr HWDisplayId HWC_DISPLAY_ID = hwDisplayId;
543544
static constexpr bool HAS_IDENTIFICATION_DATA = hasIdentificationData;
544545
static constexpr auto GET_IDENTIFICATION_DATA =
545546
connectionType == ui::DisplayConnectionType::Internal ? getInternalEdid
@@ -571,10 +572,11 @@ using OuterDisplayNonSecureVariant =
571572
/*hasIdentificationData=*/true, kNonSecure>,
572573
1080, 2092>;
573574

574-
using ExternalDisplayWithIdentificationVariant =
575-
PhysicalDisplayVariant<SecondaryDisplay<ui::DisplayConnectionType::External,
576-
/*hasIdentificationData=*/true, kNonSecure>,
577-
1920, 1280>;
575+
template <HWDisplayId hwDisplayId = 1002>
576+
using ExternalDisplayWithIdentificationVariant = PhysicalDisplayVariant<
577+
SecondaryDisplay<ui::DisplayConnectionType::External,
578+
/*hasIdentificationData=*/true, kNonSecure, hwDisplayId>,
579+
1920, 1280>;
578580
using ExternalDisplayVariant =
579581
PhysicalDisplayVariant<SecondaryDisplay<ui::DisplayConnectionType::External,
580582
/*hasIdentificationData=*/false, kSecure>,

‎services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp‎

Lines changed: 57 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithIdentificationData) {
6666
PrimaryDisplay::setupHwcGetActiveConfigCallExpectations(this);
6767
PrimaryDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected);
6868

69-
// TODO(b/241286146): Remove this unnecessary call.
69+
// TODO: b/241286146 - Remove this unnecessary call.
7070
EXPECT_CALL(*mComposer,
7171
setVsyncEnabled(PrimaryDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
7272
.WillOnce(Return(Error::NONE));
@@ -77,12 +77,12 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithIdentificationData) {
7777
mFlinger.configure();
7878

7979
// Configure an external display with identification info.
80-
using ExternalDisplay = ExternalDisplayWithIdentificationVariant;
80+
using ExternalDisplay = ExternalDisplayWithIdentificationVariant<>;
8181
ExternalDisplay::setupHwcHotplugCallExpectations(this);
8282
ExternalDisplay::setupHwcGetActiveConfigCallExpectations(this);
8383
ExternalDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected);
8484

85-
// TODO(b/241286146): Remove this unnecessary call.
85+
// TODO: b/241286146 - Remove this unnecessary call.
8686
EXPECT_CALL(*mComposer,
8787
setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
8888
.WillOnce(Return(Error::NONE));
@@ -125,7 +125,7 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithoutIdentificationData)
125125
PrimaryDisplay::setupHwcGetActiveConfigCallExpectations(this);
126126
PrimaryDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected);
127127

128-
// TODO(b/241286146): Remove this unnecessary call.
128+
// TODO: b/241286146 - Remove this unnecessary call.
129129
EXPECT_CALL(*mComposer,
130130
setVsyncEnabled(PrimaryDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
131131
.WillOnce(Return(Error::NONE));
@@ -136,12 +136,12 @@ TEST_F(HotplugTest, createsDisplaySnapshotsForDisplaysWithoutIdentificationData)
136136
mFlinger.configure();
137137

138138
// Configure an external display with identification info.
139-
using ExternalDisplay = ExternalDisplayWithIdentificationVariant;
139+
using ExternalDisplay = ExternalDisplayWithIdentificationVariant<>;
140140
ExternalDisplay::setupHwcHotplugCallExpectations(this);
141141
ExternalDisplay::setupHwcGetActiveConfigCallExpectations(this);
142142
ExternalDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected);
143143

144-
// TODO(b/241286146): Remove this unnecessary call.
144+
// TODO: b/241286146 - Remove this unnecessary call.
145145
EXPECT_CALL(*mComposer,
146146
setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
147147
.WillOnce(Return(Error::NONE));
@@ -198,7 +198,7 @@ TEST_F(HotplugTest, ignoresDuplicateDisconnection) {
198198
ExternalDisplay::setupHwcHotplugCallExpectations(this);
199199
ExternalDisplay::setupHwcGetActiveConfigCallExpectations(this);
200200

201-
// TODO(b/241286146): Remove this unnecessary call.
201+
// TODO: b/241286146 - Remove this unnecessary call.
202202
EXPECT_CALL(*mComposer,
203203
setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
204204
.WillOnce(Return(Error::NONE));
@@ -242,7 +242,7 @@ TEST_F(HotplugTest, rejectsHotplugIfFailedToLoadDisplayModes) {
242242
EXPECT_CALL(*mComposer, getActiveConfig(ExternalDisplay::HWC_DISPLAY_ID, _))
243243
.WillRepeatedly(Return(Error::BAD_DISPLAY));
244244

245-
// TODO(b/241286146): Remove this unnecessary call.
245+
// TODO: b/241286146 - Remove this unnecessary call.
246246
EXPECT_CALL(*mComposer,
247247
setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
248248
.WillOnce(Return(Error::NONE));
@@ -262,4 +262,53 @@ TEST_F(HotplugTest, rejectsHotplugIfFailedToLoadDisplayModes) {
262262
EXPECT_FALSE(hasPhysicalHwcDisplay(ExternalDisplay::HWC_DISPLAY_ID));
263263
}
264264

265+
TEST_F(HotplugTest, rejectsHotplugOnActivePortsDuplicate) {
266+
SET_FLAG_FOR_TEST(flags::connected_display, true);
267+
268+
// Inject a primary display.
269+
PrimaryDisplayVariant::injectHwcDisplay(this);
270+
271+
// Second display should come up properly.
272+
using SecondDisplay = ExternalDisplayWithIdentificationVariant<>;
273+
SecondDisplay::setupHwcHotplugCallExpectations(this);
274+
SecondDisplay::setupHwcGetActiveConfigCallExpectations(this);
275+
276+
// TODO: b/241286146 - Remove this unnecessary call.
277+
EXPECT_CALL(*mComposer,
278+
setVsyncEnabled(SecondDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
279+
.WillOnce(Return(Error::NONE));
280+
281+
EXPECT_CALL(*mFlinger.scheduler(), scheduleFrame(_)).Times(1);
282+
283+
SecondDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected);
284+
mFlinger.configure();
285+
286+
EXPECT_TRUE(hasPhysicalHwcDisplay(SecondDisplay::HWC_DISPLAY_ID));
287+
288+
// Third display will return the same port ID as the second, and the hotplug
289+
// should fail.
290+
constexpr HWDisplayId kHwDisplayId = 1234;
291+
using DuplicatePortDisplay = ExternalDisplayWithIdentificationVariant<kHwDisplayId>;
292+
293+
// We expect display identification to be fetched correctly, since EDID and
294+
// port are available and successfully retrieved from HAL.
295+
EXPECT_CALL(*mComposer,
296+
getDisplayIdentificationData(DuplicatePortDisplay::HWC_DISPLAY_ID, _, _))
297+
.WillOnce(DoAll(SetArgPointee<1>(*DuplicatePortDisplay::PORT::value),
298+
SetArgPointee<2>(getExternalEedid()), Return(Error::NONE)));
299+
300+
DuplicatePortDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Connected);
301+
mFlinger.configure();
302+
303+
// The hotplug should be rejected due to an attempt to connect a display to an already active
304+
// port. No HWComposer::DisplayData should be created.
305+
EXPECT_FALSE(hasPhysicalHwcDisplay(DuplicatePortDisplay::HWC_DISPLAY_ID));
306+
307+
// Disconnecting a display that was not successfully configured should be a no-op.
308+
DuplicatePortDisplay::injectPendingHotplugEvent(this, HWComposer::HotplugEvent::Disconnected);
309+
mFlinger.configure();
310+
311+
EXPECT_FALSE(hasPhysicalHwcDisplay(DuplicatePortDisplay::HWC_DISPLAY_ID));
312+
}
313+
265314
} // namespace android

‎services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,8 @@ void SetupNewDisplayDeviceInternalTest::setupNewDisplayDeviceInternalTest() {
241241
ASSERT_TRUE(hwcDisplayId);
242242
const auto port = Case::Display::PORT::value;
243243
ASSERT_TRUE(port);
244-
mFlinger.getHwComposer().allocatePhysicalDisplay(*hwcDisplayId, *displayId, std::nullopt);
244+
mFlinger.getHwComposer().allocatePhysicalDisplay(*hwcDisplayId, *displayId, *port,
245+
std::nullopt);
245246
DisplayModePtr activeMode = DisplayMode::Builder(Case::Display::HWC_ACTIVE_CONFIG_ID)
246247
.setResolution(Case::Display::RESOLUTION)
247248
.setVsyncPeriod(DEFAULT_VSYNC_PERIOD)
@@ -252,6 +253,7 @@ void SetupNewDisplayDeviceInternalTest::setupNewDisplayDeviceInternalTest() {
252253

253254
state.physical = {.id = *displayId,
254255
.hwcDisplayId = *hwcDisplayId,
256+
.port = *port,
255257
.activeMode = activeMode};
256258

257259
ui::ColorModes colorModes;

‎services/surfaceflinger/tests/unittests/mock/DisplayHardware/MockHWComposer.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ class HWComposer : public android::HWComposer {
4444
MOCK_METHOD(bool, allocateVirtualDisplay, (HalVirtualDisplayId, ui::Size, ui::PixelFormat*),
4545
(override));
4646
MOCK_METHOD(void, allocatePhysicalDisplay,
47-
(hal::HWDisplayId, PhysicalDisplayId, std::optional<ui::Size>), (override));
47+
(hal::HWDisplayId, PhysicalDisplayId, uint8_t port, std::optional<ui::Size>),
48+
(override));
4849

4950
MOCK_METHOD(std::shared_ptr<HWC2::Layer>, createLayer, (HalDisplayId), (override));
5051
MOCK_METHOD(status_t, getDeviceCompositionChanges,

0 commit comments

Comments
 (0)