From 79dc06a39f4a31b65873271f22aecd91b37b75af Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 22 Feb 2022 15:28:59 -0800 Subject: BLASTBufferQueue/SF: apply transactions with one-way binder This CL has three main components: 1. Expose a flag through Transaction::apply that results in one-way calls to setTransactionState 2. Use this flag in all transactions from BBQ which use its own apply token. 3. Implement and use a new transaction barrier system to preserve ordering when switching apply tokens (e.g. BLASTSync) We can see that this CL is safe by making a few assumptions and then arguing that transaction ordering is preserved. We assume: 1. All transactions on the BBQ apply token will remain in order, since they are one-way calls from a single process to a single interface on another. AND 2. The ordering of transactions applied on seperate apply tokens is undefined 3. When preparing transactions for sync, BBQ will set a commit callback, and wait on it before applying another frame of it's own. But when preparing transactions to apply directly, it will not set a callback, and can not (for performance reasons) 4. The ordering of consecutive transactions in a sync is the responsibility of the sync consumer, e.g. SysUI or WM, who will be using their own apply token. Now imagine there were two transactions for frames N and N+1 which were applied in order before this CL, but out of order after. They can't both be applied by BBQ, by assumption one. They can't both be sync transactions (by assumption 4). It can't be a sync transaction applied by a bbq transaction, because we will wait on the callback, and we didn't modify the sync transaction to be applied one-way anyway. So our hypothetically disordered frame must be frame N (applied by BBQ) and frame N+1 (sync). When we analyze this case, we can see that we actually have a bug in the existing code. By assumption 2 and 4, frame N and N+1 will be applied on different apply tokens and so their ordering is undefined. We can solve the existing issue and enable one-way transactions at the same time. When BBQ prepares a transaction for sync following a transaction that has been directly applied (e.g. our aforementioned potentially undefined N,N+1 case) it uses a new API (setBufferHasBarrier) to set a barrier on the previous frame number. SurfaceFlinger interprets this barrier by forcing the barrier dependent transaction to remain in the transaction queue until a buffer with frameNumber >= the barrier frame has arrived. We chose >= because by assumption 4, the sync consumer may choose (probably incorrectly) to apply transactions out of order and we wouldn't want this to deadlock the transaction queue. The implementation itself is relatively well commented in SurfaceFlinger.cpp and so for details refer there. We see that use of this barrier system ensures our frame sequence was in order, since we tried every combination of (Sync, NotSync) and every frame is either sync or not sync, we can see that there are no frames which are not in order. We can apply similar analysis to slowly make setTransactionState async everywhere but we will need to eliminate the several "sync transaction" usages that remain (e.g. syncInputTransactions, etc). Most of these can be replaced with commit callbacks, but there is a lot to go through. Bug: 220929926 Test: Existing tests pass Change-Id: I3fd622966a9d12e4a197cf8560040f492dff996c --- services/surfaceflinger/SurfaceFlinger.cpp | 85 +++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 13 deletions(-) (limited to 'services/surfaceflinger/SurfaceFlinger.cpp') diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e8034de52c..9a5d5b2aa9 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3674,11 +3674,12 @@ bool SurfaceFlinger::stopTransactionProcessing( return false; } -void SurfaceFlinger::flushPendingTransactionQueues( +int SurfaceFlinger::flushPendingTransactionQueues( std::vector& transactions, - std::unordered_set, SpHash>& bufferLayersReadyToPresent, + std::unordered_map, uint64_t, SpHash>& bufferLayersReadyToPresent, std::unordered_set, SpHash>& applyTokensWithUnsignaledTransactions, bool tryApplyUnsignaled) { + int transactionsPendingBarrier = 0; auto it = mPendingTransactionQueues.begin(); while (it != mPendingTransactionQueues.end()) { auto& [applyToken, transactionQueue] = *it; @@ -3701,8 +3702,21 @@ void SurfaceFlinger::flushPendingTransactionQueues( setTransactionFlags(eTransactionFlushNeeded); break; } + if (ready == TransactionReadiness::NotReadyBarrier) { + transactionsPendingBarrier++; + setTransactionFlags(eTransactionFlushNeeded); + break; + } transaction.traverseStatesWithBuffers([&](const layer_state_t& state) { - bufferLayersReadyToPresent.insert(state.surface); + const bool frameNumberChanged = + state.bufferData->flags.test(BufferData::BufferDataChange::frameNumberChanged); + if (frameNumberChanged) { + bufferLayersReadyToPresent[state.surface] = state.bufferData->frameNumber; + } else { + // Barrier function only used for BBQ which always includes a frame number + bufferLayersReadyToPresent[state.surface] = + std::numeric_limits::max(); + } }); const bool appliedUnsignaled = (ready == TransactionReadiness::ReadyUnsignaled); if (appliedUnsignaled) { @@ -3720,6 +3734,7 @@ void SurfaceFlinger::flushPendingTransactionQueues( it = std::next(it, 1); } } + return transactionsPendingBarrier; } bool SurfaceFlinger::flushTransactionQueues(int64_t vsyncId) { @@ -3728,19 +3743,21 @@ bool SurfaceFlinger::flushTransactionQueues(int64_t vsyncId) { // states) around outside the scope of the lock std::vector transactions; // Layer handles that have transactions with buffers that are ready to be applied. - std::unordered_set, SpHash> bufferLayersReadyToPresent; + std::unordered_map, uint64_t, SpHash> bufferLayersReadyToPresent; std::unordered_set, SpHash> applyTokensWithUnsignaledTransactions; { Mutex::Autolock _l(mStateLock); { Mutex::Autolock _l(mQueueLock); + int lastTransactionsPendingBarrier = 0; + int transactionsPendingBarrier = 0; // First collect transactions from the pending transaction queues. // We are not allowing unsignaled buffers here as we want to // collect all the transactions from applyTokens that are ready first. - flushPendingTransactionQueues(transactions, bufferLayersReadyToPresent, - applyTokensWithUnsignaledTransactions, - /*tryApplyUnsignaled*/ false); + transactionsPendingBarrier = + flushPendingTransactionQueues(transactions, bufferLayersReadyToPresent, + applyTokensWithUnsignaledTransactions, /*tryApplyUnsignaled*/ false); // Second, collect transactions from the transaction queue. // Here as well we are not allowing unsignaled buffers for the same @@ -3765,18 +3782,48 @@ bool SurfaceFlinger::flushTransactionQueues(int64_t vsyncId) { /*tryApplyUnsignaled*/ false); }(); ATRACE_INT("TransactionReadiness", static_cast(ready)); - if (ready == TransactionReadiness::NotReady) { + if (ready != TransactionReadiness::Ready) { + if (ready == TransactionReadiness::NotReadyBarrier) { + transactionsPendingBarrier++; + } mPendingTransactionQueues[transaction.applyToken].push(std::move(transaction)); } else { transaction.traverseStatesWithBuffers([&](const layer_state_t& state) { - bufferLayersReadyToPresent.insert(state.surface); - }); + const bool frameNumberChanged = + state.bufferData->flags.test(BufferData::BufferDataChange::frameNumberChanged); + if (frameNumberChanged) { + bufferLayersReadyToPresent[state.surface] = state.bufferData->frameNumber; + } else { + // Barrier function only used for BBQ which always includes a frame number. + // This value only used for barrier logic. + bufferLayersReadyToPresent[state.surface] = + std::numeric_limits::max(); + } + }); transactions.emplace_back(std::move(transaction)); } mTransactionQueue.pop_front(); ATRACE_INT("TransactionQueue", mTransactionQueue.size()); } + // Transactions with a buffer pending on a barrier may be on a different applyToken + // than the transaction which satisfies our barrier. In fact this is the exact use case + // that the primitive is designed for. This means we may first process + // the barrier dependent transaction, determine it ineligible to complete + // and then satisfy in a later inner iteration of flushPendingTransactionQueues. + // The barrier dependent transaction was eligible to be presented in this frame + // but we would have prevented it without case. To fix this we continually + // loop through flushPendingTransactionQueues until we perform an iteration + // where the number of transactionsPendingBarrier doesn't change. This way + // we can continue to resolve dependency chains of barriers as far as possible. + while (lastTransactionsPendingBarrier != transactionsPendingBarrier) { + lastTransactionsPendingBarrier = transactionsPendingBarrier; + transactionsPendingBarrier = + flushPendingTransactionQueues(transactions, bufferLayersReadyToPresent, + applyTokensWithUnsignaledTransactions, + /*tryApplyUnsignaled*/ false); + } + // We collected all transactions that could apply without latching unsignaled buffers. // If we are allowing latch unsignaled of some form, now it's the time to go over the // transactions that were not applied and try to apply them unsignaled. @@ -3892,7 +3939,8 @@ bool SurfaceFlinger::shouldLatchUnsignaled(const sp& layer, const layer_s auto SurfaceFlinger::transactionIsReadyToBeApplied( const FrameTimelineInfo& info, bool isAutoTimestamp, int64_t desiredPresentTime, uid_t originUid, const Vector& states, - const std::unordered_set, SpHash>& bufferLayersReadyToPresent, + const std::unordered_map< + sp, uint64_t, SpHash>& bufferLayersReadyToPresent, size_t totalTXapplied, bool tryApplyUnsignaled) const -> TransactionReadiness { ATRACE_FORMAT("transactionIsReadyToBeApplied vsyncId: %" PRId64, info.vsyncId); const nsecs_t expectedPresentTime = mExpectedPresentTime.load(); @@ -3930,6 +3978,17 @@ auto SurfaceFlinger::transactionIsReadyToBeApplied( continue; } + if (s.hasBufferChanges() && s.bufferData->hasBarrier && + ((layer->getDrawingState().frameNumber) < s.bufferData->barrierFrameNumber)) { + const bool willApplyBarrierFrame = + (bufferLayersReadyToPresent.find(s.surface) != bufferLayersReadyToPresent.end()) && + (bufferLayersReadyToPresent.at(s.surface) >= s.bufferData->barrierFrameNumber); + if (!willApplyBarrierFrame) { + ATRACE_NAME("NotReadyBarrier"); + return TransactionReadiness::NotReadyBarrier; + } + } + const bool allowLatchUnsignaled = tryApplyUnsignaled && shouldLatchUnsignaled(layer, s, states.size(), totalTXapplied); ATRACE_FORMAT("%s allowLatchUnsignaled=%s", layer->getName().c_str(), @@ -3950,8 +4009,8 @@ auto SurfaceFlinger::transactionIsReadyToBeApplied( if (s.hasBufferChanges()) { // If backpressure is enabled and we already have a buffer to commit, keep the // transaction in the queue. - const bool hasPendingBuffer = - bufferLayersReadyToPresent.find(s.surface) != bufferLayersReadyToPresent.end(); + const bool hasPendingBuffer = bufferLayersReadyToPresent.find(s.surface) != + bufferLayersReadyToPresent.end(); if (layer->backpressureEnabled() && hasPendingBuffer && isAutoTimestamp) { ATRACE_NAME("hasPendingBuffer"); return TransactionReadiness::NotReady; -- cgit v1.2.3-59-g8ed1b