From 9a4ec89429abbb9fd65f7d15df7a411120e6098f Mon Sep 17 00:00:00 2001 From: Vishnu Nair Date: Wed, 27 Mar 2024 22:20:24 -0700 Subject: DO NOT MERGE: 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 Change-Id: I4798961551f78d75c45ead6dea5ebca895e5ef7d --- services/surfaceflinger/SurfaceFlinger.cpp | 31 +++++++++++++++++----- services/surfaceflinger/SurfaceFlinger.h | 7 ++--- .../fuzzer/surfaceflinger_fuzzers_utils.h | 2 +- 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& 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&, VsyncId) REQUIRES(kMainThreadContext); - bool applyAndCommitDisplayTransactionStates(std::vector& transactions) - REQUIRES(kMainThreadContext); + bool applyAndCommitDisplayTransactionStatesLocked(std::vector& 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()); -- cgit v1.2.3