commit 6839927f9fac02a048c73e63a4a988cd7be118b9
author Dominik Laskowski <domlaskowski@google.com> Thu Sep 15 07:54:46 2022 -0700
committer Dominik Laskowski <domlaskowski@google.com> Fri Sep 16 13:32:38 2022 -0700
tree fcdf074aa119afc37d9fecee01c326dbbcb810f0
parent 3a214d74c55cc6e5b0fcc5a8eb4df49a1b3f135e

SF: Fix a sporadic crash during multi-display boot

The Scheduler is created when the first commit adds the primary display,
and accessed whenever a commit adds a physical display. Transactions are
ordered by display token, which is an arbitrary address.

In a multi-display boot, the addition of the primary display could be
reordered after other displays, causing a crash when the Scheduler is
accessed before creation.

As a short-term workaround until Scheduler initialization is decoupled
from commit of display transactions, commit the primary display first.

Bug: 244442572
Test: Boot
Change-Id: I62c4b5299de329a02e60d5f2529ed49907d74415

services/surfaceflinger/SurfaceFlinger.cpp

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 78eaa14..13331ec 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -815,16 +815,37 @@
         enableHalVirtualDisplays(true);
     }
 
-    // Process any initial hotplug and resulting display changes.
+    // Process hotplug for displays connected at boot.
     LOG_ALWAYS_FATAL_IF(!configureLocked(),
                         "Initial display configuration failed: HWC did not hotplug");
-    processDisplayChangesLocked();
 
-    const auto display = getDefaultDisplayDeviceLocked();
+    // Commit primary display.
+    sp<const DisplayDevice> display;
+    if (const auto indexOpt = mCurrentState.getDisplayIndex(getPrimaryDisplayIdLocked())) {
+        const auto& displays = mCurrentState.displays;
+
+        const auto& token = displays.keyAt(*indexOpt);
+        const auto& state = displays.valueAt(*indexOpt);
+
+        processDisplayAdded(token, state);
+        mDrawingState.displays.add(token, state);
+
+        display = getDefaultDisplayDeviceLocked();
+    }
+
     LOG_ALWAYS_FATAL_IF(!display, "Failed to configure the primary display");
     LOG_ALWAYS_FATAL_IF(!getHwComposer().isConnected(display->getPhysicalId()),
                         "Primary display is disconnected");
 
+    // TODO(b/241285876): The Scheduler needlessly depends on creating the CompositionEngine part of
+    // the DisplayDevice, hence the above commit of the primary display. Remove that special case by
+    // initializing the Scheduler after configureLocked, once decoupled from DisplayDevice.
+    initScheduler(display);
+    dispatchDisplayHotplugEvent(display->getPhysicalId(), true);
+
+    // Commit secondary display(s).
+    processDisplayChangesLocked();
+
     // initialize our drawing state
     mDrawingState = mCurrentState;
 
@@ -2952,10 +2973,13 @@
     LOG_FATAL_IF(!displaySurface);
     auto display = setupNewDisplayDeviceInternal(displayToken, std::move(compositionDisplay), state,
                                                  displaySurface, producer);
-    if (display->isPrimary()) {
-        initScheduler(display);
-    }
-    if (!state.isVirtual()) {
+
+    if (mScheduler && !display->isVirtual()) {
+        // Display modes are reloaded on hotplug reconnect.
+        if (display->isPrimary()) {
+            mScheduler->setRefreshRateConfigs(display->holdRefreshRateConfigs());
+        }
+
         dispatchDisplayHotplugEvent(display->getPhysicalId(), true);
     }
 
@@ -3367,15 +3391,9 @@
     mScheduler->onFrameRateOverridesChanged(mAppConnectionHandle, displayId);
 }
 
-void SurfaceFlinger::initScheduler(const sp<DisplayDevice>& display) {
-    if (mScheduler) {
-        // If the scheduler is already initialized, this means that we received
-        // a hotplug(connected) on the primary display. In that case we should
-        // update the scheduler with the most recent display information.
-        ALOGW("Scheduler already initialized, updating instead");
-        mScheduler->setRefreshRateConfigs(display->holdRefreshRateConfigs());
-        return;
-    }
+void SurfaceFlinger::initScheduler(const sp<const DisplayDevice>& display) {
+    LOG_ALWAYS_FATAL_IF(mScheduler);
+
     const auto currRefreshRate = display->getActiveMode()->getFps();
     mRefreshRateStats = std::make_unique<scheduler::RefreshRateStats>(*mTimeStats, currRefreshRate,
                                                                       hal::PowerMode::OFF);
@@ -7076,7 +7094,7 @@
     mRegionSamplingThread->onCompositionComplete(mScheduler->getScheduledFrameTime());
 }
 
-void SurfaceFlinger::onActiveDisplaySizeChanged(const sp<DisplayDevice>& activeDisplay) {
+void SurfaceFlinger::onActiveDisplaySizeChanged(const sp<const DisplayDevice>& activeDisplay) {
     mScheduler->onActiveDisplayAreaChanged(activeDisplay->getWidth() * activeDisplay->getHeight());
     getRenderEngine().onActiveDisplaySizeChanged(activeDisplay->getSize());
 }