SF: correct latent problem in VSyncDispatch

A negative offset calculation was incorrect in the VSyncDispatch
system. (this is code that is flagged off and in development).
Flaw was exposed in unit testing; this test corrects issue so that
the negative offset is correctly triggered.

Bug: 145013815
Test: 5 new unit tests.

Change-Id: Id8e8d0254d7875cf933675ccdb7bf864dc6c5f72

3 files changed, 81 insertions, 38 deletions

diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
index 792248494f..a79fe98a27 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
@@ -47,22 +47,26 @@ std::optional<nsecs_t> VSyncDispatchTimerQueueEntry::wakeupTime() const {
     return {mArmedInfo->mActualWakeupTime};
 }
 
-nsecs_t VSyncDispatchTimerQueueEntry::schedule(nsecs_t workDuration, nsecs_t earliestVsync,
-                                               VSyncTracker& tracker, nsecs_t now) {
+ScheduleResult VSyncDispatchTimerQueueEntry::schedule(nsecs_t workDuration, nsecs_t earliestVsync,
+                                                      VSyncTracker& tracker, nsecs_t now) {
+    auto const nextVsyncTime =
+            tracker.nextAnticipatedVSyncTimeFrom(std::max(earliestVsync, now + workDuration));
+    if (mLastDispatchTime >= nextVsyncTime) { // already dispatched a callback for this vsync
+        return ScheduleResult::CannotSchedule;
+    }
+
+    auto const nextWakeupTime = nextVsyncTime - workDuration;
+    auto result = mArmedInfo ? ScheduleResult::ReScheduled : ScheduleResult::Scheduled;
     mWorkDuration = workDuration;
     mEarliestVsync = earliestVsync;
-    arm(tracker, now);
-    return mArmedInfo->mActualWakeupTime;
+    mArmedInfo = {nextWakeupTime, nextVsyncTime};
+    return result;
 }
 
 void VSyncDispatchTimerQueueEntry::update(VSyncTracker& tracker, nsecs_t now) {
     if (!mArmedInfo) {
         return;
     }
-    arm(tracker, now);
-}
-
-void VSyncDispatchTimerQueueEntry::arm(VSyncTracker& tracker, nsecs_t now) {
     auto const nextVsyncTime =
             tracker.nextAnticipatedVSyncTimeFrom(std::max(mEarliestVsync, now + mWorkDuration));
     mArmedInfo = {nextVsyncTime - mWorkDuration, nextVsyncTime};
@@ -214,16 +218,13 @@ ScheduleResult VSyncDispatchTimerQueue::schedule(CallbackToken token, nsecs_t wo
             return result;
         }
         auto& callback = it->second;
-        result = callback->wakeupTime() ? ScheduleResult::ReScheduled : ScheduleResult::Scheduled;
-
         auto const now = mTimeKeeper->now();
-        auto const wakeupTime = callback->schedule(workDuration, earliestVsync, mTracker, now);
-
-        if (wakeupTime < now - mTimerSlack || callback->lastExecutedVsyncTarget() > wakeupTime) {
-            return ScheduleResult::CannotSchedule;
+        result = callback->schedule(workDuration, earliestVsync, mTracker, now);
+        if (result == ScheduleResult::CannotSchedule) {
+            return result;
         }
 
-        if (wakeupTime < mIntendedWakeupTime - mTimerSlack) {
+        if (callback->wakeupTime() < mIntendedWakeupTime - mTimerSlack) {
             rearmTimerSkippingUpdateFor(now, it);
         }
     }
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
index f0580999c6..530e0a6899 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
@@ -44,8 +44,8 @@ public:
     std::optional<nsecs_t> lastExecutedVsyncTarget() const;
 
     // This moves the state from disarmed->armed and will calculate the wakeupTime.
-    nsecs_t schedule(nsecs_t workDuration, nsecs_t earliestVsync, VSyncTracker& tracker,
-                     nsecs_t now);
+    ScheduleResult schedule(nsecs_t workDuration, nsecs_t earliestVsync, VSyncTracker& tracker,
+                            nsecs_t now);
     // This will update armed entries with the latest vsync information. Entry remains armed.
     void update(VSyncTracker& tracker, nsecs_t now);
 
@@ -67,7 +67,6 @@ public:
     void ensureNotRunning();
 
 private:
-    void arm(VSyncTracker& tracker, nsecs_t now);
     std::string const mName;
     std::function<void(nsecs_t)> const mCallback;
 
diff --git a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
index 82950b5792..6efe0a27e6 100644
--- a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
@@ -551,6 +551,32 @@ TEST_F(VSyncDispatchTimerQueueTest, distinguishesScheduleAndReschedule) {
     EXPECT_EQ(mDispatch.schedule(cb0, 100, 1000), ScheduleResult::ReScheduled);
 }
 
+TEST_F(VSyncDispatchTimerQueueTest, canScheduleNegativeOffsetAgainstDifferentPeriods) {
+    CountingCallback cb0(mDispatch);
+    EXPECT_EQ(mDispatch.schedule(cb0, 500, 1000), ScheduleResult::Scheduled);
+    advanceToNextCallback();
+    EXPECT_EQ(mDispatch.schedule(cb0, 1100, 2000), ScheduleResult::Scheduled);
+}
+
+TEST_F(VSyncDispatchTimerQueueTest, canScheduleLargeNegativeOffset) {
+    Sequence seq;
+    EXPECT_CALL(mMockClock, alarmIn(_, 500)).InSequence(seq);
+    EXPECT_CALL(mMockClock, alarmIn(_, 600)).InSequence(seq);
+    CountingCallback cb0(mDispatch);
+    EXPECT_EQ(mDispatch.schedule(cb0, 500, 1000), ScheduleResult::Scheduled);
+    advanceToNextCallback();
+    EXPECT_EQ(mDispatch.schedule(cb0, 1900, 2000), ScheduleResult::Scheduled);
+}
+
+TEST_F(VSyncDispatchTimerQueueTest, cannotScheduleDoesNotAffectSchedulingState) {
+    EXPECT_CALL(mMockClock, alarmIn(_, 600));
+
+    CountingCallback cb(mDispatch);
+    EXPECT_EQ(mDispatch.schedule(cb, 400, 1000), ScheduleResult::Scheduled);
+    advanceToNextCallback();
+    EXPECT_EQ(mDispatch.schedule(cb, 100, 1000), ScheduleResult::CannotSchedule);
+}
+
 TEST_F(VSyncDispatchTimerQueueTest, helperMove) {
     EXPECT_CALL(mMockClock, alarmIn(_, 500)).Times(1);
     EXPECT_CALL(mMockClock, alarmCancel()).Times(1);
@@ -599,11 +625,10 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, stateScheduling) {
     VSyncDispatchTimerQueueEntry entry("test", [](auto) {});
 
     EXPECT_FALSE(entry.wakeupTime());
-    auto const wakeup = entry.schedule(100, 500, mStubTracker, 0);
-    auto const queried = entry.wakeupTime();
-    ASSERT_TRUE(queried);
-    EXPECT_THAT(*queried, Eq(wakeup));
-    EXPECT_THAT(*queried, Eq(900));
+    EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
+    auto const wakeup = entry.wakeupTime();
+    ASSERT_TRUE(wakeup);
+    EXPECT_THAT(*wakeup, Eq(900));
 
     entry.disarm();
     EXPECT_FALSE(entry.wakeupTime());
@@ -619,11 +644,10 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, stateSchedulingReallyLongWakeupLatency)
     VSyncDispatchTimerQueueEntry entry("test", [](auto) {});
 
     EXPECT_FALSE(entry.wakeupTime());
-    auto const wakeup = entry.schedule(500, 994, mStubTracker, now);
-    auto const queried = entry.wakeupTime();
-    ASSERT_TRUE(queried);
-    EXPECT_THAT(*queried, Eq(wakeup));
-    EXPECT_THAT(*queried, Eq(9500));
+    EXPECT_THAT(entry.schedule(500, 994, mStubTracker, now), Eq(ScheduleResult::Scheduled));
+    auto const wakeup = entry.wakeupTime();
+    ASSERT_TRUE(wakeup);
+    EXPECT_THAT(*wakeup, Eq(9500));
 }
 
 TEST_F(VSyncDispatchTimerQueueEntryTest, runCallback) {
@@ -634,8 +658,10 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, runCallback) {
         calledTime = time;
     });
 
-    auto const wakeup = entry.schedule(100, 500, mStubTracker, 0);
-    EXPECT_THAT(wakeup, Eq(900));
+    EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
+    auto const wakeup = entry.wakeupTime();
+    ASSERT_TRUE(wakeup);
+    EXPECT_THAT(*wakeup, Eq(900));
 
     entry.callback(entry.executing());
 
@@ -659,23 +685,40 @@ TEST_F(VSyncDispatchTimerQueueEntryTest, updateCallback) {
     entry.update(mStubTracker, 0);
     EXPECT_FALSE(entry.wakeupTime());
 
-    auto const wakeup = entry.schedule(100, 500, mStubTracker, 0);
+    EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
+    auto wakeup = entry.wakeupTime();
+    ASSERT_TRUE(wakeup);
     EXPECT_THAT(wakeup, Eq(900));
 
     entry.update(mStubTracker, 0);
-    auto const queried = entry.wakeupTime();
-    ASSERT_TRUE(queried);
-    EXPECT_THAT(*queried, Eq(920));
+    wakeup = entry.wakeupTime();
+    ASSERT_TRUE(wakeup);
+    EXPECT_THAT(*wakeup, Eq(920));
 }
 
 TEST_F(VSyncDispatchTimerQueueEntryTest, skipsUpdateIfJustScheduled) {
     VSyncDispatchTimerQueueEntry entry("test", [](auto) {});
-    auto const wakeup = entry.schedule(100, 500, mStubTracker, 0);
+    EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
     entry.update(mStubTracker, 0);
 
-    auto const queried = entry.wakeupTime();
-    ASSERT_TRUE(queried);
-    EXPECT_THAT(*queried, Eq(wakeup));
+    auto const wakeup = entry.wakeupTime();
+    ASSERT_TRUE(wakeup);
+    EXPECT_THAT(*wakeup, Eq(wakeup));
+}
+
+TEST_F(VSyncDispatchTimerQueueEntryTest, reportsCannotScheduleIfMissedOpportunity) {
+    VSyncDispatchTimerQueueEntry entry("test", [](auto) {});
+    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));
+    EXPECT_THAT(entry.schedule(200, 1001, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
+}
+
+TEST_F(VSyncDispatchTimerQueueEntryTest, reportsReScheduleIfStillTime) {
+    VSyncDispatchTimerQueueEntry entry("test", [](auto) {});
+    EXPECT_THAT(entry.schedule(100, 500, mStubTracker, 0), Eq(ScheduleResult::Scheduled));
+    EXPECT_THAT(entry.schedule(200, 500, mStubTracker, 0), Eq(ScheduleResult::ReScheduled));
 }
 
 } // namespace android::scheduler