SF: Query display modes only once during display creation / update
Currently HWComposer::getActiveMode / getModes is called multiple
times when creating or updating a display. This is unnecessary
and risky because the getActiveMode may return a different value
if a hotplug event has ocured in between.
In this CL we call getActiveMode and getModes only once and propagate
the values using DisplayDeviceState.
Bug: 159590486
Test: presubmit
Change-Id: I97418163426a0ae19f79bc2912a7d725c953ad01
diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h
index 85f2f3b..6f07964 100644
--- a/services/surfaceflinger/DisplayDevice.h
+++ b/services/surfaceflinger/DisplayDevice.h
@@ -219,6 +219,9 @@
DisplayConnectionType type;
hardware::graphics::composer::hal::HWDisplayId hwcDisplayId;
std::optional<DeviceProductInfo> deviceProductInfo;
+ DisplayModes supportedModes;
+ DisplayModePtr activeMode;
+
bool operator==(const Physical& other) const {
return id == other.id && type == other.type && hwcDisplayId == other.hwcDisplayId;
}
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index a764cbb..47d8826 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -155,7 +155,6 @@
using ui::ColorMode;
using ui::Dataspace;
using ui::DisplayPrimaries;
-using ui::Hdr;
using ui::RenderIntent;
namespace hal = android::hardware::graphics::composer::hal;
@@ -1130,7 +1129,7 @@
clearDesiredActiveConfigState();
- const auto& refreshRate = getDefaultDisplayDeviceLocked()->getMode(modeId)->getFps();
+ const auto refreshRate = getDefaultDisplayDeviceLocked()->getMode(modeId)->getFps();
mScheduler->resyncToHardwareVsync(true, refreshRate.getPeriodNsecs());
updatePhaseConfiguration(refreshRate);
}
@@ -2368,18 +2367,20 @@
const auto it = mPhysicalDisplayTokens.find(displayId);
if (event.connection == hal::Connection::CONNECTED) {
+ auto supportedModes = getHwComposer().getModes(displayId);
+ const auto activeMode = getHwComposer().getActiveMode(displayId);
+ // TODO(b/175678215) Handle the case when activeMode is not in supportedModes
+
if (it == mPhysicalDisplayTokens.end()) {
ALOGV("Creating display %s", to_string(displayId).c_str());
- if (event.hwcDisplayId == getHwComposer().getInternalHwcDisplayId()) {
- initScheduler(displayId);
- }
-
DisplayDeviceState state;
state.physical = {.id = displayId,
.type = getHwComposer().getDisplayConnectionType(displayId),
.hwcDisplayId = event.hwcDisplayId,
- .deviceProductInfo = std::move(info->deviceProductInfo)};
+ .deviceProductInfo = std::move(info->deviceProductInfo),
+ .supportedModes = std::move(supportedModes),
+ .activeMode = activeMode};
state.isSecure = true; // All physical displays are currently considered secure.
state.displayName = std::move(info->name);
@@ -2387,6 +2388,10 @@
mCurrentState.displays.add(token, state);
mPhysicalDisplayTokens.emplace(displayId, std::move(token));
+ if (event.hwcDisplayId == getHwComposer().getInternalHwcDisplayId()) {
+ initScheduler(state);
+ }
+
mInterceptor->saveDisplayCreation(state);
} else {
ALOGV("Recreating display %s", to_string(displayId).c_str());
@@ -2394,6 +2399,8 @@
const auto token = it->second;
auto& state = mCurrentState.displays.editValueFor(token);
state.sequenceId = DisplayDeviceState{}.sequenceId; // Generate new sequenceId
+ state.physical->supportedModes = std::move(supportedModes);
+ state.physical->activeMode = activeMode;
if (getHwComposer().updatesDeviceProductInfoOnHotplugReconnect()) {
state.physical->deviceProductInfo = std::move(info->deviceProductInfo);
}
@@ -2439,11 +2446,11 @@
if (const auto& physical = state.physical) {
creationArgs.connectionType = physical->type;
+ creationArgs.supportedModes = physical->supportedModes;
}
if (const auto id = PhysicalDisplayId::tryCast(compositionDisplay->getId())) {
creationArgs.isPrimary = id == getInternalDisplayIdLocked();
- creationArgs.supportedModes = getHwComposer().getModes(*id);
if (useColorManagement) {
std::vector<ColorMode> modes = getHwComposer().getColorModes(*id);
@@ -2498,9 +2505,7 @@
RenderIntent::COLORIMETRIC,
Dataspace::UNKNOWN});
if (!state.isVirtual()) {
- const auto physicalId = display->getPhysicalId();
- auto activeConfigId = getHwComposer().getActiveMode(physicalId)->getId();
- display->setActiveMode(activeConfigId);
+ display->setActiveMode(state.physical->activeMode->getId());
display->setDeviceProductInfo(state.physical->deviceProductInfo);
}
@@ -2518,10 +2523,8 @@
int height = 0;
ui::PixelFormat pixelFormat = static_cast<ui::PixelFormat>(PIXEL_FORMAT_UNKNOWN);
if (state.physical) {
- const auto& activeConfig =
- getCompositionEngine().getHwComposer().getActiveMode(state.physical->id);
- width = activeConfig->getWidth();
- height = activeConfig->getHeight();
+ width = state.physical->activeMode->getWidth();
+ height = state.physical->activeMode->getHeight();
pixelFormat = static_cast<ui::PixelFormat>(PIXEL_FORMAT_RGBA_8888);
} else if (state.surface != nullptr) {
int status = state.surface->query(NATIVE_WINDOW_WIDTH, &width);
@@ -2645,11 +2648,9 @@
// TODO(b/175678251) Call a listener instead.
if (currentState.physical->hwcDisplayId == getHwComposer().getInternalHwcDisplayId()) {
- const auto displayId = currentState.physical->id;
- const auto configs = getHwComposer().getModes(displayId);
- const auto currentConfig = getHwComposer().getActiveMode(displayId)->getId();
- // TODO(b/175678215) Handle the case when currentConfig is not in configs
- mRefreshRateConfigs->updateDisplayConfigs(configs, currentConfig);
+ mRefreshRateConfigs
+ ->updateDisplayConfigs(currentState.physical->supportedModes,
+ currentState.physical->activeMode->getId());
mVsyncConfiguration->reset();
updatePhaseConfiguration(mRefreshRateConfigs->getCurrentRefreshRate().getFps());
if (mRefreshRateOverlay) {
@@ -2932,31 +2933,29 @@
mScheduler->onFrameRateOverridesChanged(mAppConnectionHandle, displayId);
}
-void SurfaceFlinger::initScheduler(PhysicalDisplayId primaryDisplayId) {
+void SurfaceFlinger::initScheduler(const DisplayDeviceState& displayState) {
if (mScheduler) {
// In practice it's not allowed to hotplug in/out the primary display once it's been
// connected during startup, but some tests do it, so just warn and return.
ALOGW("Can't re-init scheduler");
return;
}
-
- auto currentConfig = getHwComposer().getActiveMode(primaryDisplayId)->getId();
- const auto modes = getHwComposer().getModes(primaryDisplayId);
+ const auto displayId = displayState.physical->id;
mRefreshRateConfigs = std::make_unique<
- scheduler::RefreshRateConfigs>(modes, currentConfig,
+ scheduler::RefreshRateConfigs>(displayState.physical->supportedModes,
+ displayState.physical->activeMode->getId(),
android::sysprop::enable_frame_rate_override(true));
- const auto& currRefreshRate = mRefreshRateConfigs->getRefreshRateFromConfigId(currentConfig);
- mRefreshRateStats =
- std::make_unique<scheduler::RefreshRateStats>(*mTimeStats, currRefreshRate.getFps(),
- hal::PowerMode::OFF);
+ const auto currRefreshRate = displayState.physical->activeMode->getFps();
+ mRefreshRateStats = std::make_unique<scheduler::RefreshRateStats>(*mTimeStats, currRefreshRate,
+ hal::PowerMode::OFF);
- mVsyncConfiguration = getFactory().createVsyncConfiguration(currRefreshRate.getFps());
+ mVsyncConfiguration = getFactory().createVsyncConfiguration(currRefreshRate);
mVsyncModulator.emplace(mVsyncConfiguration->getCurrentConfigs());
// start the EventThread
mScheduler = getFactory().createScheduler(*mRefreshRateConfigs, *this);
const auto configs = mVsyncConfiguration->getCurrentConfigs();
- const nsecs_t vsyncPeriod = currRefreshRate.getVsyncPeriod();
+ const nsecs_t vsyncPeriod = currRefreshRate.getPeriodNsecs();
mAppConnectionHandle =
mScheduler->createConnection("app", mFrameTimeline->getTokenManager(),
/*workDuration=*/configs.late.appWorkDuration,
@@ -2983,7 +2982,8 @@
// This is a bit hacky, but this avoids a back-pointer into the main SF
// classes from EventThread, and there should be no run-time binder cost
// anyway since there are no connected apps at this point.
- mScheduler->onPrimaryDisplayConfigChanged(mAppConnectionHandle, primaryDisplayId, currentConfig,
+ mScheduler->onPrimaryDisplayConfigChanged(mAppConnectionHandle, displayId,
+ displayState.physical->activeMode->getId(),
vsyncPeriod);
static auto ignorePresentFences =
base::GetBoolProperty("debug.sf.vsync_reactor_ignore_present_fences"s, false);
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 6c91fbd..9b4c18d 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -714,7 +714,7 @@
void commitInputWindowCommands() REQUIRES(mStateLock);
void updateCursorAsync();
- void initScheduler(PhysicalDisplayId primaryDisplayId) REQUIRES(mStateLock);
+ void initScheduler(const DisplayDeviceState&) REQUIRES(mStateLock);
void updatePhaseConfiguration(const Fps&) REQUIRES(mStateLock);
void setVsyncConfig(const VsyncModulator::VsyncConfig&, nsecs_t vsyncPeriod);
diff --git a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
index 9ec2d16..1664301 100644
--- a/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
+++ b/services/surfaceflinger/tests/unittests/DisplayTransactionTestHelpers.h
@@ -136,7 +136,7 @@
DisplayTransactionTest();
};
-constexpr int32_t DEFAULT_REFRESH_RATE = 16'666'667;
+constexpr int32_t DEFAULT_VSYNC_PERIOD = 16'666'667;
constexpr int32_t DEFAULT_DPI = 320;
constexpr int DEFAULT_VIRTUAL_DISPLAY_SURFACE_FORMAT = HAL_PIXEL_FORMAT_RGB_565;
@@ -436,7 +436,7 @@
getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,
IComposerClient::Attribute::VSYNC_PERIOD, _))
.WillRepeatedly(
- DoAll(SetArgPointee<3>(DEFAULT_REFRESH_RATE), Return(Error::NONE)));
+ DoAll(SetArgPointee<3>(DEFAULT_VSYNC_PERIOD), Return(Error::NONE)));
EXPECT_CALL(*test->mComposer,
getDisplayAttribute(HWC_DISPLAY_ID, HWC_ACTIVE_CONFIG_ID,
IComposerClient::Attribute::DPI_X, _))
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_OnInitializeDisplaysTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_OnInitializeDisplaysTest.cpp
index 7a9403b..ef8b149 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_OnInitializeDisplaysTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_OnInitializeDisplaysTest.cpp
@@ -86,16 +86,16 @@
// The display refresh period should be set in the orientedDisplaySpaceRect tracker.
FrameStats stats;
mFlinger.getAnimFrameTracker().getStats(&stats);
- EXPECT_EQ(DEFAULT_REFRESH_RATE, stats.refreshPeriodNano);
+ EXPECT_EQ(DEFAULT_VSYNC_PERIOD, stats.refreshPeriodNano);
// The display transaction needed flag should be set.
EXPECT_TRUE(hasTransactionFlagSet(eDisplayTransactionNeeded));
// The compositor timing should be set to default values
const auto& compositorTiming = mFlinger.getCompositorTiming();
- EXPECT_EQ(-DEFAULT_REFRESH_RATE, compositorTiming.deadline);
- EXPECT_EQ(DEFAULT_REFRESH_RATE, compositorTiming.interval);
- EXPECT_EQ(DEFAULT_REFRESH_RATE, compositorTiming.presentLatency);
+ EXPECT_EQ(-DEFAULT_VSYNC_PERIOD, compositorTiming.deadline);
+ EXPECT_EQ(DEFAULT_VSYNC_PERIOD, compositorTiming.interval);
+ EXPECT_EQ(DEFAULT_VSYNC_PERIOD, compositorTiming.presentLatency);
}
} // namespace
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp
index 2117628..6502420 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetPowerModeInternalTest.cpp
@@ -105,7 +105,7 @@
struct DispSyncIsSupportedVariant {
static void setupResetModelCallExpectations(DisplayTransactionTest* test) {
- EXPECT_CALL(*test->mVsyncController, startPeriodTransition(DEFAULT_REFRESH_RATE)).Times(1);
+ EXPECT_CALL(*test->mVsyncController, startPeriodTransition(DEFAULT_VSYNC_PERIOD)).Times(1);
EXPECT_CALL(*test->mVSyncTracker, resetModel()).Times(1);
}
};
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp
index 9a95ab4..2b5cb77 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp
@@ -17,6 +17,8 @@
#undef LOG_TAG
#define LOG_TAG "LibSurfaceFlingerUnittests"
+#include "DisplayHardware/DisplayMode.h"
+
#include "DisplayTransactionTestHelpers.h"
namespace android {
@@ -232,13 +234,26 @@
// Invocation
DisplayDeviceState state;
- if (const auto connectionType = Case::Display::CONNECTION_TYPE::value) {
+ if constexpr (constexpr auto connectionType = Case::Display::CONNECTION_TYPE::value) {
const auto displayId = PhysicalDisplayId::tryCast(Case::Display::DISPLAY_ID::get());
ASSERT_TRUE(displayId);
const auto hwcDisplayId = Case::Display::HWC_DISPLAY_ID_OPT::value;
ASSERT_TRUE(hwcDisplayId);
mFlinger.getHwComposer().allocatePhysicalDisplay(*hwcDisplayId, *displayId);
- state.physical = {.id = *displayId, .type = *connectionType, .hwcDisplayId = *hwcDisplayId};
+ DisplayModePtr activeMode = DisplayMode::Builder(Case::Display::HWC_ACTIVE_CONFIG_ID)
+ .setWidth(Case::Display::WIDTH)
+ .setHeight(Case::Display::HEIGHT)
+ .setVsyncPeriod(DEFAULT_VSYNC_PERIOD)
+ .setDpiX(DEFAULT_DPI)
+ .setDpiY(DEFAULT_DPI)
+ .setConfigGroup(0)
+ .build();
+ DisplayModes modes{activeMode};
+ state.physical = {.id = *displayId,
+ .type = *connectionType,
+ .hwcDisplayId = *hwcDisplayId,
+ .supportedModes = modes,
+ .activeMode = activeMode};
}
state.isSecure = static_cast<bool>(Case::Display::SECURE);