SF: Reject hotplug if display modes failed to load
Do not abort if the repeated attempts to query the display modes and
active mode fail, which could happen if HWC disconnected the display
but the disconnect event has not yet been processed.
Bug: 241286153
Bug: 235488695
Test: HotplugTest.rejectsHotplugIfFailedToLoadDisplayModes
Change-Id: I0997a502dc88a28f993f51a8ef23aeac17694b90
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 744cb46..041ab25 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2599,10 +2599,9 @@
do {
hwcModes = getHwComposer().getModes(displayId);
activeModeHwcId = getHwComposer().getActiveMode(displayId);
- LOG_ALWAYS_FATAL_IF(!activeModeHwcId, "HWC returned no active mode");
const auto isActiveMode = [activeModeHwcId](const HWComposer::HWCDisplayMode& mode) {
- return mode.hwcId == *activeModeHwcId;
+ return mode.hwcId == activeModeHwcId;
};
if (std::any_of(hwcModes.begin(), hwcModes.end(), isActiveMode)) {
@@ -2610,10 +2609,14 @@
}
} while (++attempt < kMaxAttempts);
- LOG_ALWAYS_FATAL_IF(attempt == kMaxAttempts,
- "After %d attempts HWC still returns an active mode which is not"
- " supported. Active mode ID = %" PRIu64 ". Supported modes = %s",
- kMaxAttempts, *activeModeHwcId, base::Join(hwcModes, ", ").c_str());
+ if (attempt == kMaxAttempts) {
+ const std::string activeMode =
+ activeModeHwcId ? std::to_string(*activeModeHwcId) : "unknown"s;
+ ALOGE("HWC failed to report an active mode that is supported: activeModeHwcId=%s, "
+ "hwcModes={%s}",
+ activeMode.c_str(), base::Join(hwcModes, ", ").c_str());
+ return {};
+ }
DisplayModes oldModes;
if (const auto token = getPhysicalDisplayTokenLocked(displayId)) {
@@ -2671,6 +2674,12 @@
if (event.connection == hal::Connection::CONNECTED) {
auto [supportedModes, activeMode] = loadDisplayModes(displayId);
+ if (!activeMode) {
+ // TODO(b/241286153): Report hotplug failure to the framework.
+ ALOGE("Failed to hotplug display %s", to_string(displayId).c_str());
+ getHwComposer().disconnectDisplay(displayId);
+ continue;
+ }
if (!token) {
ALOGV("Creating display %s", to_string(displayId).c_str());
diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp b/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
index 044c112..31262b3 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
@@ -168,7 +168,12 @@
}
bool DisplayTransactionTest::hasPhysicalHwcDisplay(HWDisplayId hwcDisplayId) const {
- return mFlinger.hwcPhysicalDisplayIdMap().count(hwcDisplayId) == 1;
+ const auto& map = mFlinger.hwcPhysicalDisplayIdMap();
+
+ const auto it = map.find(hwcDisplayId);
+ if (it == map.end()) return false;
+
+ return mFlinger.hwcDisplayData().count(it->second) == 1;
}
bool DisplayTransactionTest::hasTransactionFlagSet(int32_t flag) const {
diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
index 0c071d4..885d6cd 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
@@ -434,14 +434,18 @@
}
}
+ template <bool kFailedHotplug = false>
static void setupHwcHotplugCallExpectations(DisplayTransactionTest* test) {
- constexpr auto CONNECTION_TYPE =
- PhysicalDisplay::CONNECTION_TYPE == ui::DisplayConnectionType::Internal
- ? IComposerClient::DisplayConnectionType::INTERNAL
- : IComposerClient::DisplayConnectionType::EXTERNAL;
+ if constexpr (!kFailedHotplug) {
+ constexpr auto CONNECTION_TYPE =
+ PhysicalDisplay::CONNECTION_TYPE == ui::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, 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));
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp
index c9a2b00..1da3bb6 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_HotplugTest.cpp
@@ -20,7 +20,6 @@
#include "DisplayTransactionTestHelpers.h"
namespace android {
-namespace {
class HotplugTest : public DisplayTransactionTest {};
@@ -107,5 +106,35 @@
EXPECT_TRUE(mFlinger.mutablePendingHotplugEvents().empty());
}
-} // namespace
+TEST_F(HotplugTest, rejectsHotplugIfFailedToLoadDisplayModes) {
+ // Inject a primary display.
+ PrimaryDisplayVariant::injectHwcDisplay(this);
+
+ using ExternalDisplay = ExternalDisplayVariant;
+ constexpr bool kFailedHotplug = true;
+ ExternalDisplay::setupHwcHotplugCallExpectations<kFailedHotplug>(this);
+
+ // Simulate a connect event that fails to load display modes due to HWC already having
+ // disconnected the display but SF yet having to process the queued disconnect event.
+ EXPECT_CALL(*mComposer, getActiveConfig(ExternalDisplay::HWC_DISPLAY_ID, _))
+ .WillRepeatedly(Return(Error::BAD_DISPLAY));
+
+ // TODO(b/241286146): Remove this unnecessary call.
+ EXPECT_CALL(*mComposer,
+ setVsyncEnabled(ExternalDisplay::HWC_DISPLAY_ID, IComposerClient::Vsync::DISABLE))
+ .WillOnce(Return(Error::NONE));
+
+ ExternalDisplay::injectPendingHotplugEvent(this, Connection::CONNECTED);
+ mFlinger.processDisplayHotplugEvents();
+
+ // The hotplug should be rejected, so no HWComposer::DisplayData should be created.
+ EXPECT_FALSE(hasPhysicalHwcDisplay(ExternalDisplay::HWC_DISPLAY_ID));
+
+ // Disconnecting a display that does not exist should be a no-op.
+ ExternalDisplay::injectPendingHotplugEvent(this, Connection::DISCONNECTED);
+ mFlinger.processDisplayHotplugEvents();
+
+ EXPECT_FALSE(hasPhysicalHwcDisplay(ExternalDisplay::HWC_DISPLAY_ID));
+}
+
} // namespace android
diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
index 407706d..d7e6462 100644
--- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
+++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
@@ -371,6 +371,10 @@
mFlinger->onComposerHalHotplug(hwcDisplayId, connection);
}
+ void processDisplayHotplugEvents() {
+ FTL_FAKE_GUARD(mFlinger->mStateLock, mFlinger->processDisplayHotplugEventsLocked());
+ }
+
auto setDisplayStateLocked(const DisplayState& s) {
Mutex::Autolock lock(mFlinger->mStateLock);
return mFlinger->setDisplayStateLocked(s);
@@ -500,7 +504,9 @@
const auto& currentState() const { return mFlinger->mCurrentState; }
const auto& drawingState() const { return mFlinger->mDrawingState; }
const auto& transactionFlags() const { return mFlinger->mTransactionFlags; }
+
const auto& hwcPhysicalDisplayIdMap() const { return getHwComposer().mPhysicalDisplayIdMap; }
+ const auto& hwcDisplayData() const { return getHwComposer().mDisplayData; }
auto& mutableHasWideColorDisplay() { return SurfaceFlinger::hasWideColorDisplay; }