Merge "DO NOT MERGE: Fix sync issue with handling display state changes" into main

3 files changed, 30 insertions, 10 deletions

diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index f7cc13e7c1..b980a65255 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -2252,7 +2252,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,
         }
     }
 
+    // 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;
@@ -3266,6 +3272,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
@@ -5070,9 +5089,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) {
@@ -5864,7 +5882,8 @@ void SurfaceFlinger::initializeDisplays() {
     if (mLegacyFrontEndEnabled) {
         applyTransactions(transactions, VsyncId{0});
     } else {
-        applyAndCommitDisplayTransactionStates(transactions);
+        Mutex::Autolock lock(mStateLock);
+        applyAndCommitDisplayTransactionStatesLocked(transactions);
     }
 
     {
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 6b4440193b..61360a70df 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -751,7 +751,8 @@ private:
                                             bool force = false)
             REQUIRES(mStateLock, kMainThreadContext);
 
-    void commitTransactions() EXCLUDES(mStateLock) REQUIRES(kMainThreadContext);
+    void commitTransactionsLegacy() EXCLUDES(mStateLock) REQUIRES(kMainThreadContext);
+    void commitTransactions() REQUIRES(kMainThreadContext, mStateLock);
     void commitTransactionsLocked(uint32_t transactionFlags)
             REQUIRES(mStateLock, kMainThreadContext);
     void doCommitTransactions() REQUIRES(mStateLock);
@@ -804,8 +805,8 @@ private:
     bool flushTransactionQueues(VsyncId) REQUIRES(kMainThreadContext);
 
     bool applyTransactions(std::vector<TransactionState>&, VsyncId) REQUIRES(kMainThreadContext);
-    bool applyAndCommitDisplayTransactionStates(std::vector<TransactionState>& transactions)
-            REQUIRES(kMainThreadContext);
+    bool applyAndCommitDisplayTransactionStatesLocked(std::vector<TransactionState>& transactions)
+            REQUIRES(kMainThreadContext, mStateLock);
 
     // Returns true if there is at least one transaction that needs to be flushed
     bool transactionFlushNeeded();
diff --git a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h
index 4fc39cc912..81fbc847a3 100644
--- a/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h
+++ b/services/surfaceflinger/fuzzer/surfaceflinger_fuzzers_utils.h
@@ -610,7 +610,7 @@ public:
         {
             ftl::FakeGuard guard(kMainThreadContext);
 
-            mFlinger->commitTransactions();
+            mFlinger->commitTransactionsLegacy();
             mFlinger->flushTransactionQueues(getFuzzedVsyncId(mFdp));
 
             scheduler::FrameTargeter frameTargeter(displayId, mFdp.ConsumeBool());