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
diff --git a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
index 7922484..a79fe98 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.cpp
@@ -47,22 +47,26 @@
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 @@
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 f058099..530e0a6 100644
--- a/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
+++ b/services/surfaceflinger/Scheduler/VSyncDispatchTimerQueue.h
@@ -44,8 +44,8 @@
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 @@
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 82950b5..6efe0a2 100644
--- a/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncDispatchTimerQueueTest.cpp
@@ -551,6 +551,32 @@
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 @@
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 @@
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 @@
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 @@
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