Merge "SF: VSyncDispatchTimerQueueEntry::update should not skip a frame" into main

Author: Treehugger Robot

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp

@@ -43,16 +43,6 @@ nsecs_t getExpectedCallbackTime(nsecs_t nextVsyncTime,
     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 @@ ScheduleResult VSyncDispatchTimerQueueEntry::schedule(VSyncDispatch::ScheduleTim
     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 @@ nsecs_t VSyncDispatchTimerQueueEntry::adjustVsyncIfNeeded(VSyncTracker& tracker,
     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 @@ void VSyncDispatchTimerQueueEntry::update(VSyncTracker& tracker, nsecs_t now) {
         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 @@ ScheduleResult VSyncDispatchTimerQueue::scheduleLocked(CallbackToken token,
      * 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);

services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h

@@ -69,7 +69,7 @@ class VSyncDispatchTimerQueueEntry {
 
     // 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,19 +83,22 @@ class VSyncDispatchTimerQueueEntry {
     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;
 
     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;
 

services/surfaceflinger/fuzzer/surfaceflinger_scheduler_fuzzer.cpp

@@ -175,7 +175,8 @@ void SchedulerFuzzer::fuzzVSyncDispatchTimerQueue() {
     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);

services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp

@@ -917,6 +917,8 @@ TEST_F(VSyncDispatchTimerQueueTest, skipsSchedulingIfTimerReschedulingIsImminent
 // 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);
@@ -938,6 +940,37 @@ TEST_F(VSyncDispatchTimerQueueTest, skipsSchedulingIfTimerReschedulingIsImminent
     EXPECT_THAT(cb.mCalls.size(), Eq(1));
 }
 
+// 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;
@@ -1344,14 +1377,17 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, reportsScheduledIfStillTime) {
                         .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());