Merge "SF: Reject hotplug if display modes failed to load"
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index db551d7..f7146c6 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2581,10 +2581,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)) {
@@ -2592,10 +2591,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)) {
@@ -2653,6 +2656,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 8f87123..7dd9935 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);
@@ -499,7 +503,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; }