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

Commit c94ca839 authored by Kevin DuBois's avatar Kevin DuBois
Browse files

SF: VSyncDispatch: correct vsync prediction drift 

Refine VSyncDispatch::schedule implementation so that refining
the prediction by small amounts would not lead to skipped callbacks.
The current implementation did not account for a case where
a valid vsync callback would be skipped. (exposed
in unit testing). Like the rest of VSyncDispatch, this
code is flagged off (ie, latent, not production code yet)

Fixes: 145213786
Bug: 146050690
Test: 6 new unit tests, 3 unit test change
Test: validation via systrace

Change-Id: I400fc5e3c181b49ab237b0dd0da2a62e38522fa0
parent f91e9236

services/surfaceflinger/Scheduler/VSyncDispatch.h

+1 −2
Original line number Diff line number Diff line
@@ -26,7 +26,7 @@ namespace android::scheduler { 
class TimeKeeper;

class VSyncTracker;



enum class ScheduleResult { Scheduled, ReScheduled, CannotSchedule, Error };

enum class ScheduleResult { Scheduled, CannotSchedule, Error };

enum class CancelResult { Cancelled, TooLate, Error };



/*
@@ -83,7 +83,6 @@ public: 
     * \param [in] earliestVsync   The targeted display time. This will be snapped to the closest

     *                             predicted vsync time after earliestVsync.

     * \return                     A ScheduleResult::Scheduled if callback was scheduled.

     *                             A ScheduleResult::ReScheduled if callback was rescheduled.

     *                             A ScheduleResult::CannotSchedule

     *                             if (workDuration - earliestVsync) is in the past, or

     *                             if a callback was dispatched for the predictedVsync already.

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp

+36 −12
Original line number Diff line number Diff line
@@ -29,8 +29,13 @@ VSyncTracker::~VSyncTracker() = default; 
TimeKeeper::~TimeKeeper() = default;



VSyncDispatchTimerQueueEntry::VSyncDispatchTimerQueueEntry(std::string const& name,

                                                           std::function<void(nsecs_t)> const& cb)

      : mName(name), mCallback(cb), mWorkDuration(0), mEarliestVsync(0) {}

                                                           std::function<void(nsecs_t)> const& cb,

                                                           nsecs_t minVsyncDistance)

      : mName(name),

        mCallback(cb),

        mWorkDuration(0),

        mEarliestVsync(0),

        mMinVsyncDistance(minVsyncDistance) {}



std::optional<nsecs_t> VSyncDispatchTimerQueueEntry::lastExecutedVsyncTarget() const {

    return mLastDispatchTime;
@@ -49,18 +54,28 @@ std::optional<nsecs_t> VSyncDispatchTimerQueueEntry::wakeupTime() const { 


ScheduleResult VSyncDispatchTimerQueueEntry::schedule(nsecs_t workDuration, nsecs_t earliestVsync,

                                                      VSyncTracker& tracker, nsecs_t now) {

    auto const nextVsyncTime =

    auto nextVsyncTime =

            tracker.nextAnticipatedVSyncTimeFrom(std::max(earliestVsync, now + workDuration));

    if (mLastDispatchTime >= nextVsyncTime) { // already dispatched a callback for this vsync

        return ScheduleResult::CannotSchedule;



    bool const wouldSkipAVsyncTarget =

            mArmedInfo && (nextVsyncTime > (mArmedInfo->mActualVsyncTime + mMinVsyncDistance));

    if (wouldSkipAVsyncTarget) {

        return ScheduleResult::Scheduled;

    }



    bool const alreadyDispatchedForVsync = mLastDispatchTime &&

            ((*mLastDispatchTime + mMinVsyncDistance) >= nextVsyncTime &&

             (*mLastDispatchTime - mMinVsyncDistance) <= nextVsyncTime);

    if (alreadyDispatchedForVsync) {

        nextVsyncTime =

                tracker.nextAnticipatedVSyncTimeFrom(*mLastDispatchTime + mMinVsyncDistance);

    }



    auto const nextWakeupTime = nextVsyncTime - workDuration;

    auto result = mArmedInfo ? ScheduleResult::ReScheduled : ScheduleResult::Scheduled;

    mWorkDuration = workDuration;

    mEarliestVsync = earliestVsync;

    mArmedInfo = {nextWakeupTime, nextVsyncTime};

    return result;

    return ScheduleResult::Scheduled;

}



void VSyncDispatchTimerQueueEntry::update(VSyncTracker& tracker, nsecs_t now) {
@@ -101,8 +116,12 @@ void VSyncDispatchTimerQueueEntry::ensureNotRunning() { 
}



VSyncDispatchTimerQueue::VSyncDispatchTimerQueue(std::unique_ptr<TimeKeeper> tk,

                                                 VSyncTracker& tracker, nsecs_t timerSlack)

      : mTimeKeeper(std::move(tk)), mTracker(tracker), mTimerSlack(timerSlack) {}

                                                 VSyncTracker& tracker, nsecs_t timerSlack,

                                                 nsecs_t minVsyncDistance)

      : mTimeKeeper(std::move(tk)),

        mTracker(tracker),

        mTimerSlack(timerSlack),

        mMinVsyncDistance(minVsyncDistance) {}



VSyncDispatchTimerQueue::~VSyncDispatchTimerQueue() {

    std::lock_guard<decltype(mMutex)> lk(mMutex);
@@ -187,7 +206,8 @@ VSyncDispatchTimerQueue::CallbackToken VSyncDispatchTimerQueue::registerCallback 
            mCallbacks

                    .emplace(++mCallbackToken,

                             std::make_shared<VSyncDispatchTimerQueueEntry>(callbackName,

                                                                            callbackFn))

                                                                            callbackFn,

                                                                            mMinVsyncDistance))

                    .first->first};

}
@@ -277,12 +297,16 @@ VSyncCallbackRegistration::~VSyncCallbackRegistration() { 
}



ScheduleResult VSyncCallbackRegistration::schedule(nsecs_t workDuration, nsecs_t earliestVsync) {

    if (!mValidToken) return ScheduleResult::Error;

    if (!mValidToken) {

        return ScheduleResult::Error;

    }

    return mDispatch.get().schedule(mToken, workDuration, earliestVsync);

}



CancelResult VSyncCallbackRegistration::cancel() {

    if (!mValidToken) return CancelResult::Error;

    if (!mValidToken) {

        return CancelResult::Error;

    }

    return mDispatch.get().cancel(mToken);

}

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h

+12 −2
Original line number Diff line number Diff line
@@ -36,7 +36,8 @@ public: 
    // Valid transition: disarmed -> armed ( when scheduled )

    // Valid transition: armed -> running -> disarmed ( when timer is called)

    // Valid transition: armed -> disarmed ( when cancelled )

    VSyncDispatchTimerQueueEntry(std::string const& name, std::function<void(nsecs_t)> const& fn);

    VSyncDispatchTimerQueueEntry(std::string const& name, std::function<void(nsecs_t)> const& fn,

                                 nsecs_t minVsyncDistance);

    std::string_view name() const;



    // Start: functions that are not threadsafe.
@@ -72,6 +73,7 @@ private: 


    nsecs_t mWorkDuration;

    nsecs_t mEarliestVsync;

    nsecs_t const mMinVsyncDistance;



    struct ArmingInfo {

        nsecs_t mActualWakeupTime;
@@ -91,8 +93,15 @@ private: 
 */

class VSyncDispatchTimerQueue : public VSyncDispatch {

public:

    // Constructs a VSyncDispatchTimerQueue.

    // \param[in] tk                    A timekeeper.

    // \param[in] tracker               A tracker.

    // \param[in] timerSlack            The threshold at which different similarly timed callbacks

    //                                  should be grouped into one wakeup.

    // \param[in] minVsyncDistance      The minimum distance between two vsync estimates before the

    //                                  vsyncs are considered the same vsync event.

    explicit VSyncDispatchTimerQueue(std::unique_ptr<TimeKeeper> tk, VSyncTracker& tracker,

                                     nsecs_t timerSlack);

                                     nsecs_t timerSlack, nsecs_t minVsyncDistance);

    ~VSyncDispatchTimerQueue();



    CallbackToken registerCallback(std::function<void(nsecs_t)> const& callbackFn,
@@ -119,6 +128,7 @@ private: 
    std::unique_ptr<TimeKeeper> const mTimeKeeper;

    VSyncTracker& mTracker;

    nsecs_t const mTimerSlack;

    nsecs_t const mMinVsyncDistance;



    std::mutex mutable mMutex;

    size_t mCallbackToken GUARDED_BY(mMutex) = 0;

services/surfaceflinger/Scheduler/VSyncReactor.cpp

+8 −3
Original line number Diff line number Diff line
@@ -14,6 +14,8 @@ 
 * limitations under the License.

 */



#undef LOG_TAG

#define LOG_TAG "VSyncReactor"

//#define LOG_NDEBUG 0

#include "VSyncReactor.h"

#include <log/log.h>
@@ -59,8 +61,9 @@ public: 
        mStopped = false;

        mOffset = offset;



        // TODO: (b/145213786) check the return code here sensibly

        mRegistration.schedule(calculateWorkload(), mLastCallTime);

        auto const schedule_result = mRegistration.schedule(calculateWorkload(), mLastCallTime);

        LOG_ALWAYS_FATAL_IF((schedule_result != ScheduleResult::Scheduled),

                            "Error scheduling callback: rc %X", schedule_result);

    }



    void setPeriod(nsecs_t period) {
@@ -91,7 +94,9 @@ private: 


        {

            std::lock_guard<std::mutex> lk(mMutex);

            mRegistration.schedule(calculateWorkload(), vsynctime);

            auto const schedule_result = mRegistration.schedule(calculateWorkload(), vsynctime);

            LOG_ALWAYS_FATAL_IF((schedule_result != ScheduleResult::Scheduled),

                                "Error rescheduling callback: rc %X", schedule_result);

        }

    }

services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp

+7 −3
Original line number Diff line number Diff line
@@ -92,6 +92,7 @@ private: 


struct VSyncDispatchRealtimeTest : testing::Test {

    static nsecs_t constexpr mDispatchGroupThreshold = toNs(100us);

    static nsecs_t constexpr mVsyncMoveThreshold = toNs(500us);

    static size_t constexpr mIterations = 20;

};
@@ -148,7 +149,8 @@ private: 


TEST_F(VSyncDispatchRealtimeTest, triple_alarm) {

    FixedRateIdealStubTracker tracker;

    VSyncDispatchTimerQueue dispatch(std::make_unique<Timer>(), tracker, mDispatchGroupThreshold);

    VSyncDispatchTimerQueue dispatch(std::make_unique<Timer>(), tracker, mDispatchGroupThreshold,

                                     mVsyncMoveThreshold);



    static size_t constexpr num_clients = 3;

    std::array<RepeatingCallbackReceiver, num_clients>
@@ -176,7 +178,8 @@ TEST_F(VSyncDispatchRealtimeTest, triple_alarm) { 
TEST_F(VSyncDispatchRealtimeTest, vascillating_vrr) {

    auto next_vsync_interval = toNs(3ms);

    VRRStubTracker tracker(next_vsync_interval);

    VSyncDispatchTimerQueue dispatch(std::make_unique<Timer>(), tracker, mDispatchGroupThreshold);

    VSyncDispatchTimerQueue dispatch(std::make_unique<Timer>(), tracker, mDispatchGroupThreshold,

                                     mVsyncMoveThreshold);



    RepeatingCallbackReceiver cb_receiver(dispatch, toNs(1ms));
@@ -193,7 +196,8 @@ TEST_F(VSyncDispatchRealtimeTest, vascillating_vrr) { 
// starts at 333hz, jumps to 200hz at frame 10

TEST_F(VSyncDispatchRealtimeTest, fixed_jump) {

    VRRStubTracker tracker(toNs(3ms));

    VSyncDispatchTimerQueue dispatch(std::make_unique<Timer>(), tracker, mDispatchGroupThreshold);

    VSyncDispatchTimerQueue dispatch(std::make_unique<Timer>(), tracker, mDispatchGroupThreshold,

                                     mVsyncMoveThreshold);



    RepeatingCallbackReceiver cb_receiver(dispatch, toNs(1ms));