SF: Avoid registering DisplayDevice with Scheduler
The Scheduler should only care about the RefreshRateSelector part, and
should not needlessly extend the compositionengine::Display's lifetime
until the DisplayDevice is unregistered.
Make Scheduler::registerDisplay infallible, such that SurfaceFlinger::
processDisplayChanged does not need to unregister before registering.
Bug: 241285191
Test: libsurfaceflinger_unittest
Change-Id: I12b3855167e98f48ae368d39264edcb456efb293
diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp
index 499cee6..0e1b775 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.cpp
+++ b/services/surfaceflinger/Scheduler/Scheduler.cpp
@@ -94,7 +94,7 @@
}
}
-void Scheduler::setRefreshRateSelector(std::shared_ptr<RefreshRateSelector> selectorPtr) {
+void Scheduler::setRefreshRateSelector(RefreshRateSelectorPtr selectorPtr) {
// The current RefreshRateSelector instance may outlive this call, so unbind its idle timer.
{
// mRefreshRateSelectorLock is not locked here to avoid the deadlock
@@ -126,13 +126,12 @@
mRefreshRateSelector->startIdleTimer();
}
-void Scheduler::registerDisplay(sp<const DisplayDevice> display) {
- if (display->isPrimary()) {
- mLeaderDisplayId = display->getPhysicalId();
+void Scheduler::registerDisplay(PhysicalDisplayId displayId, RefreshRateSelectorPtr selectorPtr) {
+ if (!mLeaderDisplayId) {
+ mLeaderDisplayId = displayId;
}
- const bool ok = mDisplays.try_emplace(display->getPhysicalId(), std::move(display)).second;
- ALOGE_IF(!ok, "%s: Duplicate display", __func__);
+ mRefreshRateSelectors.emplace_or_replace(displayId, std::move(selectorPtr));
}
void Scheduler::unregisterDisplay(PhysicalDisplayId displayId) {
@@ -140,7 +139,7 @@
mLeaderDisplayId.reset();
}
- mDisplays.erase(displayId);
+ mRefreshRateSelectors.erase(displayId);
}
void Scheduler::run() {
@@ -711,10 +710,9 @@
const auto globalSignals = makeGlobalSignals();
- for (const auto& [id, display] : mDisplays) {
+ for (const auto& [id, selectorPtr] : mRefreshRateSelectors) {
auto rankedRefreshRates =
- display->holdRefreshRateSelector()
- ->getRankedRefreshRates(mPolicy.contentRequirements, globalSignals);
+ selectorPtr->getRankedRefreshRates(mPolicy.contentRequirements, globalSignals);
for (const auto& [modePtr, score] : rankedRefreshRates.ranking) {
const auto [it, inserted] = refreshRateTallies.try_emplace(modePtr->getFps(), score);
@@ -733,7 +731,7 @@
// Find the first refresh rate common to all displays.
while (maxScoreIt != refreshRateTallies.cend() &&
- maxScoreIt->second.displayCount != mDisplays.size()) {
+ maxScoreIt->second.displayCount != mRefreshRateSelectors.size()) {
++maxScoreIt;
}
@@ -742,7 +740,8 @@
for (auto it = maxScoreIt + 1; it != refreshRateTallies.cend(); ++it) {
const auto [fps, tally] = *it;
- if (tally.displayCount == mDisplays.size() && tally.score > maxScoreIt->second.score) {
+ if (tally.displayCount == mRefreshRateSelectors.size() &&
+ tally.score > maxScoreIt->second.score) {
maxScoreIt = it;
}
}
diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h
index 39c41b9..901cf74 100644
--- a/services/surfaceflinger/Scheduler/Scheduler.h
+++ b/services/surfaceflinger/Scheduler/Scheduler.h
@@ -39,7 +39,6 @@
#include "Display/DisplayMap.h"
#include "Display/DisplayModeRequest.h"
-#include "DisplayDevice.h"
#include "EventThread.h"
#include "FrameRateOverrideMappings.h"
#include "LayerHistory.h"
@@ -107,10 +106,11 @@
virtual ~Scheduler();
void startTimers();
- void setRefreshRateSelector(std::shared_ptr<RefreshRateSelector>)
- EXCLUDES(mRefreshRateSelectorLock);
- void registerDisplay(sp<const DisplayDevice>);
+ using RefreshRateSelectorPtr = std::shared_ptr<RefreshRateSelector>;
+ void setRefreshRateSelector(RefreshRateSelectorPtr) EXCLUDES(mRefreshRateSelectorLock);
+
+ void registerDisplay(PhysicalDisplayId, RefreshRateSelectorPtr);
void unregisterDisplay(PhysicalDisplayId);
void run();
@@ -299,8 +299,7 @@
EXCLUDES(mRefreshRateSelectorLock);
android::impl::EventThread::GetVsyncPeriodFunction makeGetVsyncPeriodFunction() const;
- std::shared_ptr<RefreshRateSelector> holdRefreshRateSelector() const
- EXCLUDES(mRefreshRateSelectorLock) {
+ RefreshRateSelectorPtr holdRefreshRateSelector() const EXCLUDES(mRefreshRateSelectorLock) {
std::scoped_lock lock(mRefreshRateSelectorLock);
return mRefreshRateSelector;
}
@@ -336,7 +335,7 @@
mutable std::mutex mPolicyLock;
- display::PhysicalDisplayMap<PhysicalDisplayId, sp<const DisplayDevice>> mDisplays;
+ display::PhysicalDisplayMap<PhysicalDisplayId, RefreshRateSelectorPtr> mRefreshRateSelectors;
std::optional<PhysicalDisplayId> mLeaderDisplayId;
struct Policy {
@@ -359,8 +358,9 @@
std::optional<ModeChangedParams> cachedModeChangedParams;
} mPolicy GUARDED_BY(mPolicyLock);
+ // TODO(b/255635821): Remove this by instead looking up the `mLeaderDisplayId` selector.
mutable std::mutex mRefreshRateSelectorLock;
- std::shared_ptr<RefreshRateSelector> mRefreshRateSelector GUARDED_BY(mRefreshRateSelectorLock);
+ RefreshRateSelectorPtr mRefreshRateSelector GUARDED_BY(mRefreshRateSelectorLock);
std::mutex mVsyncTimelineLock;
std::optional<hal::VsyncPeriodChangeTimeline> mLastVsyncPeriodChangeTimeline
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 89d905a..aa930bc 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2937,11 +2937,15 @@
displaySurface, producer);
if (mScheduler && !display->isVirtual()) {
+ auto selectorPtr = display->holdRefreshRateSelector();
+
// Display modes are reloaded on hotplug reconnect.
if (display->isPrimary()) {
- mScheduler->setRefreshRateSelector(display->holdRefreshRateSelector());
+ mScheduler->setRefreshRateSelector(selectorPtr);
}
- mScheduler->registerDisplay(display);
+
+ const auto displayId = display->getPhysicalId();
+ mScheduler->registerDisplay(displayId, std::move(selectorPtr));
dispatchDisplayHotplugEvent(display->getPhysicalId(), true);
}
@@ -2994,8 +2998,6 @@
display->disconnect();
if (display->isVirtual()) {
releaseVirtualDisplay(display->getVirtualId());
- } else {
- mScheduler->unregisterDisplay(display->getPhysicalId());
}
}
@@ -3409,8 +3411,8 @@
}
mScheduler->createVsyncSchedule(features);
- mScheduler->setRefreshRateSelector(std::move(selectorPtr));
- mScheduler->registerDisplay(display);
+ mScheduler->setRefreshRateSelector(selectorPtr);
+ mScheduler->registerDisplay(display->getPhysicalId(), std::move(selectorPtr));
}
setVsyncEnabled(false);
mScheduler->startTimers();
diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
index 066083f..ea4666e 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -20,7 +20,6 @@
#include <mutex>
-#include "FakeDisplayInjector.h"
#include "Scheduler/EventThread.h"
#include "Scheduler/RefreshRateSelector.h"
#include "TestableScheduler.h"
@@ -41,7 +40,6 @@
using MockEventThread = android::mock::EventThread;
using MockLayer = android::mock::MockLayer;
-using FakeDisplayDeviceInjector = TestableSurfaceFlinger::FakeDisplayDeviceInjector;
class SchedulerTest : public testing::Test {
protected:
@@ -90,10 +88,6 @@
sp<MockEventThreadConnection> mEventThreadConnection;
TestableSurfaceFlinger mFlinger;
- Hwc2::mock::PowerAdvisor mPowerAdvisor;
- sp<android::mock::NativeWindow> mNativeWindow = sp<android::mock::NativeWindow>::make();
-
- FakeDisplayInjector mFakeDisplayInjector{mFlinger, mPowerAdvisor, mNativeWindow};
};
SchedulerTest::SchedulerTest() {
@@ -240,14 +234,11 @@
}
TEST_F(SchedulerTest, chooseRefreshRateForContentSelectsMaxRefreshRate) {
- const auto display = mFakeDisplayInjector.injectInternalDisplay(
- [&](FakeDisplayDeviceInjector& injector) {
- injector.setDisplayModes(kDisplay1Modes, kDisplay1Mode60->getId());
- },
- {.displayId = kDisplayId1});
+ const auto selectorPtr =
+ std::make_shared<RefreshRateSelector>(kDisplay1Modes, kDisplay1Mode60->getId());
- mScheduler->registerDisplay(display);
- mScheduler->setRefreshRateSelector(display->holdRefreshRateSelector());
+ mScheduler->registerDisplay(kDisplayId1, selectorPtr);
+ mScheduler->setRefreshRateSelector(selectorPtr);
const sp<MockLayer> layer = sp<MockLayer>::make(mFlinger.flinger());
EXPECT_CALL(*layer, isVisible()).WillOnce(Return(true));
@@ -269,13 +260,9 @@
}
TEST_F(SchedulerTest, chooseDisplayModesSingleDisplay) {
- const auto display = mFakeDisplayInjector.injectInternalDisplay(
- [&](FakeDisplayDeviceInjector& injector) {
- injector.setDisplayModes(kDisplay1Modes, kDisplay1Mode60->getId());
- },
- {.displayId = kDisplayId1});
-
- mScheduler->registerDisplay(display);
+ mScheduler->registerDisplay(kDisplayId1,
+ std::make_shared<RefreshRateSelector>(kDisplay1Modes,
+ kDisplay1Mode60->getId()));
std::vector<RefreshRateSelector::LayerRequirement> layers =
std::vector<RefreshRateSelector::LayerRequirement>({{.weight = 1.f}, {.weight = 1.f}});
@@ -314,23 +301,16 @@
EXPECT_EQ(choice->get(), DisplayModeChoice(kDisplay1Mode120, globalSignals));
mScheduler->unregisterDisplay(kDisplayId1);
- EXPECT_TRUE(mScheduler->mutableDisplays().empty());
+ EXPECT_FALSE(mScheduler->hasRefreshRateSelectors());
}
TEST_F(SchedulerTest, chooseDisplayModesMultipleDisplays) {
- const auto display1 = mFakeDisplayInjector.injectInternalDisplay(
- [&](FakeDisplayDeviceInjector& injector) {
- injector.setDisplayModes(kDisplay1Modes, kDisplay1Mode60->getId());
- },
- {.displayId = kDisplayId1, .hwcDisplayId = 42, .isPrimary = true});
- const auto display2 = mFakeDisplayInjector.injectInternalDisplay(
- [&](FakeDisplayDeviceInjector& injector) {
- injector.setDisplayModes(kDisplay2Modes, kDisplay2Mode60->getId());
- },
- {.displayId = kDisplayId2, .hwcDisplayId = 41, .isPrimary = false});
-
- mScheduler->registerDisplay(display1);
- mScheduler->registerDisplay(display2);
+ mScheduler->registerDisplay(kDisplayId1,
+ std::make_shared<RefreshRateSelector>(kDisplay1Modes,
+ kDisplay1Mode60->getId()));
+ mScheduler->registerDisplay(kDisplayId2,
+ std::make_shared<RefreshRateSelector>(kDisplay2Modes,
+ kDisplay2Mode60->getId()));
using DisplayModeChoice = TestableScheduler::DisplayModeChoice;
TestableScheduler::DisplayModeChoiceMap expectedChoices;
@@ -380,13 +360,10 @@
}
{
// This display does not support 120 Hz, so we should choose 60 Hz despite the touch signal.
- const auto display3 = mFakeDisplayInjector.injectInternalDisplay(
- [&](FakeDisplayDeviceInjector& injector) {
- injector.setDisplayModes(kDisplay3Modes, kDisplay3Mode60->getId());
- },
- {.displayId = kDisplayId3, .hwcDisplayId = 40, .isPrimary = false});
-
- mScheduler->registerDisplay(display3);
+ mScheduler
+ ->registerDisplay(kDisplayId3,
+ std::make_shared<RefreshRateSelector>(kDisplay3Modes,
+ kDisplay3Mode60->getId()));
const GlobalSignals globalSignals = {.touch = true};
mScheduler->replaceTouchTimer(10);
diff --git a/services/surfaceflinger/tests/unittests/TestableScheduler.h b/services/surfaceflinger/tests/unittests/TestableScheduler.h
index 2814d38..95c9915 100644
--- a/services/surfaceflinger/tests/unittests/TestableScheduler.h
+++ b/services/surfaceflinger/tests/unittests/TestableScheduler.h
@@ -32,15 +32,13 @@
class TestableScheduler : public Scheduler, private ICompositor {
public:
- TestableScheduler(std::shared_ptr<RefreshRateSelector> selectorPtr,
- ISchedulerCallback& callback)
+ TestableScheduler(RefreshRateSelectorPtr selectorPtr, ISchedulerCallback& callback)
: TestableScheduler(std::make_unique<mock::VsyncController>(),
std::make_unique<mock::VSyncTracker>(), std::move(selectorPtr),
callback) {}
TestableScheduler(std::unique_ptr<VsyncController> controller,
- std::unique_ptr<VSyncTracker> tracker,
- std::shared_ptr<RefreshRateSelector> selectorPtr,
+ std::unique_ptr<VSyncTracker> tracker, RefreshRateSelectorPtr selectorPtr,
ISchedulerCallback& callback)
: Scheduler(*this, callback, Feature::kContentDetection) {
mVsyncSchedule.emplace(VsyncSchedule(std::move(tracker), nullptr, std::move(controller)));
@@ -68,16 +66,15 @@
auto& mutablePrimaryHWVsyncEnabled() { return mPrimaryHWVsyncEnabled; }
auto& mutableHWVsyncAvailable() { return mHWVsyncAvailable; }
- auto& mutableLayerHistory() { return mLayerHistory; }
+ auto refreshRateSelector() { return holdRefreshRateSelector(); }
+ bool hasRefreshRateSelectors() const { return !mRefreshRateSelectors.empty(); }
- auto& mutableDisplays() { return mDisplays; }
+ auto& mutableLayerHistory() { return mLayerHistory; }
size_t layerHistorySize() NO_THREAD_SAFETY_ANALYSIS {
return mLayerHistory.mActiveLayerInfos.size() + mLayerHistory.mInactiveLayerInfos.size();
}
- auto refreshRateSelector() { return holdRefreshRateSelector(); }
-
size_t getNumActiveLayers() NO_THREAD_SAFETY_ANALYSIS {
return mLayerHistory.mActiveLayerInfos.size();
}
diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
index ff79ce0..35c037c 100644
--- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
+++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h
@@ -842,7 +842,8 @@
sp<DisplayDevice> display = sp<DisplayDevice>::make(mCreationArgs);
mFlinger.mutableDisplays().emplace_or_replace(mDisplayToken, display);
if (mFlinger.scheduler()) {
- mFlinger.scheduler()->registerDisplay(display);
+ mFlinger.scheduler()->registerDisplay(display->getPhysicalId(),
+ display->holdRefreshRateSelector());
}
DisplayDeviceState state;