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

Commit b4ad3600 authored by Alec Mouri's avatar Alec Mouri
Browse files

Tweak wakeup time for OneShotTimer to reduce jank 

Power optimizations for OneShotTimer from change
I83f968042395857237875aab8dca0e6b90d392cb accidentally relaxed the
practical timing requirements for OneShotTimer by allowing for waiting
on a semaphore longer than intended - the optimiation amounted to
throttling the number of sem_post calls, and relying on the idle timeout
thread to check if the timer reset on wakeup before either re-entering
the reset loop or entering the idle loop. But that requires adjusting
the timeout for waiting on the semaphore by the idle timeout thread,
which did not happen. So it was possible to delay moving into an idle
state by up to double what a OneShotTimer was configured to do so. The net effect is that when a device wakes up from an idle state, it may delay learning the vsync cadence if the kernel dropped the refresh rate under the scenes, which causes noticeable jank over the first few frames.

This patch resolves this discepancy by shortening the interval that the
idle timeout thread waits on the semphore for, based on the last time
OneShotTimer::reset was called.

Bug: 233713246
Test: UIBench jank tests
Change-Id: I915822ab71ea3595a02ffbebe16e591a622fcd80
parent 5f0d32a6

services/surfaceflinger/Scheduler/OneShotTimer.cpp

+12 −11
Original line number Diff line number Diff line
@@ -118,17 +118,18 @@ void OneShotTimer::loop() { 
        auto triggerTime = mClock->now() + mInterval;

        state = TimerState::WAITING;

        while (true) {

            // Wait until triggerTime time to check if we need to reset or drop into the idle state.

            if (const auto triggerInterval = triggerTime - mClock->now(); triggerInterval > 0ns) {

                mWaiting = true;

            constexpr auto zero = std::chrono::steady_clock::duration::zero();

            // Wait for mInterval time to check if we need to reset or drop into the idle state.

                struct timespec ts;

            calculateTimeoutTime(std::chrono::nanoseconds(mInterval), &ts);

                calculateTimeoutTime(triggerInterval, &ts);

                int result = sem_clockwait(&mSemaphore, CLOCK_MONOTONIC, &ts);

                if (result && errno != ETIMEDOUT && errno != EINTR) {

                    std::stringstream ss;

                    ss << "sem_clockwait failed (" << errno << ")";

                    LOG_ALWAYS_FATAL("%s", ss.str().c_str());

                }

            }



            mWaiting = false;

            state = checkForResetAndStop(state);
@@ -136,7 +137,7 @@ void OneShotTimer::loop() { 
                break;

            }



            if (state == TimerState::WAITING && (triggerTime - mClock->now()) <= zero) {

            if (state == TimerState::WAITING && (triggerTime - mClock->now()) <= 0ns) {

                triggerTimeout = true;

                state = TimerState::IDLE;

                break;

services/surfaceflinger/tests/unittests/OneShotTimerTest.cpp

+34 −0
Original line number Diff line number Diff line
@@ -130,6 +130,40 @@ TEST_F(OneShotTimerTest, DISABLED_resetBackToBackTest) { 
    EXPECT_FALSE(mResetTimerCallback.waitForUnexpectedCall().has_value());

}



// TODO(b/186417847) This test is new and passes locally, but may be flaky

TEST_F(OneShotTimerTest, DISABLED_resetBackToBackSlowAdvanceTest) {

    fake::FakeClock* clock = new fake::FakeClock();

    mIdleTimer = std::make_unique<scheduler::OneShotTimer>("TestTimer", 1ms,

                                                           mResetTimerCallback.getInvocable(),

                                                           mExpiredTimerCallback.getInvocable(),

                                                           std::unique_ptr<fake::FakeClock>(clock));

    mIdleTimer->start();

    EXPECT_TRUE(mResetTimerCallback.waitForCall().has_value());



    mIdleTimer->reset();

    EXPECT_FALSE(mResetTimerCallback.waitForUnexpectedCall().has_value());

    EXPECT_FALSE(mExpiredTimerCallback.waitForUnexpectedCall().has_value());



    clock->advanceTime(200us);

    mIdleTimer->reset();



    // Normally we would check that the timer callbacks weren't invoked here

    // after resetting the timer, but we need to precisely control the timing of

    // this test, and checking that callbacks weren't invoked requires non-zero

    // time.



    clock->advanceTime(1500us);

    EXPECT_TRUE(mExpiredTimerCallback.waitForCall(1100us).has_value());

    mIdleTimer->reset();

    EXPECT_TRUE(mResetTimerCallback.waitForCall().has_value());



    mIdleTimer->stop();

    clock->advanceTime(2ms);

    // Final quick check that no more callback were observed.

    EXPECT_FALSE(mExpiredTimerCallback.waitForUnexpectedCall().has_value());

    EXPECT_FALSE(mResetTimerCallback.waitForUnexpectedCall().has_value());

}



TEST_F(OneShotTimerTest, startNotCalledTest) {

    fake::FakeClock* clock = new fake::FakeClock();

    mIdleTimer = std::make_unique<scheduler::OneShotTimer>("TestTimer", 1ms,