SF: VSyncReactor add event subscription functions
Adds 3 more functions from the DispSync interface to VSyncReactor,
{add,remove}EventListener and changePhaseOffset.
Bug: 140303479
Test: about 10 new units
Change-Id: I2135f87a21cd0e43613367d31e006bb574380bc6
diff --git a/services/surfaceflinger/Scheduler/VSyncReactor.cpp b/services/surfaceflinger/Scheduler/VSyncReactor.cpp
index f2a7791..49ab6c1 100644
--- a/services/surfaceflinger/Scheduler/VSyncReactor.cpp
+++ b/services/surfaceflinger/Scheduler/VSyncReactor.cpp
@@ -14,7 +14,9 @@
* limitations under the License.
*/
+//#define LOG_NDEBUG 0
#include "VSyncReactor.h"
+#include <log/log.h>
#include "TimeKeeper.h"
#include "VSyncDispatch.h"
#include "VSyncTracker.h"
@@ -30,6 +32,84 @@
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;
+
+ // TODO: (b/145213786) check the return code here sensibly
+ mRegistration.schedule(calculateWorkload(), mLastCallTime);
+ }
+
+ 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);
+ mRegistration.schedule(calculateWorkload(), vsynctime);
+ }
+ }
+
+ // 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 +172,9 @@
{
std::lock_guard<std::mutex> lk(mMutex);
mPeriodChangeInProgress = true;
+ for (auto& entry : mCallbacks) {
+ entry.second->setPeriod(period);
+ }
}
}
@@ -114,4 +197,47 @@
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 786ee98..837eb75 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 @@
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 @@
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/VSyncReactorTest.cpp b/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp
index 84df019..652a7b8 100644
--- a/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp
+++ b/services/surfaceflinger/tests/unittests/VSyncReactorTest.cpp
@@ -122,21 +122,53 @@
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, 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 @@
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, 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, 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,151 @@
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, 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);
+}
+
} // namespace android::scheduler