From 3b468eca28c385ea61832b96dc5387bcaa32b25e Mon Sep 17 00:00:00 2001
From: Vishnu Nair <vishnun@google.com>
Date: Wed, 27 Mar 2024 22:20:24 -0700
Subject: Fix sync issue with handling display state changes

We may miss some state changes if a display state change comes between
processDisplayChangesLocked and commitTransactions. Fix this by grabbing
the state lock for the duration of display updates in commit.

Test: steps in bug

Bug: 330105711, 330103914, 328539539
Merged-In: I4798961551f78d75c45ead6dea5ebca895e5ef7d
Change-Id: I4798961551f78d75c45ead6dea5ebca895e5ef7d
(cherry picked from commit 878911f7df21c700ddbe9e9c9d28cd0a1776946f)
---
 services/surfaceflinger/SurfaceFlinger.cpp | 31 ++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

(limited to 'services/surfaceflinger/SurfaceFlinger.cpp')

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index cf5f55d7bd..c1f362ccd1 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2256,7 +2256,7 @@ bool SurfaceFlinger::updateLayerSnapshotsLegacy(VsyncId vsyncId, nsecs_t frameTi
     outTransactionsAreEmpty = !needsTraversal;
     const bool shouldCommit = (getTransactionFlags() & ~eTransactionFlushNeeded) || needsTraversal;
     if (shouldCommit) {
-        commitTransactions();
+        commitTransactionsLegacy();
     }
 
     bool mustComposite = latchBuffers() || shouldCommit;
@@ -2380,8 +2380,14 @@ bool SurfaceFlinger::updateLayerSnapshots(VsyncId vsyncId, nsecs_t frameTimeNs,
         mLayerHierarchyBuilder.update(mLayerLifecycleManager);
     }
 
+    // Keep a copy of the drawing state (that is going to be overwritten
+    // by commitTransactionsLocked) outside of mStateLock so that the side
+    // effects of the State assignment don't happen with mStateLock held,
+    // which can cause deadlocks.
+    State drawingState(mDrawingState);
+    Mutex::Autolock lock(mStateLock);
     bool mustComposite = false;
-    mustComposite |= applyAndCommitDisplayTransactionStates(update.transactions);
+    mustComposite |= applyAndCommitDisplayTransactionStatesLocked(update.transactions);
 
     {
         ATRACE_NAME("LayerSnapshotBuilder:update");
@@ -2420,7 +2426,7 @@ bool SurfaceFlinger::updateLayerSnapshots(VsyncId vsyncId, nsecs_t frameTimeNs,
     bool newDataLatched = false;
     if (!mLegacyFrontEndEnabled) {
         ATRACE_NAME("DisplayCallbackAndStatsUpdates");
-        mustComposite |= applyTransactions(update.transactions, vsyncId);
+        mustComposite |= applyTransactionsLocked(update.transactions, vsyncId);
         traverseLegacyLayers([&](Layer* layer) { layer->commitTransaction(); });
         const nsecs_t latchTime = systemTime();
         bool unused = false;
@@ -3257,6 +3263,19 @@ void SurfaceFlinger::computeLayerBounds() {
 
 void SurfaceFlinger::commitTransactions() {
     ATRACE_CALL();
+    mDebugInTransaction = systemTime();
+
+    // Here we're guaranteed that some transaction flags are set
+    // so we can call commitTransactionsLocked unconditionally.
+    // We clear the flags with mStateLock held to guarantee that
+    // mCurrentState won't change until the transaction is committed.
+    mScheduler->modulateVsync({}, &VsyncModulator::onTransactionCommit);
+    commitTransactionsLocked(clearTransactionFlags(eTransactionMask));
+    mDebugInTransaction = 0;
+}
+
+void SurfaceFlinger::commitTransactionsLegacy() {
+    ATRACE_CALL();
 
     // Keep a copy of the drawing state (that is going to be overwritten
     // by commitTransactionsLocked) outside of mStateLock so that the side
@@ -5219,9 +5238,8 @@ bool SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin
     return needsTraversal;
 }
 
-bool SurfaceFlinger::applyAndCommitDisplayTransactionStates(
+bool SurfaceFlinger::applyAndCommitDisplayTransactionStatesLocked(
         std::vector<TransactionState>& transactions) {
-    Mutex::Autolock lock(mStateLock);
     bool needsTraversal = false;
     uint32_t transactionFlags = 0;
     for (auto& transaction : transactions) {
@@ -6009,7 +6027,8 @@ void SurfaceFlinger::initializeDisplays() {
     if (mLegacyFrontEndEnabled) {
         applyTransactions(transactions, VsyncId{0});
     } else {
-        applyAndCommitDisplayTransactionStates(transactions);
+        Mutex::Autolock lock(mStateLock);
+        applyAndCommitDisplayTransactionStatesLocked(transactions);
     }
 
     {
-- 
cgit v1.2.3-59-g8ed1b