commit a515fd50aea0610749260045263ca6f6a03d9b86
author Dominik Laskowski <domlaskowski@google.com> Mon Jul 15 08:24:49 2024 -0400
committer Android Build Coastguard Worker <android-build-coastguard-worker@google.com> Tue Jul 23 00:14:28 2024 +0000
tree 6f9b8ecae2f4d8a070e75366398bf3ccb9c9a1c4
parent f848348555c21dc681b8bc6e5c4091f294497af6

SF: Restore mStateLock mutex for modesetting

In order to fix a deadlock during display hotplug, the modesetting mutex
was changed from mStateLock to a granular DisplayModeController lock, in
the following CLs:

    I30ec756f134d2d67a70ac8797008dc792eac035e
    Iaae02d95e175e55f52f65da1481b64f1f533a04c (reverted)

This exposed a data race leading to a crash, which, unlike the deadlock,
affects all devices with refresh rate switching. The race will be fixed
by:

    If6141d2928643e82b3251b321e18c300cd8c201c (WIP)

As a stopgap until then, restore mStateLock (on top of the DMC lock) on
the main thread to fix the race, but reintroduce the deadlock. The lock
was already restored on the idle timer thread by the reverted CL above.

Fixes: 348827779
Flag: com.android.graphics.surfaceflinger.flags.connected_display
Test: Manual (foldable, connected display)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:b2fc4235635f5160ed5ae35c1fbd1fcc3e45c59f)
Merged-In: Ie15de10d16eefeb65289574b120d2ef4cc09d6c0
Change-Id: Ie15de10d16eefeb65289574b120d2ef4cc09d6c0

services/surfaceflinger/SurfaceFlinger.cpp

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 4bdff29..fcead9f 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1414,6 +1414,8 @@
     return future.get();
 }
 
+// TODO: b/241285876 - Restore thread safety analysis once mStateLock below is unconditional.
+[[clang::no_thread_safety_analysis]]
 void SurfaceFlinger::finalizeDisplayModeChange(PhysicalDisplayId displayId) {
     ATRACE_NAME(ftl::Concat(__func__, ' ', displayId.value).c_str());
 
@@ -1429,7 +1431,7 @@
     if (const auto oldResolution =
                 mDisplayModeController.getActiveMode(displayId).modePtr->getResolution();
         oldResolution != activeMode.modePtr->getResolution()) {
-        Mutex::Autolock lock(mStateLock);
+        ConditionalLock lock(mStateLock, !FlagManager::getInstance().connected_display());
 
         auto& state = mCurrentState.displays.editValueFor(getPhysicalDisplayTokenLocked(displayId));
         // We need to generate new sequenceId in order to recreate the display (and this
@@ -1483,7 +1485,7 @@
 
     std::optional<PhysicalDisplayId> displayToUpdateImmediately;
 
-    for (const auto& [displayId, physical] : FTL_FAKE_GUARD(mStateLock, mPhysicalDisplays)) {
+    for (const auto& [displayId, physical] : mPhysicalDisplays) {
         auto desiredModeOpt = mDisplayModeController.getDesiredMode(displayId);
         if (!desiredModeOpt) {
             continue;
@@ -2618,9 +2620,13 @@
         return false;
     }
 
-    for (const auto [displayId, _] : frameTargets) {
-        if (mDisplayModeController.isModeSetPending(displayId)) {
-            finalizeDisplayModeChange(displayId);
+    {
+        ConditionalLock lock(mStateLock, FlagManager::getInstance().connected_display());
+
+        for (const auto [displayId, _] : frameTargets) {
+            if (mDisplayModeController.isModeSetPending(displayId)) {
+                finalizeDisplayModeChange(displayId);
+            }
         }
     }
 
@@ -2719,9 +2725,16 @@
                                                         ? &mLayerHierarchyBuilder.getHierarchy()
                                                         : nullptr,
                                                 updateAttachedChoreographer);
+
+        if (FlagManager::getInstance().connected_display()) {
+            initiateDisplayModeChanges();
+        }
     }
 
-    initiateDisplayModeChanges();
+    if (!FlagManager::getInstance().connected_display()) {
+        ftl::FakeGuard guard(mStateLock);
+        initiateDisplayModeChanges();
+    }
 
     updateCursorAsync();
     if (!mustComposite) {