commit 542c9dc008516bdce10d748e4ccda732ec7b511c
author Dominik Laskowski <domlaskowski@google.com> Fri Apr 10 12:42:02 2020 -0700
committer Dominik Laskowski <domlaskowski@google.com> Mon Apr 13 22:15:15 2020 -0700
tree a4ebaa97e86fdd670ca6aad9501cf349f30b6cbf
parent 470df5f0d6a272fd0c92f5967e43af0f29a5c866

SF: Remove display lookup in SurfaceTracing

Remove the strong reference to prevent a removed display from being
destroyed off the main thread. This is also a step in adding thread
annotations to display state accessors.

Also, clean up SurfaceTracing usage in the SurfaceFlinger class.

Bug: 123715322
Test: Tracing still works.
Change-Id: Iad174e7b42fbde96a46155c740838f6cd3966365

services/surfaceflinger/SurfaceFlinger.cpp

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index cd78c1a..b4ae8ed 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -169,19 +169,22 @@
 
 #pragma clang diagnostic pop
 
-class ConditionalLock {
-public:
-    ConditionalLock(Mutex& mutex, bool lock) : mMutex(mutex), mLocked(lock) {
-        if (lock) {
-            mMutex.lock();
-        }
+template <typename Mutex>
+struct ConditionalLockGuard {
+    ConditionalLockGuard(Mutex& mutex, bool lock) : mutex(mutex), lock(lock) {
+        if (lock) mutex.lock();
     }
-    ~ConditionalLock() { if (mLocked) mMutex.unlock(); }
-private:
-    Mutex& mMutex;
-    bool mLocked;
+
+    ~ConditionalLockGuard() {
+        if (lock) mutex.unlock();
+    }
+
+    Mutex& mutex;
+    const bool lock;
 };
 
+using ConditionalLock = ConditionalLockGuard<Mutex>;
+
 // TODO(b/141333600): Consolidate with HWC2::Display::Config::Builder::getDefaultDensity.
 constexpr float FALLBACK_DENSITY = ACONFIGURATION_DENSITY_TV / 160.f;
 
@@ -1948,8 +1951,15 @@
             // potentially trigger a display handoff.
             updateVrFlinger();
 
+            if (mTracingEnabledChanged) {
+                mTracingEnabled = mTracing.isEnabled();
+                mTracingEnabledChanged = false;
+            }
+
             bool refreshNeeded;
-            withTracingLock([&]() {
+            {
+                ConditionalLockGuard<std::mutex> lock(mTracingLock, mTracingEnabled);
+
                 refreshNeeded = handleMessageTransaction();
                 refreshNeeded |= handleMessageInvalidate();
                 if (mTracingEnabled) {
@@ -1959,7 +1969,7 @@
                         mTracing.notifyLocked("visibleRegionsDirty");
                     }
                 }
-            });
+            }
 
             // Layers need to get updated (in the previous line) before we can use them for
             // choosing the refresh rate.
@@ -3034,26 +3044,6 @@
     mDrawingState.traverse([&](Layer* layer) { layer->updateMirrorInfo(); });
 }
 
-void SurfaceFlinger::withTracingLock(std::function<void()> lockedOperation) {
-    if (mTracingEnabledChanged) {
-        mTracingEnabled = mTracing.isEnabled();
-        mTracingEnabledChanged = false;
-    }
-
-    // Synchronize with Tracing thread
-    std::unique_lock<std::mutex> lock;
-    if (mTracingEnabled) {
-        lock = std::unique_lock<std::mutex>(mDrawingStateLock);
-    }
-
-    lockedOperation();
-
-    // Synchronize with Tracing thread
-    if (mTracingEnabled) {
-        lock.unlock();
-    }
-}
-
 void SurfaceFlinger::commitOffscreenLayers() {
     for (Layer* offscreenLayer : mOffscreenLayers) {
         offscreenLayer->traverse(LayerVector::StateSet::Drawing, [](Layer* layer) {
@@ -4560,11 +4550,13 @@
     result.append("\n");
 }
 
-LayersProto SurfaceFlinger::dumpDrawingStateProto(
-        uint32_t traceFlags, const sp<const DisplayDevice>& displayDevice) const {
+LayersProto SurfaceFlinger::dumpDrawingStateProto(uint32_t traceFlags) const {
+    // If context is SurfaceTracing thread, mTracingLock blocks display transactions on main thread.
+    const auto display = getDefaultDisplayDeviceLocked();
+
     LayersProto layersProto;
     for (const sp<Layer>& layer : mDrawingState.layersSortedByZ) {
-        layer->writeToProto(layersProto, traceFlags, displayDevice);
+        layer->writeToProto(layersProto, traceFlags, display);
     }
 
     return layersProto;
@@ -4596,10 +4588,7 @@
 
 LayersProto SurfaceFlinger::dumpProtoFromMainThread(uint32_t traceFlags) {
     LayersProto layersProto;
-    postMessageSync(new LambdaMessage([&]() {
-        const auto& displayDevice = getDefaultDisplayDeviceLocked();
-        layersProto = dumpDrawingStateProto(traceFlags, displayDevice);
-    }));
+    postMessageSync(new LambdaMessage([&] { layersProto = dumpDrawingStateProto(traceFlags); }));
     return layersProto;
 }
 
@@ -5126,20 +5115,12 @@
                 n = data.readInt32();
                 if (n) {
                     ALOGD("LayerTracing enabled");
-                    Mutex::Autolock lock(mStateLock);
-                    mTracingEnabledChanged = true;
-                    mTracing.enable();
+                    mTracingEnabledChanged = mTracing.enable();
                     reply->writeInt32(NO_ERROR);
                 } else {
                     ALOGD("LayerTracing disabled");
-                    bool writeFile = false;
-                    {
-                        Mutex::Autolock lock(mStateLock);
-                        mTracingEnabledChanged = true;
-                        writeFile = mTracing.disable();
-                    }
-
-                    if (writeFile) {
+                    mTracingEnabledChanged = mTracing.disable();
+                    if (mTracingEnabledChanged) {
                         reply->writeInt32(mTracing.writeToFile());
                     } else {
                         reply->writeInt32(NO_ERROR);