Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b5a094b9 authored by Dominik Laskowski's avatar Dominik Laskowski
Browse files

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
parent 3681e5a1

services/surfaceflinger/Scheduler/Scheduler.cpp

+11 −12
Original line number Diff line number Diff line
@@ -94,7 +94,7 @@ void Scheduler::startTimers() { 
    }

}



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 @@ void Scheduler::setRefreshRateSelector(std::shared_ptr<RefreshRateSelector> sele 
    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 @@ 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;

            }

        }

services/surfaceflinger/Scheduler/Scheduler.h

+8 −8
Original line number Diff line number Diff line
@@ -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<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 @@ private: 
            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 @@ private: 


    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 @@ private: 
        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

services/surfaceflinger/SurfaceFlinger.cpp

+8 −6
Original line number Diff line number Diff line
@@ -2937,11 +2937,15 @@ void SurfaceFlinger::processDisplayAdded(const wp<IBinder>& 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<IBinder>& displayToken, 
            display->disconnect();

            if (display->isVirtual()) {

                releaseVirtualDisplay(display->getVirtualId());

            } else {

                mScheduler->unregisterDisplay(display->getPhysicalId());

            }

        }
@@ -3409,8 +3411,8 @@ void SurfaceFlinger::initScheduler(const sp<const DisplayDevice>& 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();

services/surfaceflinger/tests/unittests/SchedulerTest.cpp

+18 −41
Original line number Diff line number Diff line
@@ -20,7 +20,6 @@ 


#include <mutex>



#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<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 @@ 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<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, 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<RefreshRateSelector>(kDisplay1Modes,

                                                                      kDisplay1Mode60->getId()));



    std::vector<RefreshRateSelector::LayerRequirement> layers =

            std::vector<RefreshRateSelector::LayerRequirement>({{.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<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 @@ 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<RefreshRateSelector>(kDisplay3Modes,

                                                                        kDisplay3Mode60->getId()));



        const GlobalSignals globalSignals = {.touch = true};

        mScheduler->replaceTouchTimer(10);

services/surfaceflinger/tests/unittests/TestableScheduler.h

+5 −8
Original line number Diff line number Diff line
@@ -32,15 +32,13 @@ namespace android::scheduler { 


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 @@ 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();

    }