diff options
8 files changed, 538 insertions, 58 deletions
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatch.h b/services/surfaceflinger/Scheduler/VSyncDispatch.h index e001080015..56b3252272 100644 --- a/services/surfaceflinger/Scheduler/VSyncDispatch.h +++ b/services/surfaceflinger/Scheduler/VSyncDispatch.h @@ -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. diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp index a79fe98a27..48f2abb7aa 100644 --- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp +++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp @@ -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); } diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h index fc78da3977..0c9b4fe53e 100644 --- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h +++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h @@ -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; diff --git a/services/surfaceflinger/Scheduler/VSyncReactor.cpp b/services/surfaceflinger/Scheduler/VSyncReactor.cpp index f2a7791ada..47e3f4f9e9 100644 --- a/services/surfaceflinger/Scheduler/VSyncReactor.cpp +++ b/services/surfaceflinger/Scheduler/VSyncReactor.cpp @@ -14,7 +14,11 @@ * limitations under the License. */ +#undef LOG_TAG +#define LOG_TAG "VSyncReactor" +//#define LOG_NDEBUG 0 #include "VSyncReactor.h" +#include <log/log.h> #include "TimeKeeper.h" #include "VSyncDispatch.h" #include "VSyncTracker.h" @@ -30,6 +34,87 @@ VSyncReactor::VSyncReactor(std::unique_ptr<Clock> clock, std::unique_ptr<VSyncDi mTracker(std::move(tracker)), mPendingLimit(pendingFenceLimit) {} +VSyncReactor::~VSyncReactor() = default; + +// The DispSync interface has a 'repeat this callback at rate' semantic. This object adapts +// VSyncDispatch's individually-scheduled callbacks so as to meet DispSync's existing semantic +// for now. +class CallbackRepeater { +public: + CallbackRepeater(VSyncDispatch& dispatch, DispSync::Callback* cb, const char* name, + nsecs_t period, nsecs_t offset, nsecs_t notBefore) + : mCallback(cb), + mRegistration(dispatch, + std::bind(&CallbackRepeater::callback, this, std::placeholders::_1), + std::string(name)), + mPeriod(period), + mOffset(offset), + mLastCallTime(notBefore) {} + + ~CallbackRepeater() { + std::lock_guard<std::mutex> lk(mMutex); + mRegistration.cancel(); + } + + void start(nsecs_t offset) { + std::lock_guard<std::mutex> lk(mMutex); + mStopped = false; + mOffset = offset; + + 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) { + std::lock_guard<std::mutex> lk(mMutex); + if (period == mPeriod) { + return; + } + mPeriod = period; + } + + void stop() { + std::lock_guard<std::mutex> lk(mMutex); + LOG_ALWAYS_FATAL_IF(mStopped, "DispSyncInterface misuse: callback already stopped"); + mStopped = true; + mRegistration.cancel(); + } + +private: + void callback(nsecs_t vsynctime) { + nsecs_t period = 0; + { + std::lock_guard<std::mutex> lk(mMutex); + period = mPeriod; + mLastCallTime = vsynctime; + } + + mCallback->onDispSyncEvent(vsynctime - period); + + { + std::lock_guard<std::mutex> lk(mMutex); + auto const schedule_result = mRegistration.schedule(calculateWorkload(), vsynctime); + LOG_ALWAYS_FATAL_IF((schedule_result != ScheduleResult::Scheduled), + "Error rescheduling callback: rc %X", schedule_result); + } + } + + // DispSync offsets are defined as time after the vsync before presentation. + // VSyncReactor workloads are defined as time before the intended presentation vsync. + // Note change in sign between the two defnitions. + nsecs_t calculateWorkload() REQUIRES(mMutex) { return mPeriod - mOffset; } + + DispSync::Callback* const mCallback; + + std::mutex mutable mMutex; + VSyncCallbackRegistration mRegistration GUARDED_BY(mMutex); + bool mStopped GUARDED_BY(mMutex) = false; + nsecs_t mPeriod GUARDED_BY(mMutex); + nsecs_t mOffset GUARDED_BY(mMutex); + nsecs_t mLastCallTime GUARDED_BY(mMutex); +}; + bool VSyncReactor::addPresentFence(const std::shared_ptr<FenceTime>& fence) { if (!fence) { return false; @@ -92,6 +177,9 @@ void VSyncReactor::setPeriod(nsecs_t period) { { std::lock_guard<std::mutex> lk(mMutex); mPeriodChangeInProgress = true; + for (auto& entry : mCallbacks) { + entry.second->setPeriod(period); + } } } @@ -114,4 +202,47 @@ bool VSyncReactor::addResyncSample(nsecs_t timestamp, bool* periodFlushed) { return false; } +status_t VSyncReactor::addEventListener(const char* name, nsecs_t phase, + DispSync::Callback* callback, + nsecs_t /* lastCallbackTime */) { + std::lock_guard<std::mutex> lk(mMutex); + auto it = mCallbacks.find(callback); + if (it == mCallbacks.end()) { + // TODO (b/146557561): resolve lastCallbackTime semantics in DispSync i/f. + static auto constexpr maxListeners = 3; + if (mCallbacks.size() >= maxListeners) { + ALOGE("callback %s not added, exceeded callback limit of %i (currently %zu)", name, + maxListeners, mCallbacks.size()); + return NO_MEMORY; + } + + auto const period = mTracker->currentPeriod(); + auto repeater = std::make_unique<CallbackRepeater>(*mDispatch, callback, name, period, + phase, mClock->now()); + it = mCallbacks.emplace(std::pair(callback, std::move(repeater))).first; + } + + it->second->start(phase); + return NO_ERROR; +} + +status_t VSyncReactor::removeEventListener(DispSync::Callback* callback, + nsecs_t* /* outLastCallback */) { + std::lock_guard<std::mutex> lk(mMutex); + auto const it = mCallbacks.find(callback); + LOG_ALWAYS_FATAL_IF(it == mCallbacks.end(), "callback %p not registered", callback); + + it->second->stop(); + return NO_ERROR; +} + +status_t VSyncReactor::changePhaseOffset(DispSync::Callback* callback, nsecs_t phase) { + std::lock_guard<std::mutex> lk(mMutex); + auto const it = mCallbacks.find(callback); + LOG_ALWAYS_FATAL_IF(it == mCallbacks.end(), "callback was %p not registered", callback); + + it->second->start(phase); + return NO_ERROR; +} + } // namespace android::scheduler diff --git a/services/surfaceflinger/Scheduler/VSyncReactor.h b/services/surfaceflinger/Scheduler/VSyncReactor.h index 786ee98cca..837eb75005 100644 --- a/services/surfaceflinger/Scheduler/VSyncReactor.h +++ b/services/surfaceflinger/Scheduler/VSyncReactor.h @@ -20,19 +20,23 @@ #include <ui/FenceTime.h> #include <memory> #include <mutex> +#include <unordered_map> #include <vector> +#include "DispSync.h" namespace android::scheduler { class Clock; class VSyncDispatch; class VSyncTracker; +class CallbackRepeater; // TODO (b/145217110): consider renaming. class VSyncReactor /* TODO (b/140201379): : public android::DispSync */ { public: VSyncReactor(std::unique_ptr<Clock> clock, std::unique_ptr<VSyncDispatch> dispatch, std::unique_ptr<VSyncTracker> tracker, size_t pendingFenceLimit); + ~VSyncReactor(); bool addPresentFence(const std::shared_ptr<FenceTime>& fence); void setIgnorePresentFences(bool ignoration); @@ -48,6 +52,11 @@ public: bool addResyncSample(nsecs_t timestamp, bool* periodFlushed); void endResync(); + status_t addEventListener(const char* name, nsecs_t phase, DispSync::Callback* callback, + nsecs_t lastCallbackTime); + status_t removeEventListener(DispSync::Callback* callback, nsecs_t* outLastCallback); + status_t changePhaseOffset(DispSync::Callback* callback, nsecs_t phase); + private: std::unique_ptr<Clock> const mClock; std::unique_ptr<VSyncDispatch> const mDispatch; @@ -58,6 +67,8 @@ private: bool mIgnorePresentFences GUARDED_BY(mMutex) = false; std::vector<std::shared_ptr<FenceTime>> mUnfiredFences GUARDED_BY(mMutex); bool mPeriodChangeInProgress GUARDED_BY(mMutex) = false; + std::unordered_map<DispSync::Callback*, std::unique_ptr<CallbackRepeater>> mCallbacks + GUARDED_BY(mMutex); }; } // namespace android::scheduler diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp index 484947d3f3..5846c77533 100644 --- a/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp +++ b/services/surfaceflinger/tests/unittests/VSyncDispatchRealtimeTest.cpp @@ -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)); diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp index d668a41bf1..5aff4296f9 100644 --- a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp +++ b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp @@ -196,16 +196,18 @@ protected: NiceMock<ControllableClock> mMockClock; static nsecs_t constexpr mDispatchGroupThreshold = 5; nsecs_t const mPeriod = 1000; + nsecs_t const mVsyncMoveThreshold = 300; NiceMock<MockVSyncTracker> mStubTracker{mPeriod}; - VSyncDispatchTimerQueue mDispatch{createTimeKeeper(), mStubTracker, mDispatchGroupThreshold}; + VSyncDispatchTimerQueue mDispatch{createTimeKeeper(), mStubTracker, mDispatchGroupThreshold, + mVsyncMoveThreshold}; }; TEST_F(VSyncDispatchTimerQueueTest, unregistersSetAlarmOnDestruction) { EXPECT_CALL(mMockClock, alarmIn(_, 900)); EXPECT_CALL(mMockClock, alarmCancel()); { - VSyncDispatchTimerQueue mDispatch{createTimeKeeper(), mStubTracker, - mDispatchGroupThreshold}; + VSyncDispatchTimerQueue mDispatch{createTimeKeeper(), mStubTracker, mDispatchGroupThreshold, + mVsyncMoveThreshold}; CountingCallback cb(mDispatch); EXPECT_EQ(mDispatch.schedule(cb, 100, 1000), ScheduleResult::Scheduled); } @@ -468,14 +470,24 @@ TEST_F(VSyncDispatchTimerQueueTest, callbackReentrancy) { TEST_F(VSyncDispatchTimerQueueTest, callbackReentrantWithPastWakeup) { VSyncDispatch::CallbackToken tmp; + std::optional<nsecs_t> lastTarget; tmp = mDispatch.registerCallback( - [&](auto) { - EXPECT_EQ(mDispatch.schedule(tmp, 400, 1000), ScheduleResult::CannotSchedule); + [&](auto timestamp) { + EXPECT_EQ(mDispatch.schedule(tmp, 400, timestamp - mVsyncMoveThreshold), + ScheduleResult::Scheduled); + EXPECT_EQ(mDispatch.schedule(tmp, 400, timestamp), ScheduleResult::Scheduled); + EXPECT_EQ(mDispatch.schedule(tmp, 400, timestamp + mVsyncMoveThreshold), + ScheduleResult::Scheduled); + lastTarget = timestamp; }, "oo"); mDispatch.schedule(tmp, 999, 1000); advanceToNextCallback(); + EXPECT_THAT(lastTarget, Eq(1000)); + + advanceToNextCallback(); + EXPECT_THAT(lastTarget, Eq(2000)); } TEST_F(VSyncDispatchTimerQueueTest, modificationsAroundVsyncTime) { @@ -547,10 +559,33 @@ TEST_F(VSyncDispatchTimerQueueTest, makingUpIdsError) { EXPECT_THAT(mDispatch.cancel(token), Eq(CancelResult::Error)); } -TEST_F(VSyncDispatchTimerQueueTest, distinguishesScheduleAndReschedule) { +TEST_F(VSyncDispatchTimerQueueTest, canMoveCallbackBackwardsInTime) { CountingCallback cb0(mDispatch); EXPECT_EQ(mDispatch.schedule(cb0, 500, 1000), ScheduleResult::Scheduled); - EXPECT_EQ(mDispatch.schedule(cb0, 100, 1000), ScheduleResult::ReScheduled); + EXPECT_EQ(mDispatch.schedule(cb0, 100, 1000), ScheduleResult::Scheduled); +} + +// b/1450138150 +TEST_F(VSyncDispatchTimerQueueTest, doesNotMoveCallbackBackwardsAndSkipAScheduledTargetVSync) { + EXPECT_CALL(mMockClock, alarmIn(_, 500)); + CountingCallback cb(mDispatch); + EXPECT_EQ(mDispatch.schedule(cb, 500, 1000), ScheduleResult::Scheduled); + mMockClock.advanceBy(400); + + EXPECT_EQ(mDispatch.schedule(cb, 800, 1000), ScheduleResult::Scheduled); + advanceToNextCallback(); + ASSERT_THAT(cb.mCalls.size(), Eq(1)); +} + +TEST_F(VSyncDispatchTimerQueueTest, targetOffsetMovingBackALittleCanStillSchedule) { + EXPECT_CALL(mStubTracker, nextAnticipatedVSyncTimeFrom(1000)) + .Times(2) + .WillOnce(Return(1000)) + .WillOnce(Return(1002)); + CountingCallback cb(mDispatch); + EXPECT_EQ(mDispatch.schedule(cb, 500, 1000), ScheduleResult::Scheduled); + mMockClock.advanceBy(400); + EXPECT_EQ(mDispatch.schedule(cb, 400, 1000), ScheduleResult::Scheduled); } TEST_F(VSyncDispatchTimerQueueTest, canScheduleNegativeOffsetAgainstDifferentPeriods) { @@ -570,13 +605,15 @@ TEST_F(VSyncDispatchTimerQueueTest, canScheduleLargeNegativeOffset) { EXPECT_EQ(mDispatch.schedule(cb0, 1900, 2000), ScheduleResult::Scheduled); } -TEST_F(VSyncDispatchTimerQueueTest, cannotScheduleDoesNotAffectSchedulingState) { +TEST_F(VSyncDispatchTimerQueueTest, scheduleUpdatesDoesNotAffectSchedulingState) { EXPECT_CALL(mMockClock, alarmIn(_, 600)); CountingCallback cb(mDispatch); EXPECT_EQ(mDispatch.schedule(cb, 400, 1000), ScheduleResult::Scheduled); + + EXPECT_EQ(mDispatch.schedule(cb, 1400, 1000), ScheduleResult::Scheduled); + advanceToNextCallback(); - EXPECT_EQ(mDispatch.schedule(cb, 100, 1000), ScheduleResult::CannotSchedule); } TEST_F(VSyncDispatchTimerQueueTest, helperMove) { @@ -612,19 +649,22 @@ TEST_F(VSyncDispatchTimerQueueTest, helperMoveAssign) { class VSyncDispatchTimerQueueEntryTest : public testing::Test { protected: nsecs_t const mPeriod = 1000; + nsecs_t const mVsyncMoveThreshold = 200; NiceMock<MockVSyncTracker> mStubTracker{mPeriod}; }; TEST_F(VSyncDispatchTimerQueueEntryTest, stateAfterInitialization) { std::string name("basicname"); - VSyncDispatchTimerQueueEntry entry(name, [](auto) {}); + VSyncDispatchTimerQueueEntry entry( + name, [](auto) {}, mVsyncMoveThreshold); EXPECT_THAT(entry.name(), Eq(name)); EXPECT_FALSE(entry.lastExecutedVsyncTarget()); EXPECT_FALSE(entry.wakeupTime()); } TEST_F(VSyncDispatchTimerQueueEntryTest, stateScheduling) { - VSyncDispatchTimerQueueEntry entry("test", [](auto) {}); + VSyncDispatchTimerQueueEntry entry( + "test", [](auto) {}, mVsyncMoveThreshold); EXPECT_FALSE(entry.wakeupTime()); EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); @@ -643,7 +683,8 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, stateSchedulingReallyLongWakeupLatency) EXPECT_CALL(mStubTracker, nextAnticipatedVSyncTimeFrom(now + duration)) .Times(1) .WillOnce(Return(10000)); - VSyncDispatchTimerQueueEntry entry("test", [](auto) {}); + VSyncDispatchTimerQueueEntry entry( + "test", [](auto) {}, mVsyncMoveThreshold); EXPECT_FALSE(entry.wakeupTime()); EXPECT_THAT(entry.schedule(500, 994, mStubTracker, now), Eq(ScheduleResult::Scheduled)); @@ -655,10 +696,13 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, stateSchedulingReallyLongWakeupLatency) TEST_F(VSyncDispatchTimerQueueEntryTest, runCallback) { auto callCount = 0; auto calledTime = 0; - VSyncDispatchTimerQueueEntry entry("test", [&](auto time) { - callCount++; - calledTime = time; - }); + VSyncDispatchTimerQueueEntry entry( + "test", + [&](auto time) { + callCount++; + calledTime = time; + }, + mVsyncMoveThreshold); EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); auto const wakeup = entry.wakeupTime(); @@ -681,7 +725,8 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, updateCallback) { .WillOnce(Return(1000)) .WillOnce(Return(1020)); - VSyncDispatchTimerQueueEntry entry("test", [](auto) {}); + VSyncDispatchTimerQueueEntry entry( + "test", [](auto) {}, mVsyncMoveThreshold); EXPECT_FALSE(entry.wakeupTime()); entry.update(mStubTracker, 0); @@ -699,7 +744,8 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, updateCallback) { } TEST_F(VSyncDispatchTimerQueueEntryTest, skipsUpdateIfJustScheduled) { - VSyncDispatchTimerQueueEntry entry("test", [](auto) {}); + VSyncDispatchTimerQueueEntry entry( + "test", [](auto) {}, mVsyncMoveThreshold); EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); entry.update(mStubTracker, 0); @@ -708,19 +754,52 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, skipsUpdateIfJustScheduled) { EXPECT_THAT(*wakeup, Eq(wakeup)); } -TEST_F(VSyncDispatchTimerQueueEntryTest, reportsCannotScheduleIfMissedOpportunity) { - VSyncDispatchTimerQueueEntry entry("test", [](auto) {}); +TEST_F(VSyncDispatchTimerQueueEntryTest, willSnapToNextTargettableVSync) { + VSyncDispatchTimerQueueEntry entry( + "test", [](auto) {}, mVsyncMoveThreshold); EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); - entry.executing(); - EXPECT_THAT(entry.schedule(200, 500, mStubTracker, 0), Eq(ScheduleResult::CannotSchedule)); - EXPECT_THAT(entry.schedule(50, 500, mStubTracker, 0), Eq(ScheduleResult::CannotSchedule)); + entry.executing(); // 1000 is executing + // had 1000 not been executing, this could have been scheduled for time 800. + EXPECT_THAT(entry.schedule(200, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); + EXPECT_THAT(*entry.wakeupTime(), Eq(1800)); + + EXPECT_THAT(entry.schedule(50, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); + EXPECT_THAT(*entry.wakeupTime(), Eq(1950)); + EXPECT_THAT(entry.schedule(200, 1001, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); + EXPECT_THAT(*entry.wakeupTime(), Eq(1800)); +} + +TEST_F(VSyncDispatchTimerQueueEntryTest, + willRequestNextEstimateWhenSnappingToNextTargettableVSync) { + VSyncDispatchTimerQueueEntry entry( + "test", [](auto) {}, mVsyncMoveThreshold); + + Sequence seq; + EXPECT_CALL(mStubTracker, nextAnticipatedVSyncTimeFrom(500)) + .InSequence(seq) + .WillOnce(Return(1000)); + EXPECT_CALL(mStubTracker, nextAnticipatedVSyncTimeFrom(500)) + .InSequence(seq) + .WillOnce(Return(1000)); + EXPECT_CALL(mStubTracker, nextAnticipatedVSyncTimeFrom(1000 + mVsyncMoveThreshold)) + .InSequence(seq) + .WillOnce(Return(2000)); + + EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); + + entry.executing(); // 1000 is executing + + EXPECT_THAT(entry.schedule(200, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); } -TEST_F(VSyncDispatchTimerQueueEntryTest, reportsReScheduleIfStillTime) { - VSyncDispatchTimerQueueEntry entry("test", [](auto) {}); +TEST_F(VSyncDispatchTimerQueueEntryTest, reportsScheduledIfStillTime) { + VSyncDispatchTimerQueueEntry entry( + "test", [](auto) {}, mVsyncMoveThreshold); EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); - EXPECT_THAT(entry.schedule(200, 500, mStubTracker, 0), Eq(ScheduleResult::ReScheduled)); + EXPECT_THAT(entry.schedule(200, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); + EXPECT_THAT(entry.schedule(50, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); + EXPECT_THAT(entry.schedule(1200, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled)); } } // namespace android::scheduler diff --git a/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp b/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp index 84df019d53..537cc80bed 100644 --- a/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp +++ b/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp @@ -122,21 +122,53 @@ std::shared_ptr<FenceTime> generateSignalledFenceWithTime(nsecs_t time) { return ft; } +class StubCallback : public DispSync::Callback { +public: + void onDispSyncEvent(nsecs_t when) final { + std::lock_guard<std::mutex> lk(mMutex); + mLastCallTime = when; + } + std::optional<nsecs_t> lastCallTime() const { + std::lock_guard<std::mutex> lk(mMutex); + return mLastCallTime; + } + +private: + std::mutex mutable mMutex; + std::optional<nsecs_t> mLastCallTime GUARDED_BY(mMutex); +}; + class VSyncReactorTest : public testing::Test { protected: VSyncReactorTest() - : mMockDispatch(std::make_shared<MockVSyncDispatch>()), + : mMockDispatch(std::make_shared<NiceMock<MockVSyncDispatch>>()), mMockTracker(std::make_shared<NiceMock<MockVSyncTracker>>()), mMockClock(std::make_shared<NiceMock<MockClock>>()), mReactor(std::make_unique<ClockWrapper>(mMockClock), std::make_unique<VSyncDispatchWrapper>(mMockDispatch), - std::make_unique<VSyncTrackerWrapper>(mMockTracker), kPendingLimit) {} + std::make_unique<VSyncTrackerWrapper>(mMockTracker), kPendingLimit) { + ON_CALL(*mMockClock, now()).WillByDefault(Return(mFakeNow)); + ON_CALL(*mMockTracker, currentPeriod()).WillByDefault(Return(period)); + } std::shared_ptr<MockVSyncDispatch> mMockDispatch; std::shared_ptr<MockVSyncTracker> mMockTracker; std::shared_ptr<MockClock> mMockClock; static constexpr size_t kPendingLimit = 3; - static constexpr nsecs_t dummyTime = 47; + static constexpr nsecs_t mDummyTime = 47; + static constexpr nsecs_t mPhase = 3000; + static constexpr nsecs_t mAnotherPhase = 5200; + static constexpr nsecs_t period = 10000; + static constexpr nsecs_t mAnotherPeriod = 23333; + static constexpr nsecs_t mFakeCbTime = 2093; + static constexpr nsecs_t mFakeNow = 2214; + static constexpr const char mName[] = "callbacky"; + VSyncDispatch::CallbackToken const mFakeToken{2398}; + + nsecs_t lastCallbackTime = 0; + StubCallback outerCb; + std::function<void(nsecs_t)> innerCb; + VSyncReactor mReactor; }; @@ -149,8 +181,8 @@ TEST_F(VSyncReactorTest, addingInvalidFenceSignalsNeedsMoreInfo) { } TEST_F(VSyncReactorTest, addingSignalledFenceAddsToTracker) { - EXPECT_CALL(*mMockTracker, addVsyncTimestamp(dummyTime)); - EXPECT_FALSE(mReactor.addPresentFence(generateSignalledFenceWithTime(dummyTime))); + EXPECT_CALL(*mMockTracker, addVsyncTimestamp(mDummyTime)); + EXPECT_FALSE(mReactor.addPresentFence(generateSignalledFenceWithTime(mDummyTime))); } TEST_F(VSyncReactorTest, addingPendingFenceAddsSignalled) { @@ -161,9 +193,9 @@ TEST_F(VSyncReactorTest, addingPendingFenceAddsSignalled) { EXPECT_FALSE(mReactor.addPresentFence(pendingFence)); Mock::VerifyAndClearExpectations(mMockTracker.get()); - signalFenceWithTime(pendingFence, dummyTime); + signalFenceWithTime(pendingFence, mDummyTime); - EXPECT_CALL(*mMockTracker, addVsyncTimestamp(dummyTime)); + EXPECT_CALL(*mMockTracker, addVsyncTimestamp(mDummyTime)); EXPECT_CALL(*mMockTracker, addVsyncTimestamp(anotherDummyTime)); EXPECT_FALSE(mReactor.addPresentFence(generateSignalledFenceWithTime(anotherDummyTime))); } @@ -193,15 +225,15 @@ TEST_F(VSyncReactorTest, limitsPendingFences) { TEST_F(VSyncReactorTest, ignoresPresentFencesWhenToldTo) { static constexpr size_t aFewTimes = 8; - EXPECT_CALL(*mMockTracker, addVsyncTimestamp(dummyTime)).Times(1); + EXPECT_CALL(*mMockTracker, addVsyncTimestamp(mDummyTime)).Times(1); mReactor.setIgnorePresentFences(true); for (auto i = 0; i < aFewTimes; i++) { - mReactor.addPresentFence(generateSignalledFenceWithTime(dummyTime)); + mReactor.addPresentFence(generateSignalledFenceWithTime(mDummyTime)); } mReactor.setIgnorePresentFences(false); - EXPECT_FALSE(mReactor.addPresentFence(generateSignalledFenceWithTime(dummyTime))); + EXPECT_FALSE(mReactor.addPresentFence(generateSignalledFenceWithTime(mDummyTime))); } TEST_F(VSyncReactorTest, queriesTrackerForNextRefreshNow) { @@ -227,11 +259,11 @@ TEST_F(VSyncReactorTest, queriesTrackerForExpectedPresentTime) { TEST_F(VSyncReactorTest, queriesTrackerForNextRefreshFuture) { nsecs_t const fakeTimestamp = 4839; nsecs_t const fakePeriod = 1010; - nsecs_t const fakeNow = 2214; + nsecs_t const mFakeNow = 2214; int const numPeriodsOut = 3; - EXPECT_CALL(*mMockClock, now()).WillOnce(Return(fakeNow)); + EXPECT_CALL(*mMockClock, now()).WillOnce(Return(mFakeNow)); EXPECT_CALL(*mMockTracker, currentPeriod()).WillOnce(Return(fakePeriod)); - EXPECT_CALL(*mMockTracker, nextAnticipatedVSyncTimeFrom(fakeNow + numPeriodsOut * fakePeriod)) + EXPECT_CALL(*mMockTracker, nextAnticipatedVSyncTimeFrom(mFakeNow + numPeriodsOut * fakePeriod)) .WillOnce(Return(fakeTimestamp)); EXPECT_THAT(mReactor.computeNextRefresh(numPeriodsOut), Eq(fakeTimestamp)); } @@ -267,4 +299,194 @@ TEST_F(VSyncReactorTest, addResyncSamplePeriodChanges) { EXPECT_TRUE(periodFlushed); } +static nsecs_t computeWorkload(nsecs_t period, nsecs_t phase) { + return period - phase; +} + +TEST_F(VSyncReactorTest, addEventListener) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(Return(mFakeToken)); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, cancel(mFakeToken)).Times(2).InSequence(seq); + EXPECT_CALL(*mMockDispatch, unregisterCallback(mFakeToken)).InSequence(seq); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + mReactor.removeEventListener(&outerCb, &lastCallbackTime); +} + +TEST_F(VSyncReactorTest, addEventListenerTwiceChangesPhase) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(Return(mFakeToken)); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, + schedule(mFakeToken, computeWorkload(period, mAnotherPhase), _)) // mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, cancel(mFakeToken)).InSequence(seq); + EXPECT_CALL(*mMockDispatch, unregisterCallback(mFakeToken)).InSequence(seq); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + mReactor.addEventListener(mName, mAnotherPhase, &outerCb, lastCallbackTime); +} + +TEST_F(VSyncReactorTest, eventListenerGetsACallbackAndReschedules) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(DoAll(SaveArg<0>(&innerCb), Return(mFakeToken))); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeCbTime)) + .Times(2) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, cancel(mFakeToken)).InSequence(seq); + EXPECT_CALL(*mMockDispatch, unregisterCallback(mFakeToken)).InSequence(seq); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + ASSERT_TRUE(innerCb); + innerCb(mFakeCbTime); + innerCb(mFakeCbTime); +} + +TEST_F(VSyncReactorTest, callbackTimestampReadapted) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, _)) + .InSequence(seq) + .WillOnce(DoAll(SaveArg<0>(&innerCb), Return(mFakeToken))); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeCbTime)) + .InSequence(seq); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + ASSERT_TRUE(innerCb); + innerCb(mFakeCbTime); + EXPECT_THAT(outerCb.lastCallTime(), Optional(mFakeCbTime - period)); +} + +TEST_F(VSyncReactorTest, eventListenersRemovedOnDestruction) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(Return(mFakeToken)); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, cancel(mFakeToken)).InSequence(seq); + EXPECT_CALL(*mMockDispatch, unregisterCallback(mFakeToken)).InSequence(seq); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); +} + +TEST_F(VSyncReactorTest, addEventListenerChangePeriod) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(Return(mFakeToken)); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, + schedule(mFakeToken, computeWorkload(period, mAnotherPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockDispatch, cancel(mFakeToken)).InSequence(seq); + EXPECT_CALL(*mMockDispatch, unregisterCallback(mFakeToken)).InSequence(seq); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + mReactor.addEventListener(mName, mAnotherPhase, &outerCb, lastCallbackTime); +} + +TEST_F(VSyncReactorTest, changingPeriodChangesOfsetsOnNextCb) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(Return(mFakeToken)); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), mFakeNow)) + .InSequence(seq); + EXPECT_CALL(*mMockTracker, setPeriod(mAnotherPeriod)); + EXPECT_CALL(*mMockDispatch, + schedule(mFakeToken, computeWorkload(mAnotherPeriod, mPhase), mFakeNow)) + .InSequence(seq); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + mReactor.setPeriod(mAnotherPeriod); + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); +} + +TEST_F(VSyncReactorTest, offsetsAppliedOnNextOpportunity) { + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(DoAll(SaveArg<0>(&innerCb), Return(mFakeToken))); + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mPhase), _)) + .InSequence(seq) + .WillOnce(Return(ScheduleResult::Scheduled)); + + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mAnotherPhase), _)) + .InSequence(seq) + .WillOnce(Return(ScheduleResult::Scheduled)); + + EXPECT_CALL(*mMockDispatch, schedule(mFakeToken, computeWorkload(period, mAnotherPhase), _)) + .InSequence(seq) + .WillOnce(Return(ScheduleResult::Scheduled)); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + mReactor.changePhaseOffset(&outerCb, mAnotherPhase); + ASSERT_TRUE(innerCb); + innerCb(mFakeCbTime); +} + +TEST_F(VSyncReactorTest, negativeOffsetsApplied) { + nsecs_t const negativePhase = -4000; + Sequence seq; + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .InSequence(seq) + .WillOnce(Return(mFakeToken)); + EXPECT_CALL(*mMockDispatch, + schedule(mFakeToken, computeWorkload(period, negativePhase), mFakeNow)) + .InSequence(seq); + mReactor.addEventListener(mName, negativePhase, &outerCb, lastCallbackTime); +} + +using VSyncReactorDeathTest = VSyncReactorTest; +TEST_F(VSyncReactorDeathTest, invalidRemoval) { + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + mReactor.removeEventListener(&outerCb, &lastCallbackTime); + EXPECT_DEATH(mReactor.removeEventListener(&outerCb, &lastCallbackTime), ".*"); +} + +TEST_F(VSyncReactorDeathTest, invalidChange) { + EXPECT_DEATH(mReactor.changePhaseOffset(&outerCb, mPhase), ".*"); + + // the current DispSync-interface usage pattern has evolved around an implementation quirk, + // which is a callback is assumed to always exist, and it is valid api usage to change the + // offset of an object that is in the removed state. + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + mReactor.removeEventListener(&outerCb, &lastCallbackTime); + mReactor.changePhaseOffset(&outerCb, mPhase); +} + +TEST_F(VSyncReactorDeathTest, cannotScheduleOnRegistration) { + ON_CALL(*mMockDispatch, schedule(_, _, _)) + .WillByDefault(Return(ScheduleResult::CannotSchedule)); + EXPECT_DEATH(mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime), ".*"); +} + +TEST_F(VSyncReactorDeathTest, cannotScheduleOnCallback) { + EXPECT_CALL(*mMockDispatch, registerCallback(_, std::string(mName))) + .WillOnce(DoAll(SaveArg<0>(&innerCb), Return(mFakeToken))); + EXPECT_CALL(*mMockDispatch, schedule(_, _, _)).WillOnce(Return(ScheduleResult::Scheduled)); + + mReactor.addEventListener(mName, mPhase, &outerCb, lastCallbackTime); + ASSERT_TRUE(innerCb); + Mock::VerifyAndClearExpectations(mMockDispatch.get()); + + ON_CALL(*mMockDispatch, schedule(_, _, _)) + .WillByDefault(Return(ScheduleResult::CannotSchedule)); + EXPECT_DEATH(innerCb(mFakeCbTime), ".*"); +} + } // namespace android::scheduler |