From c1cf402b81afb82a19bd1bedddd9568544b84808 Mon Sep 17 00:00:00 2001 From: chaviw Date: Fri, 3 Jun 2022 13:32:33 -0500 Subject: Account for release callbacks when determing if sync is finished The current code only considers sync complete when BBQ receives a commit callback for a frame that's >= to the last acquired buffer. This is because syncs always blocked anything that wasn't a sync so it was guaranteed that the last acquired buffer was for a sync. However, this doesn't work if we want to handle sync buffers that are dropped. Those transactions may never get applied and therefore the commit callback may never occur. In those cases, we still want to consider the sync complete if the buffer is released since it's never going to get applied. Test: BLASTBufferQueueTest Bug: 233625646 Change-Id: I43be3f8bfb41ef5d57e64ca118604fbbc7a81514 --- libs/gui/BLASTBufferQueue.cpp | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) (limited to 'libs/gui/BLASTBufferQueue.cpp') diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp index dbccf30fae..aba81f60d8 100644 --- a/libs/gui/BLASTBufferQueue.cpp +++ b/libs/gui/BLASTBufferQueue.cpp @@ -287,18 +287,17 @@ void BLASTBufferQueue::transactionCommittedCallback(nsecs_t /*latchTime*/, // We need to check if we were waiting for a transaction callback in order to // process any pending buffers and unblock. It's possible to get transaction - // callbacks for previous requests so we need to ensure the frame from this - // transaction callback matches the last acquired buffer. Since acquireNextBuffer - // will stop processing buffers when mWaitForTransactionCallback is set, we know - // that mLastAcquiredFrameNumber is the frame we're waiting on. - // We also want to check if mNextTransaction is null because it's possible another + // callbacks for previous requests so we need to ensure that there are no pending + // frame numbers that were in a sync. We remove the frame from mSyncedFrameNumbers + // set and then check if it's empty. If there are no more pending syncs, we can + // proceed with flushing the shadow queue. + // We also want to check if mSyncTransaction is null because it's possible another // sync request came in while waiting, but it hasn't started processing yet. In that // case, we don't actually want to flush the frames in between since they will get // processed and merged with the sync transaction and released earlier than if they // were sent to SF - if (mWaitForTransactionCallback && mSyncTransaction == nullptr && - currFrameNumber >= mLastAcquiredFrameNumber) { - mWaitForTransactionCallback = false; + mSyncedFrameNumbers.erase(currFrameNumber); + if (mSyncedFrameNumbers.empty() && mSyncTransaction == nullptr) { flushShadowQueue(); } } else { @@ -416,9 +415,11 @@ void BLASTBufferQueue::releaseBufferCallback( const auto releasedBuffer = mPendingRelease.front(); mPendingRelease.pop_front(); releaseBuffer(releasedBuffer.callbackId, releasedBuffer.releaseFence); - // Don't process the transactions here if mWaitForTransactionCallback is set. Instead, let - // onFrameAvailable handle processing them since it will merge with the syncTransaction. - if (!mWaitForTransactionCallback) { + // Don't process the transactions here if mSyncedFrameNumbers is not empty. That means + // are still transactions that have sync buffers in them that have not been applied or + // dropped. Instead, let onFrameAvailable handle processing them since it will merge with + // the syncTransaction. + if (mSyncedFrameNumbers.empty()) { acquireNextBufferLocked(std::nullopt); } } @@ -442,6 +443,9 @@ void BLASTBufferQueue::releaseBuffer(const ReleaseCallbackId& callbackId, BQA_LOGV("released %s", callbackId.to_string().c_str()); mBufferItemConsumer->releaseBuffer(it->second, releaseFence); mSubmitted.erase(it); + // Remove the frame number from mSyncedFrameNumbers since we can get a release callback + // without getting a transaction committed if the buffer was dropped. + mSyncedFrameNumbers.erase(callbackId.framenumber); } void BLASTBufferQueue::acquireNextBufferLocked( @@ -608,7 +612,7 @@ void BLASTBufferQueue::acquireAndReleaseBuffer() { } void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock& lock) { - if (mWaitForTransactionCallback && mNumFrameAvailable > 0) { + if (!mSyncedFrameNumbers.empty() && mNumFrameAvailable > 0) { // We are waiting on a previous sync's transaction callback so allow another sync // transaction to proceed. // @@ -635,6 +639,8 @@ void BLASTBufferQueue::flushAndWaitForFreeBuffer(std::unique_lock& l void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { std::function prevCallback = nullptr; SurfaceComposerClient::Transaction* prevTransaction = nullptr; + bool waitForTransactionCallback = !mSyncedFrameNumbers.empty(); + { BBQ_TRACE(); std::unique_lock _lock{mMutex}; @@ -666,7 +672,7 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { // add to shadow queue mNumFrameAvailable++; - if (mWaitForTransactionCallback && mNumFrameAvailable >= 2) { + if (waitForTransactionCallback && mNumFrameAvailable >= 2) { acquireAndReleaseBuffer(); } ATRACE_INT(mQueuedBufferTrace.c_str(), @@ -683,14 +689,14 @@ void BLASTBufferQueue::onFrameAvailable(const BufferItem& item) { incStrong((void*)transactionCommittedCallbackThunk); mSyncTransaction->addTransactionCommittedCallback(transactionCommittedCallbackThunk, static_cast(this)); - mWaitForTransactionCallback = true; + mSyncedFrameNumbers.emplace(item.mFrameNumber); if (mAcquireSingleBuffer) { prevCallback = mTransactionReadyCallback; prevTransaction = mSyncTransaction; mTransactionReadyCallback = nullptr; mSyncTransaction = nullptr; } - } else if (!mWaitForTransactionCallback) { + } else if (!waitForTransactionCallback) { acquireNextBufferLocked(std::nullopt); } } @@ -1097,9 +1103,9 @@ void BLASTBufferQueue::abandon() { } // Clear sync states - if (mWaitForTransactionCallback) { - BQA_LOGD("mWaitForTransactionCallback cleared"); - mWaitForTransactionCallback = false; + if (!mSyncedFrameNumbers.empty()) { + BQA_LOGD("mSyncedFrameNumbers cleared"); + mSyncedFrameNumbers.clear(); } if (mSyncTransaction != nullptr) { -- cgit v1.2.3-59-g8ed1b