SF: Define DisplayId as struct type
This prevents accidental mix-up with HWC display IDs, which have the
same backing type, as well as other implicitly convertible integers,
e.g. EDID manufacturer IDs. This CL also fixes misuses detected by
type checking:
1) Recycling of virtual display IDs.
2) Skipping composition for non-HWC virtual displays.
3) Unit tests for fallback/virtual display IDs.
Bug: 74619554
Bug: 119412688
Test: libsurfaceflinger_unittest
Test: vrflinger_test on walleye_xr
Change-Id: I0be41cc93c82860e859f1adf427430436c926595
diff --git a/services/surfaceflinger/DisplayHardware/DisplayIdentification.cpp b/services/surfaceflinger/DisplayHardware/DisplayIdentification.cpp
index ec240f3..ba7818d 100644
--- a/services/surfaceflinger/DisplayHardware/DisplayIdentification.cpp
+++ b/services/surfaceflinger/DisplayHardware/DisplayIdentification.cpp
@@ -64,12 +64,16 @@
return letter < 'A' || letter > 'Z' ? '\0' : letter;
}
-DisplayId getEdidDisplayId(uint8_t port, uint16_t manufacturerId, uint32_t displayNameHash) {
- return (static_cast<DisplayId>(manufacturerId) << 40) |
- (static_cast<DisplayId>(displayNameHash) << 8) | port;
+} // namespace
+
+uint16_t DisplayId::manufacturerId() const {
+ return static_cast<uint16_t>(value >> 40);
}
-} // namespace
+DisplayId DisplayId::fromEdid(uint8_t port, uint16_t manufacturerId, uint32_t displayNameHash) {
+ return {(static_cast<Type>(manufacturerId) << 40) | (static_cast<Type>(displayNameHash) << 8) |
+ port};
+}
bool isEdid(const DisplayIdentificationData& data) {
const uint8_t kMagic[] = {0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0};
@@ -168,6 +172,10 @@
return a && b && c ? std::make_optional(PnpId{a, b, c}) : std::nullopt;
}
+std::optional<PnpId> getPnpId(DisplayId displayId) {
+ return getPnpId(displayId.manufacturerId());
+}
+
std::optional<DisplayIdentificationInfo> parseDisplayIdentificationData(
uint8_t port, const DisplayIdentificationData& data) {
if (!isEdid(data)) {
@@ -183,16 +191,16 @@
// Hash display name instead of using product code or serial number, since the latter have been
// observed to change on some displays with multiple inputs.
const auto hash = static_cast<uint32_t>(std::hash<std::string_view>()(edid->displayName));
- return DisplayIdentificationInfo{getEdidDisplayId(port, edid->manufacturerId, hash),
+ return DisplayIdentificationInfo{DisplayId::fromEdid(port, edid->manufacturerId, hash),
std::string(edid->displayName)};
}
DisplayId getFallbackDisplayId(uint8_t port) {
- return getEdidDisplayId(port, kFallbackEdidManufacturerId, 0);
+ return DisplayId::fromEdid(port, kFallbackEdidManufacturerId, 0);
}
DisplayId getVirtualDisplayId(uint32_t id) {
- return getEdidDisplayId(0, kVirtualEdidManufacturerId, id);
+ return DisplayId::fromEdid(0, kVirtualEdidManufacturerId, id);
}
} // namespace android
diff --git a/services/surfaceflinger/DisplayHardware/DisplayIdentification.h b/services/surfaceflinger/DisplayHardware/DisplayIdentification.h
index 1f2e789..1599995 100644
--- a/services/surfaceflinger/DisplayHardware/DisplayIdentification.h
+++ b/services/surfaceflinger/DisplayHardware/DisplayIdentification.h
@@ -25,7 +25,27 @@
namespace android {
-using DisplayId = uint64_t;
+struct DisplayId {
+ using Type = uint64_t;
+ Type value;
+
+ uint16_t manufacturerId() const;
+
+ static DisplayId fromEdid(uint8_t port, uint16_t manufacturerId, uint32_t displayNameHash);
+};
+
+inline bool operator==(DisplayId lhs, DisplayId rhs) {
+ return lhs.value == rhs.value;
+}
+
+inline bool operator!=(DisplayId lhs, DisplayId rhs) {
+ return !(lhs == rhs);
+}
+
+inline std::string to_string(DisplayId displayId) {
+ return std::to_string(displayId.value);
+}
+
using DisplayIdentificationData = std::vector<uint8_t>;
struct DisplayIdentificationInfo {
@@ -45,6 +65,7 @@
bool isEdid(const DisplayIdentificationData&);
std::optional<Edid> parseEdid(const DisplayIdentificationData&);
std::optional<PnpId> getPnpId(uint16_t manufacturerId);
+std::optional<PnpId> getPnpId(DisplayId);
std::optional<DisplayIdentificationInfo> parseDisplayIdentificationData(
uint8_t port, const DisplayIdentificationData&);
@@ -53,3 +74,14 @@
DisplayId getVirtualDisplayId(uint32_t id);
} // namespace android
+
+namespace std {
+
+template <>
+struct hash<android::DisplayId> {
+ size_t operator()(android::DisplayId displayId) const {
+ return hash<android::DisplayId::Type>()(displayId.value);
+ }
+};
+
+} // namespace std
diff --git a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
index f3f8d9d..27812f7 100644
--- a/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
+++ b/services/surfaceflinger/DisplayHardware/FramebufferSurface.cpp
@@ -63,7 +63,7 @@
mHasPendingRelease(false),
mPreviousBufferSlot(BufferQueue::INVALID_BUFFER_SLOT),
mPreviousBuffer() {
- ALOGV("Creating for display %" PRIu64, displayId);
+ ALOGV("Creating for display %s", to_string(displayId).c_str());
mName = "FramebufferSurface";
mConsumer->setConsumerName(mName);
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
index 4c472b8..0f25b52 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
@@ -58,11 +58,11 @@
ALOGE("%s failed for HWC display %" PRIu64 ": %s", __FUNCTION__, hwcDisplayId, msg)
#define LOG_DISPLAY_ERROR(displayId, msg) \
- ALOGE("%s failed for display %" PRIu64 ": %s", __FUNCTION__, displayId, msg)
+ ALOGE("%s failed for display %s: %s", __FUNCTION__, to_string(displayId).c_str(), msg)
-#define LOG_HWC_ERROR(what, error, displayId) \
- ALOGE("%s: %s failed for display %" PRIu64 ": %s (%d)", __FUNCTION__, what, displayId, \
- to_string(error).c_str(), static_cast<int32_t>(error))
+#define LOG_HWC_ERROR(what, error, displayId) \
+ ALOGE("%s: %s failed for display %s: %s (%d)", __FUNCTION__, what, \
+ to_string(displayId).c_str(), to_string(error).c_str(), static_cast<int32_t>(error))
#define RETURN_IF_INVALID_DISPLAY(displayId, ...) \
do { \
@@ -159,9 +159,9 @@
if (!info) return {};
}
- ALOGV("%s: %s %s display %" PRIu64 " with HWC ID %" PRIu64, __FUNCTION__,
- to_string(connection).c_str(),
- hwcDisplayId == mInternalHwcDisplayId ? "internal" : "external", info->id, hwcDisplayId);
+ ALOGV("%s: %s %s display %s with HWC ID %" PRIu64, __FUNCTION__, to_string(connection).c_str(),
+ hwcDisplayId == mInternalHwcDisplayId ? "internal" : "external",
+ to_string(info->id).c_str(), hwcDisplayId);
mHwcDevice->onHotplug(hwcDisplayId, connection);
@@ -198,15 +198,15 @@
// is a bug in the HWC implementation, but filter the extra events
// out here so they don't cause havoc downstream.
if (timestamp == mLastHwVSync[*displayId]) {
- ALOGW("Ignoring duplicate VSYNC event from HWC for display %" PRIu64 " (t=%" PRId64 ")",
- *displayId, timestamp);
+ ALOGW("Ignoring duplicate VSYNC event from HWC for display %s (t=%" PRId64 ")",
+ to_string(*displayId).c_str(), timestamp);
return false;
}
mLastHwVSync[*displayId] = timestamp;
}
- const auto tag = "HW_VSYNC_" + std::to_string(*displayId);
+ const auto tag = "HW_VSYNC_" + to_string(*displayId);
ATRACE_INT(tag.c_str(), ++mVSyncCounts[*displayId] & 1);
return true;
@@ -383,7 +383,7 @@
displayData.vsyncEnabled = enabled;
- const auto tag = "HW_VSYNC_ON_" + std::to_string(displayId);
+ const auto tag = "HW_VSYNC_ON_" + to_string(displayId);
ATRACE_INT(tag.c_str(), enabled == HWC2::Vsync::Enable ? 1 : 0);
}
}
@@ -393,7 +393,7 @@
ui::Dataspace dataspace) {
RETURN_IF_INVALID_DISPLAY(displayId, BAD_INDEX);
- ALOGV("setClientTarget for display %" PRIu64, displayId);
+ ALOGV("%s for display %s", __FUNCTION__, to_string(displayId).c_str());
auto& hwcDisplay = mDisplayData[displayId].hwcDisplay;
auto error = hwcDisplay->setClientTarget(slot, target, acquireFence, dataspace);
RETURN_IF_HWC_ERROR(error, displayId, BAD_VALUE);
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h
index 5074c2c..b78433d 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.h
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.h
@@ -233,7 +233,7 @@
cb_context* mCBContext = nullptr;
std::unordered_map<DisplayId, size_t> mVSyncCounts;
- std::unordered_set<uint32_t> mFreeVirtualDisplayIds;
+ std::unordered_set<DisplayId> mFreeVirtualDisplayIds;
uint32_t mNextVirtualDisplayId = 0;
uint32_t mRemainingHwcVirtualDisplays{mHwcDevice->getMaxVirtualDisplayCount()};