SF: Deduplicate state for active display mode
RefreshRateConfigs tracks the active mode, so remove the duplicate (and
not thread-safe) state in DisplayDevice.
Bug: 241285191
Test: Build (-Wthread-safety)
Change-Id: I6b551cc68da3916a797a28085be984667fa1901e
diff --git a/services/surfaceflinger/DisplayDevice.cpp b/services/surfaceflinger/DisplayDevice.cpp
index b823e06..a25296c 100644
--- a/services/surfaceflinger/DisplayDevice.cpp
+++ b/services/surfaceflinger/DisplayDevice.cpp
@@ -192,17 +192,16 @@
}
void DisplayDevice::setActiveMode(DisplayModeId modeId, const display::DisplaySnapshot& snapshot) {
- const auto modeOpt = snapshot.displayModes().get(modeId);
- LOG_ALWAYS_FATAL_IF(!modeOpt, "Unknown mode");
+ const auto fpsOpt = snapshot.displayModes().get(modeId).transform(
+ [](const DisplayModePtr& mode) { return mode->getFps(); });
- mActiveMode = modeOpt->get();
- const Fps fps = mActiveMode->getFps();
+ LOG_ALWAYS_FATAL_IF(!fpsOpt, "Unknown mode");
+ const Fps fps = *fpsOpt;
ATRACE_INT(mActiveModeFPSTrace.c_str(), fps.getIntValue());
- if (mRefreshRateConfigs) {
- mRefreshRateConfigs->setActiveModeId(modeId);
- }
+ mRefreshRateConfigs->setActiveModeId(modeId);
+
if (mRefreshRateOverlay) {
mRefreshRateOverlay->changeRefreshRate(fps);
}
@@ -224,10 +223,6 @@
constraints, outTimeline);
}
-const DisplayModePtr& DisplayDevice::getActiveMode() const {
- return mActiveMode;
-}
-
nsecs_t DisplayDevice::getVsyncPeriodFromHWC() const {
const auto physicalId = getPhysicalId();
if (!mHwComposer.isConnected(physicalId)) {
@@ -240,7 +235,7 @@
return vsyncPeriod;
}
- return getActiveMode()->getFps().getPeriodNsecs();
+ return refreshRateConfigs().getActiveModePtr()->getVsyncPeriod();
}
nsecs_t DisplayDevice::getRefreshTimestamp() const {
@@ -451,7 +446,7 @@
mRefreshRateOverlay = std::make_unique<RefreshRateOverlay>(fpsRange, showSpinnner);
mRefreshRateOverlay->setLayerStack(getLayerStack());
mRefreshRateOverlay->setViewport(getSize());
- mRefreshRateOverlay->changeRefreshRate(getActiveMode()->getFps());
+ mRefreshRateOverlay->changeRefreshRate(getActiveMode().getFps());
}
bool DisplayDevice::onKernelTimerChanged(std::optional<DisplayModeId> desiredModeId,
@@ -492,7 +487,7 @@
}
// Check if we are already at the desired mode
- if (getActiveMode()->getId() == info.mode->getId()) {
+ if (refreshRateConfigs().getActiveModePtr()->getId() == info.mode->getId()) {
return false;
}
diff --git a/services/surfaceflinger/DisplayDevice.h b/services/surfaceflinger/DisplayDevice.h
index d79a6b5..0f52aff 100644
--- a/services/surfaceflinger/DisplayDevice.h
+++ b/services/surfaceflinger/DisplayDevice.h
@@ -189,8 +189,6 @@
/* ------------------------------------------------------------------------
* Display mode management.
*/
- const DisplayModePtr& getActiveMode() const;
-
struct ActiveModeInfo {
DisplayModePtr mode;
scheduler::DisplayModeEvent event = scheduler::DisplayModeEvent::None;
@@ -207,6 +205,10 @@
return mUpcomingActiveMode;
}
+ const DisplayMode& getActiveMode() const REQUIRES(kMainThreadContext) {
+ return mRefreshRateConfigs->getActiveMode();
+ }
+
// Precondition: DisplaySnapshot must contain a mode with DisplayModeId.
void setActiveMode(DisplayModeId, const display::DisplaySnapshot&) REQUIRES(kMainThreadContext);
@@ -226,7 +228,7 @@
}
// Enables an overlay to be displayed with the current refresh rate
- void enableRefreshRateOverlay(bool enable, bool showSpinner);
+ void enableRefreshRateOverlay(bool enable, bool showSpinner) REQUIRES(kMainThreadContext);
bool isRefreshRateOverlayEnabled() const { return mRefreshRateOverlay != nullptr; }
bool onKernelTimerChanged(std::optional<DisplayModeId>, bool timerExpired);
void animateRefreshRateOverlay();
@@ -265,10 +267,10 @@
static ui::Transform::RotationFlags sPrimaryDisplayRotationFlags;
- // allow initial power mode as null.
+ // Allow nullopt as initial power mode.
std::optional<hardware::graphics::composer::hal::PowerMode> mPowerMode;
- DisplayModePtr mActiveMode;
- std::optional<float> mStagedBrightness = std::nullopt;
+
+ std::optional<float> mStagedBrightness;
float mBrightness = -1.f;
std::atomic<nsecs_t> mLastHwVsync = 0;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index bcd2b43..55a54fe 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -680,7 +680,7 @@
sp<IBinder> input(defaultServiceManager()->getService(String16("inputflinger")));
- static_cast<void>(mScheduler->schedule([=] {
+ static_cast<void>(mScheduler->schedule([=]() FTL_FAKE_GUARD(kMainThreadContext) {
if (input == nullptr) {
ALOGE("Failed to link to input service");
} else {
@@ -708,8 +708,9 @@
mBootStage = BootStage::FINISHED;
- if (property_get_bool("sf.debug.show_refresh_rate_overlay", false)) {
- FTL_FAKE_GUARD(mStateLock, enableRefreshRateOverlay(true));
+ if (base::GetBoolProperty("sf.debug.show_refresh_rate_overlay"s, false)) {
+ ftl::FakeGuard guard(mStateLock);
+ enableRefreshRateOverlay(true);
}
}));
}
@@ -1059,7 +1060,7 @@
const PhysicalDisplayId displayId = snapshot.displayId();
- info->activeDisplayModeId = display->getActiveMode()->getId().value();
+ info->activeDisplayModeId = display->refreshRateConfigs().getActiveModePtr()->getId().value();
info->activeColorMode = display->getCompositionDisplay()->getState().colorMode;
info->supportedColorModes = getDisplayColorModes(displayId);
info->hdrCapabilities = display->getHdrCapabilities();
@@ -1183,7 +1184,7 @@
return;
}
- if (display->getActiveMode()->getResolution() != upcomingModeInfo.mode->getResolution()) {
+ if (display->getActiveMode().getResolution() != upcomingModeInfo.mode->getResolution()) {
auto& state = mCurrentState.displays.editValueFor(display->getDisplayToken());
// We need to generate new sequenceId in order to recreate the display (and this
// way the framebuffer).
@@ -1269,7 +1270,7 @@
ALOGV("%s changing active mode to %d(%s) for display %s", __func__, desiredModeId.value(),
to_string(*refreshRateOpt).c_str(), to_string(display->getId()).c_str());
- if (display->getActiveMode()->getId() == desiredModeId) {
+ if (display->getActiveMode().getId() == desiredModeId) {
// we are already in the requested mode, there is nothing left to do
desiredActiveModeChangeDone(display);
continue;
@@ -1318,7 +1319,7 @@
const auto display = getDisplayDeviceLocked(*displayToUpdateImmediately);
const auto desiredActiveMode = display->getDesiredActiveMode();
if (desiredActiveMode &&
- display->getActiveMode()->getId() == desiredActiveMode->mode->getId()) {
+ display->getActiveMode().getId() == desiredActiveMode->mode->getId()) {
desiredActiveModeChangeDone(display);
}
}
@@ -2107,7 +2108,7 @@
activeDisplay->getPowerMode() == hal::PowerMode::ON;
if (mPowerHintSessionEnabled) {
const auto& display = FTL_FAKE_GUARD(mStateLock, getDefaultDisplayDeviceLocked()).get();
- const Period vsyncPeriod = Period::fromNs(display->getActiveMode()->getVsyncPeriod());
+ const Period vsyncPeriod = Period::fromNs(display->getActiveMode().getVsyncPeriod());
mPowerAdvisor->setCommitStart(frameTime);
mPowerAdvisor->setExpectedPresentTime(mExpectedPresentTime);
@@ -3084,7 +3085,7 @@
void SurfaceFlinger::updateInternalDisplayVsyncLocked(const sp<DisplayDevice>& activeDisplay) {
mVsyncConfiguration->reset();
- const Fps refreshRate = activeDisplay->refreshRateConfigs().getActiveMode().getFps();
+ const Fps refreshRate = activeDisplay->getActiveMode().getFps();
updatePhaseConfiguration(refreshRate);
mRefreshRateStats->setRefreshRate(refreshRate);
}
@@ -3394,11 +3395,13 @@
void SurfaceFlinger::initScheduler(const sp<const DisplayDevice>& display) {
LOG_ALWAYS_FATAL_IF(mScheduler);
- const auto currRefreshRate = display->getActiveMode()->getFps();
- mRefreshRateStats = std::make_unique<scheduler::RefreshRateStats>(*mTimeStats, currRefreshRate,
- hal::PowerMode::OFF);
+ const auto activeModePtr = display->refreshRateConfigs().getActiveModePtr();
+ const Fps activeRefreshRate = activeModePtr->getFps();
+ mRefreshRateStats =
+ std::make_unique<scheduler::RefreshRateStats>(*mTimeStats, activeRefreshRate,
+ hal::PowerMode::OFF);
- mVsyncConfiguration = getFactory().createVsyncConfiguration(currRefreshRate);
+ mVsyncConfiguration = getFactory().createVsyncConfiguration(activeRefreshRate);
mVsyncModulator = sp<VsyncModulator>::make(mVsyncConfiguration->getCurrentConfigs());
using Feature = scheduler::Feature;
@@ -3431,7 +3434,7 @@
mScheduler->startTimers();
const auto configs = mVsyncConfiguration->getCurrentConfigs();
- const nsecs_t vsyncPeriod = currRefreshRate.getPeriodNsecs();
+ const nsecs_t vsyncPeriod = activeRefreshRate.getPeriodNsecs();
mAppConnectionHandle =
mScheduler->createConnection("app", mFrameTimeline->getTokenManager(),
/*workDuration=*/configs.late.appWorkDuration,
@@ -3459,7 +3462,7 @@
// 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->onPrimaryDisplayModeChanged(mAppConnectionHandle, display->getActiveMode());
+ mScheduler->onPrimaryDisplayModeChanged(mAppConnectionHandle, activeModePtr);
}
void SurfaceFlinger::updatePhaseConfiguration(const Fps& refreshRate) {
@@ -5348,10 +5351,10 @@
if (const auto display = getDefaultDisplayDeviceLocked()) {
std::string fps, xDpi, yDpi;
- if (const auto activeMode = display->getActiveMode()) {
- fps = to_string(activeMode->getFps());
+ if (const auto activeModePtr = display->refreshRateConfigs().getActiveModePtr()) {
+ fps = to_string(activeModePtr->getFps());
- const auto dpi = activeMode->getDpi();
+ const auto dpi = activeModePtr->getDpi();
xDpi = base::StringPrintf("%.2f", dpi.x);
yDpi = base::StringPrintf("%.2f", dpi.y);
} else {
@@ -5851,19 +5854,17 @@
return NO_ERROR;
}
case 1034: {
- auto future = mScheduler->schedule([&] {
- switch (n = data.readInt32()) {
- case 0:
- case 1:
- FTL_FAKE_GUARD(mStateLock,
- enableRefreshRateOverlay(static_cast<bool>(n)));
- break;
- default: {
- reply->writeBool(
- FTL_FAKE_GUARD(mStateLock, isRefreshRateOverlayEnabled()));
- }
- }
- });
+ auto future = mScheduler->schedule(
+ [&]() FTL_FAKE_GUARD(mStateLock) FTL_FAKE_GUARD(kMainThreadContext) {
+ switch (n = data.readInt32()) {
+ case 0:
+ case 1:
+ enableRefreshRateOverlay(static_cast<bool>(n));
+ break;
+ default:
+ reply->writeBool(isRefreshRateOverlayEnabled());
+ }
+ });
future.wait();
return NO_ERROR;
@@ -6799,12 +6800,12 @@
// TODO(b/140204874): Leave the event in until we do proper testing with all apps that might
// be depending in this callback.
- const auto activeMode = display->getActiveMode();
+ const auto activeModePtr = display->refreshRateConfigs().getActiveModePtr();
if (isDisplayActiveLocked(display)) {
- mScheduler->onPrimaryDisplayModeChanged(mAppConnectionHandle, activeMode);
+ mScheduler->onPrimaryDisplayModeChanged(mAppConnectionHandle, activeModePtr);
toggleKernelIdleTimer();
} else {
- mScheduler->onNonPrimaryDisplayModeChanged(mAppConnectionHandle, activeMode);
+ mScheduler->onNonPrimaryDisplayModeChanged(mAppConnectionHandle, activeModePtr);
}
auto preferredModeOpt =
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 1f8874d..fdaacf0 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -1340,7 +1340,7 @@
std::unique_ptr<Hwc2::PowerAdvisor> mPowerAdvisor;
- void enableRefreshRateOverlay(bool enable) REQUIRES(mStateLock);
+ void enableRefreshRateOverlay(bool enable) REQUIRES(mStateLock, kMainThreadContext);
// Flag used to set override desired display mode from backdoor
bool mDebugDisplayModeSetByBackdoor = false;
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
index 57937dc..6b7e353 100644
--- a/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
+++ b/services/surfaceflinger/tests/unittests/SurfaceFlinger_DisplayModeSwitching.cpp
@@ -20,6 +20,7 @@
#include "DisplayTransactionTestHelpers.h"
+#include <ftl/fake_guard.h>
#include <scheduler/Fps.h>
namespace android {
@@ -111,8 +112,10 @@
}
TEST_F(DisplayModeSwitchingTest, changeRefreshRate_OnActiveDisplay_WithRefreshRequired) {
+ ftl::FakeGuard guard(kMainThreadContext);
+
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId60);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId60);
mFlinger.onActiveDisplayChanged(mDisplay);
@@ -121,7 +124,7 @@
ASSERT_TRUE(mDisplay->getDesiredActiveMode().has_value());
ASSERT_EQ(mDisplay->getDesiredActiveMode()->mode->getId(), kModeId90);
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId60);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId60);
// Verify that next commit will call setActiveConfigWithConstraints in HWC
const VsyncPeriodChangeTimeline timeline{.refreshRequired = true};
@@ -134,7 +137,7 @@
Mock::VerifyAndClearExpectations(mComposer);
ASSERT_TRUE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId60);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId60);
// Verify that the next commit will complete the mode change and send
// a onModeChanged event to the framework.
@@ -144,10 +147,12 @@
Mock::VerifyAndClearExpectations(mAppEventThread);
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId90);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId90);
}
TEST_F(DisplayModeSwitchingTest, changeRefreshRate_OnActiveDisplay_WithoutRefreshRequired) {
+ ftl::FakeGuard guard(kMainThreadContext);
+
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
mFlinger.onActiveDisplayChanged(mDisplay);
@@ -157,7 +162,7 @@
ASSERT_TRUE(mDisplay->getDesiredActiveMode().has_value());
ASSERT_EQ(mDisplay->getDesiredActiveMode()->mode->getId(), kModeId90);
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId60);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId60);
// Verify that next commit will call setActiveConfigWithConstraints in HWC
// and complete the mode change.
@@ -172,15 +177,17 @@
mFlinger.commit();
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId90);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId90);
}
TEST_F(DisplayModeSwitchingTest, twoConsecutiveSetDesiredDisplayModeSpecs) {
+ ftl::FakeGuard guard(kMainThreadContext);
+
// Test that if we call setDesiredDisplayModeSpecs while a previous mode change
// is still being processed the later call will be respected.
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId60);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId60);
mFlinger.onActiveDisplayChanged(mDisplay);
@@ -214,12 +221,14 @@
mFlinger.commit();
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId120);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId120);
}
TEST_F(DisplayModeSwitchingTest, changeResolution_OnActiveDisplay_WithoutRefreshRequired) {
+ ftl::FakeGuard guard(kMainThreadContext);
+
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId60);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId60);
mFlinger.onActiveDisplayChanged(mDisplay);
@@ -228,7 +237,7 @@
ASSERT_TRUE(mDisplay->getDesiredActiveMode().has_value());
ASSERT_EQ(mDisplay->getDesiredActiveMode()->mode->getId(), kModeId90_4K);
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId60);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId60);
// Verify that next commit will call setActiveConfigWithConstraints in HWC
// and complete the mode change.
@@ -263,7 +272,7 @@
mDisplay = mFlinger.getDisplay(displayToken);
ASSERT_FALSE(mDisplay->getDesiredActiveMode().has_value());
- ASSERT_EQ(mDisplay->getActiveMode()->getId(), kModeId90_4K);
+ ASSERT_EQ(mDisplay->getActiveMode().getId(), kModeId90_4K);
}
} // namespace
diff --git a/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp b/services/surfaceflinger/tests/unittests/SurfaceFlinger_SetupNewDisplayDeviceInternalTest.cpp
index ec2c2b4..073c459 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 <ftl/fake_guard.h>
+
#include "DisplayHardware/DisplayMode.h"
#include "DisplayTransactionTestHelpers.h"
@@ -265,7 +267,7 @@
// --------------------------------------------------------------------
// Postconditions
- ASSERT_TRUE(device != nullptr);
+ ASSERT_NE(nullptr, device);
EXPECT_EQ(Case::Display::DISPLAY_ID::get(), device->getId());
EXPECT_EQ(static_cast<bool>(Case::Display::VIRTUAL), device->isVirtual());
EXPECT_EQ(static_cast<bool>(Case::Display::SECURE), device->isSecure());
@@ -282,8 +284,8 @@
device->receivesInput());
if constexpr (Case::Display::CONNECTION_TYPE::value) {
- EXPECT_NE(nullptr, device->getActiveMode());
- EXPECT_EQ(Case::Display::HWC_ACTIVE_CONFIG_ID, device->getActiveMode()->getHwcId());
+ ftl::FakeGuard guard(kMainThreadContext);
+ EXPECT_EQ(Case::Display::HWC_ACTIVE_CONFIG_ID, device->getActiveMode().getHwcId());
}
}