SF: VSyncDispatchTimerQueueEntry::update should not skip a frame
Change-Id: Ic3e55c073a941a36a5bae9f502657e6383caef72
Test: SysUI Jank Regression
Bug: 324149129
Bug: 273702768
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
index f5aaf06..b92fa24 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
@@ -43,16 +43,6 @@
return nextVsyncTime - timing.readyDuration - timing.workDuration;
}
-nsecs_t getExpectedCallbackTime(VSyncTracker& tracker, nsecs_t now,
- const VSyncDispatch::ScheduleTiming& timing) {
- const auto nextVsyncTime =
- tracker.nextAnticipatedVSyncTimeFrom(std::max(timing.lastVsync,
- now + timing.workDuration +
- timing.readyDuration),
- timing.lastVsync);
- return getExpectedCallbackTime(nextVsyncTime, timing);
-}
-
} // namespace
VSyncDispatch::~VSyncDispatch() = default;
@@ -128,8 +118,11 @@
return nextWakeupTime;
}
-void VSyncDispatchTimerQueueEntry::addPendingWorkloadUpdate(VSyncDispatch::ScheduleTiming timing) {
+nsecs_t VSyncDispatchTimerQueueEntry::addPendingWorkloadUpdate(
+ VSyncTracker& tracker, nsecs_t now, VSyncDispatch::ScheduleTiming timing) {
mWorkloadUpdateInfo = timing;
+ const auto armedInfo = update(tracker, now, timing, mArmedInfo);
+ return armedInfo.mActualWakeupTime;
}
bool VSyncDispatchTimerQueueEntry::hasPendingWorkloadUpdate() const {
@@ -157,6 +150,31 @@
return nextVsyncTime;
}
+auto VSyncDispatchTimerQueueEntry::update(VSyncTracker& tracker, nsecs_t now,
+ VSyncDispatch::ScheduleTiming timing,
+ std::optional<ArmingInfo> armedInfo) const -> ArmingInfo {
+ const auto earliestReadyBy = now + timing.workDuration + timing.readyDuration;
+ const auto earliestVsync = std::max(earliestReadyBy, timing.lastVsync);
+
+ const auto nextVsyncTime =
+ adjustVsyncIfNeeded(tracker, /*nextVsyncTime*/
+ tracker.nextAnticipatedVSyncTimeFrom(earliestVsync,
+ timing.lastVsync));
+ const auto nextReadyTime = nextVsyncTime - timing.readyDuration;
+ const auto nextWakeupTime = nextReadyTime - timing.workDuration;
+
+ bool const wouldSkipAVsyncTarget =
+ armedInfo && (nextVsyncTime > (armedInfo->mActualVsyncTime + mMinVsyncDistance));
+ bool const wouldSkipAWakeup =
+ armedInfo && (nextWakeupTime > (armedInfo->mActualWakeupTime + mMinVsyncDistance));
+ if (FlagManager::getInstance().dont_skip_on_early_ro() &&
+ (wouldSkipAVsyncTarget || wouldSkipAWakeup)) {
+ return *armedInfo;
+ }
+
+ return ArmingInfo{nextWakeupTime, nextVsyncTime, nextReadyTime};
+}
+
void VSyncDispatchTimerQueueEntry::update(VSyncTracker& tracker, nsecs_t now) {
if (!mArmedInfo && !mWorkloadUpdateInfo) {
return;
@@ -167,17 +185,7 @@
mWorkloadUpdateInfo.reset();
}
- const auto earliestReadyBy = now + mScheduleTiming.workDuration + mScheduleTiming.readyDuration;
- const auto earliestVsync = std::max(earliestReadyBy, mScheduleTiming.lastVsync);
-
- const auto nextVsyncTime =
- adjustVsyncIfNeeded(tracker, /*nextVsyncTime*/
- tracker.nextAnticipatedVSyncTimeFrom(earliestVsync,
- mScheduleTiming.lastVsync));
- const auto nextReadyTime = nextVsyncTime - mScheduleTiming.readyDuration;
- const auto nextWakeupTime = nextReadyTime - mScheduleTiming.workDuration;
-
- mArmedInfo = {nextWakeupTime, nextVsyncTime, nextReadyTime};
+ mArmedInfo = update(tracker, now, mScheduleTiming, mArmedInfo);
}
void VSyncDispatchTimerQueueEntry::disarm() {
@@ -394,8 +402,7 @@
* timer recalculation to avoid cancelling a callback that is about to fire. */
auto const rearmImminent = now > mIntendedWakeupTime;
if (CC_UNLIKELY(rearmImminent)) {
- callback->addPendingWorkloadUpdate(scheduleTiming);
- return getExpectedCallbackTime(*mTracker, now, scheduleTiming);
+ return callback->addPendingWorkloadUpdate(*mTracker, now, scheduleTiming);
}
const ScheduleResult result = callback->schedule(scheduleTiming, *mTracker, now);
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
index 81c746e..b5ddd25 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
@@ -69,7 +69,7 @@
// Adds a pending upload of the earliestVSync and workDuration that will be applied on the next
// call to update()
- void addPendingWorkloadUpdate(VSyncDispatch::ScheduleTiming);
+ nsecs_t addPendingWorkloadUpdate(VSyncTracker&, nsecs_t now, VSyncDispatch::ScheduleTiming);
// Checks if there is a pending update to the workload, returning true if so.
bool hasPendingWorkloadUpdate() const;
@@ -83,7 +83,15 @@
void dump(std::string& result) const;
private:
+ struct ArmingInfo {
+ nsecs_t mActualWakeupTime;
+ nsecs_t mActualVsyncTime;
+ nsecs_t mActualReadyTime;
+ };
+
nsecs_t adjustVsyncIfNeeded(VSyncTracker& tracker, nsecs_t nextVsyncTime) const;
+ ArmingInfo update(VSyncTracker&, nsecs_t now, VSyncDispatch::ScheduleTiming,
+ std::optional<ArmingInfo>) const;
const std::string mName;
const VSyncDispatch::Callback mCallback;
@@ -91,11 +99,6 @@
VSyncDispatch::ScheduleTiming mScheduleTiming;
const nsecs_t mMinVsyncDistance;
- struct ArmingInfo {
- nsecs_t mActualWakeupTime;
- nsecs_t mActualVsyncTime;
- nsecs_t mActualReadyTime;
- };
std::optional<ArmingInfo> mArmedInfo;
std::optional<nsecs_t> mLastDispatchTime;
diff --git a/services/surfaceflinger/fuzzer/surfaceflinger_scheduler_fuzzer.cpp b/services/surfaceflinger/fuzzer/surfaceflinger_scheduler_fuzzer.cpp
index 8cd6e1b..ff2ee7e 100644
--- a/services/surfaceflinger/fuzzer/surfaceflinger_scheduler_fuzzer.cpp
+++ b/services/surfaceflinger/fuzzer/surfaceflinger_scheduler_fuzzer.cpp
@@ -175,7 +175,8 @@
auto const wakeup = entry.wakeupTime();
auto const ready = entry.readyTime();
entry.callback(entry.executing(), *wakeup, *ready);
- entry.addPendingWorkloadUpdate({.workDuration = mFdp.ConsumeIntegral<nsecs_t>(),
+ entry.addPendingWorkloadUpdate(*stubTracker, 0,
+ {.workDuration = mFdp.ConsumeIntegral<nsecs_t>(),
.readyDuration = mFdp.ConsumeIntegral<nsecs_t>(),
.lastVsync = mFdp.ConsumeIntegral<nsecs_t>()});
dump<scheduler::VSyncDispatchTimerQueueEntry>(&entry, &mFdp);
diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
index c9a4094..eb4e84e 100644
--- a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
@@ -917,6 +917,8 @@
// If the same callback tries to reschedule itself after it's too late, timer opts to apply the
// update later, as opposed to blocking the calling thread.
TEST_F(VSyncDispatchTimerQueueTest, skipsSchedulingIfTimerReschedulingIsImminentSameCallback) {
+ SET_FLAG_FOR_TEST(flags::dont_skip_on_early_ro, false);
+
Sequence seq;
EXPECT_CALL(mMockClock, alarmAt(_, 600)).InSequence(seq);
EXPECT_CALL(mMockClock, alarmAt(_, 1630)).InSequence(seq);
@@ -939,6 +941,37 @@
}
// b/154303580.
+// If the same callback tries to reschedule itself after it's too late, timer opts to apply the
+// update later, as opposed to blocking the calling thread.
+TEST_F(VSyncDispatchTimerQueueTest, doesntSkipSchedulingIfTimerReschedulingIsImminentSameCallback) {
+ SET_FLAG_FOR_TEST(flags::dont_skip_on_early_ro, true);
+
+ Sequence seq;
+ EXPECT_CALL(mMockClock, alarmAt(_, 600)).InSequence(seq);
+ EXPECT_CALL(mMockClock, alarmAt(_, 1630)).InSequence(seq);
+ CountingCallback cb(mDispatch);
+
+ auto result =
+ mDispatch->schedule(cb, {.workDuration = 400, .readyDuration = 0, .lastVsync = 1000});
+ EXPECT_TRUE(result.has_value());
+ EXPECT_EQ(600, *result);
+
+ mMockClock.setLag(100);
+ mMockClock.advanceBy(620);
+
+ result = mDispatch->schedule(cb, {.workDuration = 370, .readyDuration = 0, .lastVsync = 2000});
+ EXPECT_TRUE(result.has_value());
+ EXPECT_EQ(600, *result);
+ mMockClock.advanceBy(80);
+
+ ASSERT_EQ(1, cb.mCalls.size());
+ EXPECT_EQ(1000, cb.mCalls[0]);
+
+ ASSERT_EQ(1, cb.mWakeupTime.size());
+ EXPECT_EQ(600, cb.mWakeupTime[0]);
+}
+
+// b/154303580.
TEST_F(VSyncDispatchTimerQueueTest, skipsRearmingWhenNotNextScheduled) {
Sequence seq;
EXPECT_CALL(mMockClock, alarmAt(_, 600)).InSequence(seq);
@@ -1344,14 +1377,17 @@
.has_value());
}
-TEST_F(VSyncDispatchTimerQueueEntryTest, storesPendingUpdatesUntilUpdate) {
+TEST_F(VSyncDispatchTimerQueueEntryTest, storesPendingUpdatesUntilUpdateAndDontSkip) {
static constexpr auto effectualOffset = 200;
VSyncDispatchTimerQueueEntry entry(
"test", [](auto, auto, auto) {}, mVsyncMoveThreshold);
EXPECT_FALSE(entry.hasPendingWorkloadUpdate());
- entry.addPendingWorkloadUpdate({.workDuration = 100, .readyDuration = 0, .lastVsync = 400});
- entry.addPendingWorkloadUpdate(
- {.workDuration = effectualOffset, .readyDuration = 0, .lastVsync = 400});
+ entry.addPendingWorkloadUpdate(*mStubTracker.get(), 0,
+ {.workDuration = 100, .readyDuration = 0, .lastVsync = 400});
+ entry.addPendingWorkloadUpdate(*mStubTracker.get(), 0,
+ {.workDuration = effectualOffset,
+ .readyDuration = 0,
+ .lastVsync = 400});
EXPECT_TRUE(entry.hasPendingWorkloadUpdate());
entry.update(*mStubTracker.get(), 0);
EXPECT_FALSE(entry.hasPendingWorkloadUpdate());