From b5a094b997f4db0b2f21ea6b2be6fa8bc6ba2f57 Mon Sep 17 00:00:00 2001 From: Dominik Laskowski Date: Thu, 27 Oct 2022 12:00:12 -0400 Subject: 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 --- services/surfaceflinger/Scheduler/Scheduler.cpp | 23 ++++----- services/surfaceflinger/Scheduler/Scheduler.h | 16 +++--- services/surfaceflinger/SurfaceFlinger.cpp | 14 ++--- .../tests/unittests/SchedulerTest.cpp | 59 +++++++--------------- .../tests/unittests/TestableScheduler.h | 13 ++--- .../tests/unittests/TestableSurfaceFlinger.h | 3 +- 6 files changed, 52 insertions(+), 76 deletions(-) diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index 499cee68f1..0e1b775a8c 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -94,7 +94,7 @@ void Scheduler::startTimers() { } } -void Scheduler::setRefreshRateSelector(std::shared_ptr 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 @@ void Scheduler::setRefreshRateSelector(std::shared_ptr sele mRefreshRateSelector->startIdleTimer(); } -void Scheduler::registerDisplay(sp 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 @@ void Scheduler::unregisterDisplay(PhysicalDisplayId displayId) { mLeaderDisplayId.reset(); } - mDisplays.erase(displayId); + mRefreshRateSelectors.erase(displayId); } void Scheduler::run() { @@ -711,10 +710,9 @@ auto Scheduler::chooseDisplayModes() const -> DisplayModeChoiceMap { 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 @@ auto Scheduler::chooseDisplayModes() const -> DisplayModeChoiceMap { // 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 @@ auto Scheduler::chooseDisplayModes() const -> DisplayModeChoiceMap { 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 39c41b99da..901cf74558 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 @@ public: virtual ~Scheduler(); void startTimers(); - void setRefreshRateSelector(std::shared_ptr) - EXCLUDES(mRefreshRateSelectorLock); - void registerDisplay(sp); + using RefreshRateSelectorPtr = std::shared_ptr; + void setRefreshRateSelector(RefreshRateSelectorPtr) EXCLUDES(mRefreshRateSelectorLock); + + void registerDisplay(PhysicalDisplayId, RefreshRateSelectorPtr); void unregisterDisplay(PhysicalDisplayId); void run(); @@ -299,8 +299,7 @@ private: EXCLUDES(mRefreshRateSelectorLock); android::impl::EventThread::GetVsyncPeriodFunction makeGetVsyncPeriodFunction() const; - std::shared_ptr holdRefreshRateSelector() const - EXCLUDES(mRefreshRateSelectorLock) { + RefreshRateSelectorPtr holdRefreshRateSelector() const EXCLUDES(mRefreshRateSelectorLock) { std::scoped_lock lock(mRefreshRateSelectorLock); return mRefreshRateSelector; } @@ -336,7 +335,7 @@ private: mutable std::mutex mPolicyLock; - display::PhysicalDisplayMap> mDisplays; + display::PhysicalDisplayMap mRefreshRateSelectors; std::optional mLeaderDisplayId; struct Policy { @@ -359,8 +358,9 @@ private: std::optional cachedModeChangedParams; } mPolicy GUARDED_BY(mPolicyLock); + // TODO(b/255635821): Remove this by instead looking up the `mLeaderDisplayId` selector. mutable std::mutex mRefreshRateSelectorLock; - std::shared_ptr mRefreshRateSelector GUARDED_BY(mRefreshRateSelectorLock); + RefreshRateSelectorPtr mRefreshRateSelector GUARDED_BY(mRefreshRateSelectorLock); std::mutex mVsyncTimelineLock; std::optional mLastVsyncPeriodChangeTimeline diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 89d905a4ad..aa930bcff3 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -2937,11 +2937,15 @@ void SurfaceFlinger::processDisplayAdded(const wp& displayToken, 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 @@ void SurfaceFlinger::processDisplayChanged(const wp& displayToken, display->disconnect(); if (display->isVirtual()) { releaseVirtualDisplay(display->getVirtualId()); - } else { - mScheduler->unregisterDisplay(display->getPhysicalId()); } } @@ -3409,8 +3411,8 @@ void SurfaceFlinger::initScheduler(const sp& display) { } 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 066083fffa..ea4666ed4b 100644 --- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp +++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp @@ -20,7 +20,6 @@ #include -#include "FakeDisplayInjector.h" #include "Scheduler/EventThread.h" #include "Scheduler/RefreshRateSelector.h" #include "TestableScheduler.h" @@ -41,7 +40,6 @@ namespace { using MockEventThread = android::mock::EventThread; using MockLayer = android::mock::MockLayer; -using FakeDisplayDeviceInjector = TestableSurfaceFlinger::FakeDisplayDeviceInjector; class SchedulerTest : public testing::Test { protected: @@ -90,10 +88,6 @@ protected: sp mEventThreadConnection; TestableSurfaceFlinger mFlinger; - Hwc2::mock::PowerAdvisor mPowerAdvisor; - sp mNativeWindow = sp::make(); - - FakeDisplayInjector mFakeDisplayInjector{mFlinger, mPowerAdvisor, mNativeWindow}; }; SchedulerTest::SchedulerTest() { @@ -240,14 +234,11 @@ MATCHER(Is120Hz, "") { } TEST_F(SchedulerTest, chooseRefreshRateForContentSelectsMaxRefreshRate) { - const auto display = mFakeDisplayInjector.injectInternalDisplay( - [&](FakeDisplayDeviceInjector& injector) { - injector.setDisplayModes(kDisplay1Modes, kDisplay1Mode60->getId()); - }, - {.displayId = kDisplayId1}); + const auto selectorPtr = + std::make_shared(kDisplay1Modes, kDisplay1Mode60->getId()); - mScheduler->registerDisplay(display); - mScheduler->setRefreshRateSelector(display->holdRefreshRateSelector()); + mScheduler->registerDisplay(kDisplayId1, selectorPtr); + mScheduler->setRefreshRateSelector(selectorPtr); const sp layer = sp::make(mFlinger.flinger()); EXPECT_CALL(*layer, isVisible()).WillOnce(Return(true)); @@ -269,13 +260,9 @@ TEST_F(SchedulerTest, chooseRefreshRateForContentSelectsMaxRefreshRate) { } 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(kDisplay1Modes, + kDisplay1Mode60->getId())); std::vector layers = std::vector({{.weight = 1.f}, {.weight = 1.f}}); @@ -314,23 +301,16 @@ TEST_F(SchedulerTest, chooseDisplayModesSingleDisplay) { 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(kDisplay1Modes, + kDisplay1Mode60->getId())); + mScheduler->registerDisplay(kDisplayId2, + std::make_shared(kDisplay2Modes, + kDisplay2Mode60->getId())); using DisplayModeChoice = TestableScheduler::DisplayModeChoice; TestableScheduler::DisplayModeChoiceMap expectedChoices; @@ -380,13 +360,10 @@ TEST_F(SchedulerTest, chooseDisplayModesMultipleDisplays) { } { // 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(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 2814d38b47..95c99150b3 100644 --- a/services/surfaceflinger/tests/unittests/TestableScheduler.h +++ b/services/surfaceflinger/tests/unittests/TestableScheduler.h @@ -32,15 +32,13 @@ namespace android::scheduler { class TestableScheduler : public Scheduler, private ICompositor { public: - TestableScheduler(std::shared_ptr selectorPtr, - ISchedulerCallback& callback) + TestableScheduler(RefreshRateSelectorPtr selectorPtr, ISchedulerCallback& callback) : TestableScheduler(std::make_unique(), std::make_unique(), std::move(selectorPtr), callback) {} TestableScheduler(std::unique_ptr controller, - std::unique_ptr tracker, - std::shared_ptr selectorPtr, + std::unique_ptr tracker, RefreshRateSelectorPtr selectorPtr, ISchedulerCallback& callback) : Scheduler(*this, callback, Feature::kContentDetection) { mVsyncSchedule.emplace(VsyncSchedule(std::move(tracker), nullptr, std::move(controller))); @@ -68,16 +66,15 @@ public: 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 ff79ce099e..35c037c051 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -842,7 +842,8 @@ public: sp display = sp::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; -- cgit v1.2.3-59-g8ed1b