commit ccf37d7447b6ced5f08955e8d75b9c992052bf98
author Dominik Laskowski <domlaskowski@google.com> Fri Feb 01 16:47:58 2019 -0800
committer Dominik Laskowski <domlaskowski@google.com> Wed Feb 06 15:56:41 2019 -0800
tree f591310d70f18d6b019db59d8ba81c3553cd9006
parent 846c8337f0d48ce794cc745c92449ce97fcb1bc7

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

services/surfaceflinger/Scheduler/EventThread.cpp

diff --git a/services/surfaceflinger/Scheduler/EventThread.cpp b/services/surfaceflinger/Scheduler/EventThread.cpp
index 91ae087..5d9cfde 100644
--- a/services/surfaceflinger/Scheduler/EventThread.cpp
+++ b/services/surfaceflinger/Scheduler/EventThread.cpp
@@ -100,8 +100,10 @@
 } // namespace
 
 EventThreadConnection::EventThreadConnection(EventThread* eventThread,
-                                             ResyncCallback resyncCallback)
+                                             ResyncCallback resyncCallback,
+                                             ResetIdleTimerCallback resetIdleTimerCallback)
       : resyncCallback(std::move(resyncCallback)),
+        resetIdleTimerCallback(std::move(resetIdleTimerCallback)),
         mEventThread(eventThread),
         mChannel(gui::BitTube::DefaultSize) {}
 
@@ -147,22 +149,18 @@
 namespace impl {
 
 EventThread::EventThread(std::unique_ptr<VSyncSource> src,
-                         const InterceptVSyncsCallback& interceptVSyncsCallback,
-                         const ResetIdleTimerCallback& resetIdleTimerCallback,
-                         const char* threadName)
-      : EventThread(nullptr, std::move(src), interceptVSyncsCallback, threadName) {
-    mResetIdleTimer = resetIdleTimerCallback;
-}
+                         InterceptVSyncsCallback interceptVSyncsCallback, const char* threadName)
+      : EventThread(nullptr, std::move(src), std::move(interceptVSyncsCallback), threadName) {}
 
 EventThread::EventThread(VSyncSource* src, InterceptVSyncsCallback interceptVSyncsCallback,
                          const char* threadName)
-      : EventThread(src, nullptr, interceptVSyncsCallback, threadName) {}
+      : EventThread(src, nullptr, std::move(interceptVSyncsCallback), threadName) {}
 
 EventThread::EventThread(VSyncSource* src, std::unique_ptr<VSyncSource> uniqueSrc,
                          InterceptVSyncsCallback interceptVSyncsCallback, const char* threadName)
       : mVSyncSource(src),
         mVSyncSourceUnique(std::move(uniqueSrc)),
-        mInterceptVSyncsCallback(interceptVSyncsCallback),
+        mInterceptVSyncsCallback(std::move(interceptVSyncsCallback)),
         mThreadName(threadName) {
     if (src == nullptr) {
         mVSyncSource = mVSyncSourceUnique.get();
@@ -205,8 +203,10 @@
     mVSyncSource->setPhaseOffset(phaseOffset);
 }
 
-sp<EventThreadConnection> EventThread::createEventConnection(ResyncCallback resyncCallback) const {
-    return new EventThreadConnection(const_cast<EventThread*>(this), std::move(resyncCallback));
+sp<EventThreadConnection> EventThread::createEventConnection(
+        ResyncCallback resyncCallback, ResetIdleTimerCallback resetIdleTimerCallback) const {
+    return new EventThreadConnection(const_cast<EventThread*>(this), std::move(resyncCallback),
+                                     std::move(resetIdleTimerCallback));
 }
 
 status_t EventThread::registerDisplayEventConnection(const sp<EventThreadConnection>& connection) {
@@ -249,9 +249,9 @@
 }
 
 void EventThread::requestNextVsync(const sp<EventThreadConnection>& connection, bool reset) {
-    if (mResetIdleTimer && reset) {
+    if (connection->resetIdleTimerCallback && reset) {
         ATRACE_NAME("resetIdleTimer");
-        mResetIdleTimer();
+        connection->resetIdleTimerCallback();
     }
 
     if (connection->resyncCallback) {