SF: Clean up HWComposer
Besides cosmetic changes, this CL consolidates DisplayId hash tables,
adds thread annotations, and removes an unused lock.
Bug: 74619554
Test: Boot
Change-Id: I1d6d0b5df0d30c919aea4c3a9c8777babc4316b1
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.cpp b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
index 0f25b52..9bbc37f 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.cpp
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.cpp
@@ -20,31 +20,12 @@
#define LOG_TAG "HWComposer"
#define ATRACE_TAG ATRACE_TAG_GRAPHICS
-#include <inttypes.h>
-#include <math.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/types.h>
-
#include <utils/Errors.h>
-#include <utils/misc.h>
-#include <utils/NativeHandle.h>
-#include <utils/String8.h>
-#include <utils/Thread.h>
#include <utils/Trace.h>
-#include <utils/Vector.h>
#include <ui/DebugUtils.h>
#include <ui/GraphicBuffer.h>
-#include <hardware/hardware.h>
-#include <hardware/hwcomposer.h>
-
-#include <android/configuration.h>
-
-#include <cutils/properties.h>
#include <log/log.h>
#include "HWComposer.h"
@@ -85,11 +66,7 @@
namespace android {
-#define MIN_HWC_HEADER_VERSION HWC_HEADER_VERSION
-
-// ---------------------------------------------------------------------------
-
-HWComposer::HWComposer(std::unique_ptr<android::Hwc2::Composer> composer)
+HWComposer::HWComposer(std::unique_ptr<Hwc2::Composer> composer)
: mHwcDevice(std::make_unique<HWC2::Device>(std::move(composer))) {}
HWComposer::~HWComposer() {
@@ -184,30 +161,31 @@
RETURN_IF_INVALID_DISPLAY(*displayId, false);
- const auto& displayData = mDisplayData[*displayId];
+ auto& displayData = mDisplayData[*displayId];
if (displayData.isVirtual) {
LOG_DISPLAY_ERROR(*displayId, "Invalid operation on virtual display");
return false;
}
{
- Mutex::Autolock _l(mLock);
+ std::lock_guard lock(displayData.lastHwVsyncLock);
// There have been reports of HWCs that signal several vsync events
// with the same timestamp when turning the display off and on. This
// 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]) {
+ if (timestamp == displayData.lastHwVsync) {
ALOGW("Ignoring duplicate VSYNC event from HWC for display %s (t=%" PRId64 ")",
to_string(*displayId).c_str(), timestamp);
return false;
}
- mLastHwVSync[*displayId] = timestamp;
+ displayData.lastHwVsync = timestamp;
}
const auto tag = "HW_VSYNC_" + to_string(*displayId);
- ATRACE_INT(tag.c_str(), ++mVSyncCounts[*displayId] & 1);
+ ATRACE_INT(tag.c_str(), displayData.vsyncTraceToggle);
+ displayData.vsyncTraceToggle = !displayData.vsyncTraceToggle;
return true;
}
@@ -270,13 +248,14 @@
nsecs_t HWComposer::getRefreshTimestamp(DisplayId displayId) const {
RETURN_IF_INVALID_DISPLAY(displayId, 0);
+ const auto& displayData = mDisplayData.at(displayId);
// this returns the last refresh timestamp.
// if the last one is not available, we estimate it based on
// the refresh period and whatever closest timestamp we have.
- Mutex::Autolock _l(mLock);
+ std::lock_guard lock(displayData.lastHwVsyncLock);
nsecs_t now = systemTime(CLOCK_MONOTONIC);
auto vsyncPeriod = getActiveConfig(displayId)->getVsyncPeriod();
- return now - ((now - mLastHwVSync[displayId]) % vsyncPeriod);
+ return now - ((now - displayData.lastHwVsync) % vsyncPeriod);
}
bool HWComposer::isConnected(DisplayId displayId) const {
@@ -375,17 +354,19 @@
// into the HWC with the lock held, and we want to make sure
// that even if HWC blocks (which it shouldn't), it won't
// affect other threads.
- Mutex::Autolock _l(mVsyncLock);
- if (enabled != displayData.vsyncEnabled) {
- ATRACE_CALL();
- auto error = displayData.hwcDisplay->setVsyncEnabled(enabled);
- RETURN_IF_HWC_ERROR(error, displayId);
-
- displayData.vsyncEnabled = enabled;
-
- const auto tag = "HW_VSYNC_ON_" + to_string(displayId);
- ATRACE_INT(tag.c_str(), enabled == HWC2::Vsync::Enable ? 1 : 0);
+ std::lock_guard lock(displayData.vsyncEnabledLock);
+ if (enabled == displayData.vsyncEnabled) {
+ return;
}
+
+ ATRACE_CALL();
+ auto error = displayData.hwcDisplay->setVsyncEnabled(enabled);
+ RETURN_IF_HWC_ERROR(error, displayId);
+
+ displayData.vsyncEnabled = enabled;
+
+ const auto tag = "HW_VSYNC_ON_" + to_string(displayId);
+ ATRACE_INT(tag.c_str(), enabled == HWC2::Vsync::Enable ? 1 : 0);
}
status_t HWComposer::setClientTarget(DisplayId displayId, uint32_t slot,
@@ -405,8 +386,6 @@
RETURN_IF_INVALID_DISPLAY(displayId, BAD_INDEX);
- Mutex::Autolock _l(mDisplayLock);
-
auto& displayData = mDisplayData[displayId];
auto& hwcDisplay = displayData.hwcDisplay;
if (!hwcDisplay->isConnected()) {
@@ -428,7 +407,7 @@
// back to validate when there is any client layer.
displayData.validateWasSkipped = false;
if (!displayData.hasClientComposition) {
- sp<android::Fence> outPresentFence;
+ sp<Fence> outPresentFence;
uint32_t state = UINT32_MAX;
error = hwcDisplay->presentOrValidate(&numTypes, &numRequests, &outPresentFence , &state);
if (error != HWC2::Error::HasChanges) {
@@ -689,7 +668,6 @@
const auto hwcDisplayId = displayData.hwcDisplay->getId();
mPhysicalDisplayIdMap.erase(hwcDisplayId);
mDisplayData.erase(displayId);
- mVSyncCounts.erase(displayId);
// TODO(b/74619554): Select internal/external display from remaining displays.
if (hwcDisplayId == mInternalHwcDisplayId) {
@@ -755,26 +733,6 @@
return matrix;
}
-// Converts a PixelFormat to a human-readable string. Max 11 chars.
-// (Could use a table of prefab String8 objects.)
-/*
-static String8 getFormatStr(PixelFormat format) {
- switch (format) {
- case PIXEL_FORMAT_RGBA_8888: return String8("RGBA_8888");
- case PIXEL_FORMAT_RGBX_8888: return String8("RGBx_8888");
- case PIXEL_FORMAT_RGB_888: return String8("RGB_888");
- case PIXEL_FORMAT_RGB_565: return String8("RGB_565");
- case PIXEL_FORMAT_BGRA_8888: return String8("BGRA_8888");
- case HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED:
- return String8("ImplDef");
- default:
- String8 result;
- result.appendFormat("? %08x", format);
- return result;
- }
-}
-*/
-
bool HWComposer::isUsingVrComposer() const {
return getComposer()->isUsingVrComposer();
}
diff --git a/services/surfaceflinger/DisplayHardware/HWComposer.h b/services/surfaceflinger/DisplayHardware/HWComposer.h
index b78433d..2f57907 100644
--- a/services/surfaceflinger/DisplayHardware/HWComposer.h
+++ b/services/surfaceflinger/DisplayHardware/HWComposer.h
@@ -17,49 +17,26 @@
#ifndef ANDROID_SF_HWCOMPOSER_H
#define ANDROID_SF_HWCOMPOSER_H
-#include "HWC2.h"
-
-#include <stdint.h>
-#include <sys/types.h>
-
-#include <ui/Fence.h>
-#include <ui/GraphicTypes.h>
-#include <utils/BitSet.h>
-#include <utils/Condition.h>
-#include <utils/Mutex.h>
-#include <utils/StrongPointer.h>
-#include <utils/Thread.h>
-#include <utils/Timers.h>
-#include <utils/Vector.h>
-
+#include <cstdint>
#include <memory>
+#include <mutex>
#include <optional>
#include <unordered_map>
#include <unordered_set>
#include <vector>
+#include <android-base/thread_annotations.h>
+#include <ui/Fence.h>
+#include <ui/GraphicTypes.h>
+#include <utils/StrongPointer.h>
+#include <utils/Timers.h>
+
#include "DisplayIdentification.h"
-
-extern "C" int clock_nanosleep(clockid_t clock_id, int flags,
- const struct timespec *request,
- struct timespec *remain);
-
-struct framebuffer_device_t;
-
-namespace HWC2 {
- class Device;
- class Display;
-}
+#include "HWC2.h"
namespace android {
-// ---------------------------------------------------------------------------
-class DisplayDevice;
-class Fence;
-class FloatRect;
class GraphicBuffer;
-class NativeHandle;
-class Region;
class String8;
class TestableSurfaceFlinger;
struct CompositionInfo;
@@ -71,7 +48,7 @@
class HWComposer
{
public:
- explicit HWComposer(std::unique_ptr<android::Hwc2::Composer> composer);
+ explicit HWComposer(std::unique_ptr<Hwc2::Composer> composer);
~HWComposer();
@@ -177,7 +154,7 @@
// for debugging ----------------------------------------------------------
void dump(String8& out) const;
- android::Hwc2::Composer* getComposer() const { return mHwcDevice->getComposer(); }
+ Hwc2::Composer* getComposer() const { return mHwcDevice->getComposer(); }
// TODO(b/74619554): Remove special cases for internal/external display.
std::optional<hwc2_display_t> getInternalHwcDisplayId() const { return mInternalHwcDisplayId; }
@@ -194,8 +171,6 @@
static void validateChange(HWC2::Composition from, HWC2::Composition to);
- struct cb_context;
-
struct DisplayData {
bool isVirtual = false;
bool hasClientComposition = false;
@@ -209,11 +184,16 @@
mutable std::unordered_map<int32_t,
std::shared_ptr<const HWC2::Display::Config>> configMap;
- // protected by mVsyncLock
- HWC2::Vsync vsyncEnabled = HWC2::Vsync::Disable;
-
bool validateWasSkipped;
HWC2::Error presentError;
+
+ bool vsyncTraceToggle = false;
+
+ std::mutex vsyncEnabledLock;
+ HWC2::Vsync vsyncEnabled GUARDED_BY(vsyncEnabledLock) = HWC2::Vsync::Disable;
+
+ mutable std::mutex lastHwVsyncLock;
+ nsecs_t lastHwVsync GUARDED_BY(lastHwVsyncLock) = 0;
};
std::unordered_map<DisplayId, DisplayData> mDisplayData;
@@ -227,25 +207,11 @@
std::optional<hwc2_display_t> mExternalHwcDisplayId;
bool mHasMultiDisplaySupport = false;
- // protect mDisplayData from races between prepare and dump
- mutable Mutex mDisplayLock;
-
- cb_context* mCBContext = nullptr;
- std::unordered_map<DisplayId, size_t> mVSyncCounts;
-
std::unordered_set<DisplayId> mFreeVirtualDisplayIds;
uint32_t mNextVirtualDisplayId = 0;
uint32_t mRemainingHwcVirtualDisplays{mHwcDevice->getMaxVirtualDisplayCount()};
-
- // protected by mLock
- mutable Mutex mLock;
- mutable std::unordered_map<DisplayId, nsecs_t> mLastHwVSync;
-
- // thread-safe
- mutable Mutex mVsyncLock;
};
-// ---------------------------------------------------------------------------
-}; // namespace android
+} // namespace android
#endif // ANDROID_SF_HWCOMPOSER_H
diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp b/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
index ac6a78b..f6dd689 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTest.cpp
@@ -1270,7 +1270,7 @@
// Insert display data so that the HWC thinks it created the virtual display.
const auto displayId = Case::Display::DISPLAY_ID::get();
ASSERT_TRUE(displayId);
- mFlinger.mutableHwcDisplayData()[*displayId] = {};
+ mFlinger.mutableHwcDisplayData().try_emplace(*displayId);
setupNewDisplayDeviceInternalTest<Case>();
}
@@ -1783,7 +1783,7 @@
// A virtual display is set up but is removed from the current state.
const auto displayId = Case::Display::DISPLAY_ID::get();
ASSERT_TRUE(displayId);
- mFlinger.mutableHwcDisplayData()[*displayId] = {};
+ mFlinger.mutableHwcDisplayData().try_emplace(*displayId);
Case::Display::injectHwcDisplay(this);
auto existing = Case::Display::makeFakeExistingDisplayInjector(this);
existing.inject();
@@ -2908,7 +2908,7 @@
// Insert display data so that the HWC thinks it created the virtual display.
const auto displayId = Case::Display::DISPLAY_ID::get();
ASSERT_TRUE(displayId);
- mFlinger.mutableHwcDisplayData()[*displayId] = {};
+ mFlinger.mutableHwcDisplayData().try_emplace(*displayId);
// A virtual display device is set up
Case::Display::injectHwcDisplay(this);