SF: Fix thread safety for scheduler callbacks
SurfaceFlinger::setRefreshRateTo, called from the Scheduler callbacks,
reads HWC and SF state without locking mStateLock, concurrently with
writes from the main thread. This CL registers ResetIdleTimerCallback
per EventThreadConnection, and locks mStateLock for connections used
off the main thread. Note that ExpiredTimerCallback locks mStateLock
unconditionally, since it is always called from the IdleTimer thread.
This CL also adds a thread annotation to setRefreshRateTo, and refactors
it accordingly.
Bug: 123715322
Test: libsurfaceflinger_unittest
Test: Boot with scheduler enabled
Change-Id: Id62c48ae22da38f292ffc18e8731a1c49a0a083c
diff --git a/services/surfaceflinger/tests/unittests/EventThreadTest.cpp b/services/surfaceflinger/tests/unittests/EventThreadTest.cpp
index ad7dcb4..2be4fd0 100644
--- a/services/surfaceflinger/tests/unittests/EventThreadTest.cpp
+++ b/services/surfaceflinger/tests/unittests/EventThreadTest.cpp
@@ -53,8 +53,10 @@
class MockEventThreadConnection : public EventThreadConnection {
public:
MockEventThreadConnection(android::impl::EventThread* eventThread,
- ResyncCallback&& resyncCallback)
- : EventThreadConnection(eventThread, std::move(resyncCallback)) {}
+ ResyncCallback&& resyncCallback,
+ ResetIdleTimerCallback&& resetIdleTimerCallback)
+ : EventThreadConnection(eventThread, std::move(resyncCallback),
+ std::move(resetIdleTimerCallback)) {}
MOCK_METHOD1(postEvent, status_t(const DisplayEventReceiver::Event& event));
};
@@ -82,6 +84,7 @@
AsyncCallRecorder<void (*)(VSyncSource::Callback*)> mVSyncSetCallbackCallRecorder;
AsyncCallRecorder<void (*)(nsecs_t)> mVSyncSetPhaseOffsetCallRecorder;
AsyncCallRecorder<void (*)()> mResyncCallRecorder;
+ AsyncCallRecorder<void (*)()> mResetIdleTimerCallRecorder;
AsyncCallRecorder<void (*)(nsecs_t)> mInterceptVSyncCallRecorder;
ConnectionEventRecorder mConnectionEventCallRecorder{0};
@@ -136,7 +139,8 @@
sp<EventThreadTest::MockEventThreadConnection> EventThreadTest::createConnection(
ConnectionEventRecorder& recorder) {
sp<MockEventThreadConnection> connection =
- new MockEventThreadConnection(mThread.get(), mResyncCallRecorder.getInvocable());
+ new MockEventThreadConnection(mThread.get(), mResyncCallRecorder.getInvocable(),
+ mResetIdleTimerCallRecorder.getInvocable());
EXPECT_CALL(*connection, postEvent(_)).WillRepeatedly(Invoke(recorder.getInvocable()));
return connection;
}
@@ -207,6 +211,7 @@
EXPECT_FALSE(mVSyncSetCallbackCallRecorder.waitForCall(0us).has_value());
EXPECT_FALSE(mVSyncSetPhaseOffsetCallRecorder.waitForCall(0us).has_value());
EXPECT_FALSE(mResyncCallRecorder.waitForCall(0us).has_value());
+ EXPECT_FALSE(mResetIdleTimerCallRecorder.waitForCall(0us).has_value());
EXPECT_FALSE(mInterceptVSyncCallRecorder.waitForCall(0us).has_value());
EXPECT_FALSE(mConnectionEventCallRecorder.waitForCall(0us).has_value());
}
@@ -224,9 +229,10 @@
TEST_F(EventThreadTest, requestNextVsyncPostsASingleVSyncEventToTheConnection) {
// Signal that we want the next vsync event to be posted to the connection
- mThread->requestNextVsync(mConnection, false);
+ mThread->requestNextVsync(mConnection, true);
- // EventThread should immediately request a resync.
+ // EventThread should immediately reset the idle timer and request a resync.
+ EXPECT_TRUE(mResetIdleTimerCallRecorder.waitForCall().has_value());
EXPECT_TRUE(mResyncCallRecorder.waitForCall().has_value());
// EventThread should enable vsync callbacks.
diff --git a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
index 0db96d9..26b6d0c 100644
--- a/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
+++ b/services/surfaceflinger/tests/unittests/SchedulerTest.cpp
@@ -25,7 +25,7 @@
class MockEventThreadConnection : public android::EventThreadConnection {
public:
explicit MockEventThreadConnection(EventThread* eventThread)
- : EventThreadConnection(eventThread, ResyncCallback()) {}
+ : EventThreadConnection(eventThread, ResyncCallback(), ResetIdleTimerCallback()) {}
~MockEventThreadConnection() = default;
MOCK_METHOD1(stealReceiveChannel, status_t(gui::BitTube* outChannel));
@@ -78,10 +78,11 @@
// createConnection call to scheduler makes a createEventConnection call to EventThread. Make
// sure that call gets executed and returns an EventThread::Connection object.
- EXPECT_CALL(*mEventThread, createEventConnection(_))
+ EXPECT_CALL(*mEventThread, createEventConnection(_, _))
.WillRepeatedly(Return(mEventThreadConnection));
mConnectionHandle = mScheduler->createConnection("appConnection", 16, ResyncCallback(),
+ ResetIdleTimerCallback(),
impl::EventThread::InterceptVSyncsCallback());
EXPECT_TRUE(mConnectionHandle != nullptr);
}
@@ -102,7 +103,8 @@
// exceptions, just gracefully continues.
sp<IDisplayEventConnection> returnedValue;
ASSERT_NO_FATAL_FAILURE(
- returnedValue = mScheduler->createDisplayEventConnection(nullptr, ResyncCallback()));
+ returnedValue = mScheduler->createDisplayEventConnection(nullptr, ResyncCallback(),
+ ResetIdleTimerCallback()));
EXPECT_TRUE(returnedValue == nullptr);
EXPECT_TRUE(mScheduler->getEventThread(nullptr) == nullptr);
EXPECT_TRUE(mScheduler->getEventConnection(nullptr) == nullptr);
@@ -123,7 +125,8 @@
sp<IDisplayEventConnection> returnedValue;
ASSERT_NO_FATAL_FAILURE(
returnedValue =
- mScheduler->createDisplayEventConnection(connectionHandle, ResyncCallback()));
+ mScheduler->createDisplayEventConnection(connectionHandle, ResyncCallback(),
+ ResetIdleTimerCallback()));
EXPECT_TRUE(returnedValue == nullptr);
EXPECT_TRUE(mScheduler->getEventThread(connectionHandle) == nullptr);
EXPECT_TRUE(mScheduler->getEventConnection(connectionHandle) == nullptr);
@@ -152,7 +155,8 @@
sp<IDisplayEventConnection> returnedValue;
ASSERT_NO_FATAL_FAILURE(
returnedValue =
- mScheduler->createDisplayEventConnection(mConnectionHandle, ResyncCallback()));
+ mScheduler->createDisplayEventConnection(mConnectionHandle, ResyncCallback(),
+ ResetIdleTimerCallback()));
EXPECT_TRUE(returnedValue != nullptr);
ASSERT_EQ(returnedValue, mEventThreadConnection);
diff --git a/services/surfaceflinger/tests/unittests/mock/MockEventThread.h b/services/surfaceflinger/tests/unittests/mock/MockEventThread.h
index aaf67e9..589237d 100644
--- a/services/surfaceflinger/tests/unittests/mock/MockEventThread.h
+++ b/services/surfaceflinger/tests/unittests/mock/MockEventThread.h
@@ -28,7 +28,8 @@
EventThread();
~EventThread() override;
- MOCK_CONST_METHOD1(createEventConnection, sp<EventThreadConnection>(ResyncCallback));
+ MOCK_CONST_METHOD2(createEventConnection,
+ sp<EventThreadConnection>(ResyncCallback, ResetIdleTimerCallback));
MOCK_METHOD0(onScreenReleased, void());
MOCK_METHOD0(onScreenAcquired, void());
MOCK_METHOD2(onHotplugReceived, void(PhysicalDisplayId, bool));