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

Commit 4235ea0f authored by Leon Scroggins III's avatar Leon Scroggins III
Browse files

Loosen requirement that getVsyncSchedule returns an object 

In some cases, a display may have been removed in between referring to
a display and when we attempt to retrieve its VsyncSchedule. Remove the
assert that there must be an associated VsyncSchedule, replacing it with
handling that is specific to the call site.

Anywhere std::null_opt is passed to getVsyncSchedule (the default),
leave the call sites unchanged. This will pick the pacesetter, which
should have a VsyncSchedule every time the method is called. This should
always return a valid VsyncSchedule. Otherwise, the old code would
LOG_ALWAYS_FATAL, while the new code would have a null pointer
dereference.

LOG_ALWAYS_FATAL if there is no associated VsyncSchedule in the
following methods. They are all called on the main thread, when the
schedule should always be available when requested. This matches the
old behavior. Where these are not documented as main-thread only, add
this annotation, since that's what makes this safe to assume.
- enableHardwareVsync
- disableHardwareVsync
- setDisplayPowerMode
- addPresentFence

In addResyncSample, which is called from a binder thread, the display
may have been removed since the callback was initiated. No-op with a
warning in this case.

In getDisplayStats, which also may be called from a binder thread, fall
back to mActiveDisplayId, just in case it is called in between Scheduler
demoting and promoting the pacesetter display. Add a test for this
method.

In the following methods, log an error and early exit. This may be more
conservative than necessary.
- resyncToHardwareVsyncLocked
- setRenderRate

In setVsyncEnabled, fail to setPendingHardwareVsyncState silently,
which matches the behavior for the call to setHWCVsyncEnabled.

Bug: 266094575
Bug: 277623302
Bug: 267636813
Bug: 278806588
Bug: 275691508
Bug: 275691150
Bug: 277364366
Test: builds, boots
Test: SurfaceFlingerGetDisplayStatsTest
Change-Id: I3541909448745b04252e88f299a4283f37e755e8
parent 8bf29931

services/surfaceflinger/Scheduler/Scheduler.cpp

+22 −5
Original line number Diff line number Diff line
@@ -406,11 +406,13 @@ void Scheduler::setVsyncConfig(const VsyncConfig& config, Period vsyncPeriod) { 


void Scheduler::enableHardwareVsync(PhysicalDisplayId id) {

    auto schedule = getVsyncSchedule(id);

    LOG_ALWAYS_FATAL_IF(!schedule);

    schedule->enableHardwareVsync(mSchedulerCallback);

}



void Scheduler::disableHardwareVsync(PhysicalDisplayId id, bool disallow) {

    auto schedule = getVsyncSchedule(id);

    LOG_ALWAYS_FATAL_IF(!schedule);

    schedule->disableHardwareVsync(mSchedulerCallback, disallow);

}
@@ -427,7 +429,10 @@ void Scheduler::resyncAllToHardwareVsync(bool allowToEnable) { 
void Scheduler::resyncToHardwareVsyncLocked(PhysicalDisplayId id, bool allowToEnable,

                                            std::optional<Fps> refreshRate) {

    const auto displayOpt = mDisplays.get(id);

    LOG_ALWAYS_FATAL_IF(!displayOpt);

    if (!displayOpt) {

        ALOGW("%s: Invalid display %s!", __func__, to_string(id).c_str());

        return;

    }

    const Display& display = *displayOpt;



    if (display.schedulePtr->isHardwareVsyncAllowed(allowToEnable)) {
@@ -446,7 +451,10 @@ void Scheduler::setRenderRate(PhysicalDisplayId id, Fps renderFrameRate) { 
    ftl::FakeGuard guard(kMainThreadContext);



    const auto displayOpt = mDisplays.get(id);

    LOG_ALWAYS_FATAL_IF(!displayOpt);

    if (!displayOpt) {

        ALOGW("%s: Invalid display %s!", __func__, to_string(id).c_str());

        return;

    }

    const Display& display = *displayOpt;

    const auto mode = display.selectorPtr->getActiveMode();
@@ -478,12 +486,18 @@ bool Scheduler::addResyncSample(PhysicalDisplayId id, nsecs_t timestamp, 
    const auto hwcVsyncPeriod = ftl::Optional(hwcVsyncPeriodIn).transform([](nsecs_t nanos) {

        return Period::fromNs(nanos);

    });

    return getVsyncSchedule(id)->addResyncSample(mSchedulerCallback, TimePoint::fromNs(timestamp),

    auto schedule = getVsyncSchedule(id);

    if (!schedule) {

        ALOGW("%s: Invalid display %s!", __func__, to_string(id).c_str());

        return false;

    }

    return schedule->addResyncSample(mSchedulerCallback, TimePoint::fromNs(timestamp),

                                     hwcVsyncPeriod);

}



void Scheduler::addPresentFence(PhysicalDisplayId id, std::shared_ptr<FenceTime> fence) {

    auto schedule = getVsyncSchedule(id);

    LOG_ALWAYS_FATAL_IF(!schedule);

    const bool needMoreSignals = schedule->getController().addPresentFence(std::move(fence));

    if (needMoreSignals) {

        schedule->enableHardwareVsync(mSchedulerCallback);
@@ -553,6 +567,7 @@ void Scheduler::setDisplayPowerMode(PhysicalDisplayId id, hal::PowerMode powerMo 
    {

        std::scoped_lock lock(mDisplayLock);

        auto vsyncSchedule = getVsyncScheduleLocked(id);

        LOG_ALWAYS_FATAL_IF(!vsyncSchedule);

        vsyncSchedule->getController().setDisplayPowerMode(powerMode);

    }

    if (!isPacesetter) return;
@@ -582,7 +597,9 @@ auto Scheduler::getVsyncScheduleLocked(std::optional<PhysicalDisplayId> idOpt) c 
    }



    const auto displayOpt = mDisplays.get(*idOpt);

    LOG_ALWAYS_FATAL_IF(!displayOpt);

    if (!displayOpt) {

        return nullptr;

    }

    return displayOpt->get().schedulePtr;

}

services/surfaceflinger/Scheduler/Scheduler.h

+4 −3
Original line number Diff line number Diff line
@@ -196,8 +196,8 @@ public: 
    // Sets the render rate for the scheduler to run at.

    void setRenderRate(PhysicalDisplayId, Fps);



    void enableHardwareVsync(PhysicalDisplayId);

    void disableHardwareVsync(PhysicalDisplayId, bool disallow);

    void enableHardwareVsync(PhysicalDisplayId) REQUIRES(kMainThreadContext);

    void disableHardwareVsync(PhysicalDisplayId, bool disallow) REQUIRES(kMainThreadContext);



    // Resyncs the scheduler to hardware vsync.

    // If allowToEnable is true, then hardware vsync will be turned on.
@@ -219,7 +219,8 @@ public: 
    // otherwise.

    bool addResyncSample(PhysicalDisplayId, nsecs_t timestamp,

                         std::optional<nsecs_t> hwcVsyncPeriod);

    void addPresentFence(PhysicalDisplayId, std::shared_ptr<FenceTime>) EXCLUDES(mDisplayLock);

    void addPresentFence(PhysicalDisplayId, std::shared_ptr<FenceTime>) EXCLUDES(mDisplayLock)

            REQUIRES(kMainThreadContext);



    // Layers are registered on creation, and unregistered when the weak reference expires.

    void registerLayer(Layer*);

services/surfaceflinger/SurfaceFlinger.cpp

+19 −8
Original line number Diff line number Diff line
@@ -1146,17 +1146,26 @@ status_t SurfaceFlinger::getDisplayStats(const sp<IBinder>& displayToken, 
    std::optional<PhysicalDisplayId> displayIdOpt;

    {

        Mutex::Autolock lock(mStateLock);

        if (displayToken) {

            displayIdOpt = getPhysicalDisplayIdLocked(displayToken);

            if (!displayIdOpt) {

                ALOGW("%s: Invalid physical display token %p", __func__, displayToken.get());

                return NAME_NOT_FOUND;

            }



        } else {

            // TODO (b/277364366): Clients should be updated to pass in the display they

    // want, rather than us picking an arbitrary one (the pacesetter, in this

            // want, rather than us picking an arbitrary one (the active display, in this

            // case).

    if (displayToken && !displayIdOpt) {

        ALOGE("%s: Invalid physical display token %p", __func__, displayToken.get());

        return NAME_NOT_FOUND;

            displayIdOpt = mActiveDisplayId;

        }

    }



    const auto schedule = mScheduler->getVsyncSchedule(displayIdOpt);

    if (!schedule) {

        ALOGE("%s: Missing VSYNC schedule for display %s!", __func__,

              to_string(*displayIdOpt).c_str());

        return NAME_NOT_FOUND;

    }

    outStats->vsyncTime = schedule->vsyncDeadlineAfter(TimePoint::now()).ns();

    outStats->vsyncPeriod = schedule->period().ns();

    return NO_ERROR;
@@ -2136,7 +2145,9 @@ void SurfaceFlinger::setVsyncEnabled(PhysicalDisplayId id, bool enabled) { 
    static_cast<void>(mScheduler->schedule([=]() FTL_FAKE_GUARD(mStateLock) {

        {

            ftl::FakeGuard guard(kMainThreadContext);

            mScheduler->getVsyncSchedule(id)->setPendingHardwareVsyncState(enabled);

            if (auto schedule = mScheduler->getVsyncSchedule(id)) {

                schedule->setPendingHardwareVsyncState(enabled);

            }

        }



        ATRACE_FORMAT("%s (%d) for %" PRIu64 " (main thread)", whence, enabled, id.value);

services/surfaceflinger/tests/unittests/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -104,6 +104,7 @@ cc_test { 
        "SurfaceFlinger_DisplayTransactionCommitTest.cpp",

        "SurfaceFlinger_ExcludeDolbyVisionTest.cpp",

        "SurfaceFlinger_GetDisplayNativePrimariesTest.cpp",

        "SurfaceFlinger_GetDisplayStatsTest.cpp",

        "SurfaceFlinger_HdrOutputControlTest.cpp",

        "SurfaceFlinger_HotplugTest.cpp",

        "SurfaceFlinger_InitializeDisplaysTest.cpp",

services/surfaceflinger/tests/unittests/SurfaceFlinger_GetDisplayStatsTest.cpp

0 → 100644
+116 −0
Original line number Diff line number Diff line 
/*

 * Copyright 2023 The Android Open Source Project

 *

 * Licensed under the Apache License, Version 2.0 (the "License");

 * you may not use this file except in compliance with the License.

 * You may obtain a copy of the License at

 *

 *      http://www.apache.org/licenses/LICENSE-2.0

 *

 * Unless required by applicable law or agreed to in writing, software

 * distributed under the License is distributed on an "AS IS" BASIS,

 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

 * See the License for the specific language governing permissions and

 * limitations under the License.

 */



#undef LOG_TAG

#define LOG_TAG "SurfaceFlingerGetDisplayStatsTest"



#include <compositionengine/Display.h>

#include <compositionengine/mock/DisplaySurface.h>

#include <gmock/gmock.h>

#include <gtest/gtest.h>

#include <renderengine/mock/RenderEngine.h>

#include <ui/DisplayStatInfo.h>

#include "TestableSurfaceFlinger.h"

#include "mock/DisplayHardware/MockComposer.h"

#include "mock/DisplayHardware/MockPowerAdvisor.h"

#include "mock/MockTimeStats.h"

#include "mock/system/window/MockNativeWindow.h"



using namespace android;

using namespace testing;



namespace android {

namespace {

using FakeHwcDisplayInjector = TestableSurfaceFlinger::FakeHwcDisplayInjector;

using FakeDisplayDeviceInjector = TestableSurfaceFlinger::FakeDisplayDeviceInjector;



constexpr hal::HWDisplayId HWC_DISPLAY = FakeHwcDisplayInjector::DEFAULT_HWC_DISPLAY_ID;

constexpr PhysicalDisplayId DEFAULT_DISPLAY_ID = PhysicalDisplayId::fromPort(42u);

constexpr int DEFAULT_DISPLAY_WIDTH = 1920;

constexpr int DEFAULT_DISPLAY_HEIGHT = 1024;



class SurfaceFlingerGetDisplayStatsTest : public Test {

public:

    void SetUp() override;



protected:

    TestableSurfaceFlinger mFlinger;

    renderengine::mock::RenderEngine* mRenderEngine = new renderengine::mock::RenderEngine();

    sp<DisplayDevice> mDisplay;

    sp<compositionengine::mock::DisplaySurface> mDisplaySurface =

            sp<compositionengine::mock::DisplaySurface>::make();

    sp<mock::NativeWindow> mNativeWindow = sp<mock::NativeWindow>::make();

    mock::TimeStats* mTimeStats = new mock::TimeStats();

    Hwc2::mock::PowerAdvisor* mPowerAdvisor = nullptr;

    Hwc2::mock::Composer* mComposer = nullptr;

};



void SurfaceFlingerGetDisplayStatsTest::SetUp() {

    mFlinger.setupMockScheduler({.displayId = DEFAULT_DISPLAY_ID});

    mComposer = new Hwc2::mock::Composer();

    mPowerAdvisor = new Hwc2::mock::PowerAdvisor();

    mFlinger.setupRenderEngine(std::unique_ptr<renderengine::RenderEngine>(mRenderEngine));

    mFlinger.setupTimeStats(std::shared_ptr<TimeStats>(mTimeStats));

    mFlinger.setupComposer(std::unique_ptr<Hwc2::Composer>(mComposer));

    mFlinger.setupPowerAdvisor(std::unique_ptr<Hwc2::PowerAdvisor>(mPowerAdvisor));

    static constexpr bool kIsPrimary = true;

    FakeHwcDisplayInjector(DEFAULT_DISPLAY_ID, hal::DisplayType::PHYSICAL, kIsPrimary)

            .setPowerMode(hal::PowerMode::ON)

            .inject(&mFlinger, mComposer);

    auto compostionEngineDisplayArgs =

            compositionengine::DisplayCreationArgsBuilder()

                    .setId(DEFAULT_DISPLAY_ID)

                    .setPixels({DEFAULT_DISPLAY_WIDTH, DEFAULT_DISPLAY_HEIGHT})

                    .setPowerAdvisor(mPowerAdvisor)

                    .setName("injected display")

                    .build();

    auto compositionDisplay =

            compositionengine::impl::createDisplay(mFlinger.getCompositionEngine(),

                                                   std::move(compostionEngineDisplayArgs));

    mDisplay =

            FakeDisplayDeviceInjector(mFlinger, compositionDisplay,

                                      ui::DisplayConnectionType::Internal, HWC_DISPLAY, kIsPrimary)

                    .setDisplaySurface(mDisplaySurface)

                    .setNativeWindow(mNativeWindow)

                    .setPowerMode(hal::PowerMode::ON)

                    .setRefreshRateSelector(mFlinger.scheduler()->refreshRateSelector())

                    .skipRegisterDisplay()

                    .inject();

}



// TODO (b/277364366): Clients should be updated to pass in the display they want.

TEST_F(SurfaceFlingerGetDisplayStatsTest, nullptrSucceeds) {

    DisplayStatInfo info;

    status_t status = mFlinger.getDisplayStats(nullptr, &info);

    EXPECT_EQ(status, NO_ERROR);

}



TEST_F(SurfaceFlingerGetDisplayStatsTest, explicitToken) {

    DisplayStatInfo info;

    status_t status = mFlinger.getDisplayStats(mDisplay->getDisplayToken().promote(), &info);

    EXPECT_EQ(status, NO_ERROR);

}



TEST_F(SurfaceFlingerGetDisplayStatsTest, invalidToken) {

    const String8 displayName("fakeDisplay");

    sp<IBinder> displayToken = mFlinger.createDisplay(displayName, false);

    DisplayStatInfo info;

    status_t status = mFlinger.getDisplayStats(displayToken, &info);

    EXPECT_EQ(status, NAME_NOT_FOUND);

}



} // namespace

} // namespace android