[sf] Fix latch unsignaled issues
With LatchUnsignaledConfig::AutoSingleLayer flag, we should only
apply a transaction with an unsignaled fence if its the only transaction.
If we pass an unsignaled fence to HWC, HWC might miss presenting
the frame if the fence does not fire in time. If we apply another
transaction we may penalize the other transaction unfairly.
With LatchUnsignaledConfig::Always flag, respect backpressure flag
and take the opportunity to simplify the logic by treating the flag as
a signal to ignore the fence when applying transactions.
Test: surfaceflinger unit tests
Bug: 276229937
Change-Id: Ib5cd8205d62fdf8912b7f5c2cad312a01cae4300
diff --git a/services/surfaceflinger/FrontEnd/TransactionHandler.cpp b/services/surfaceflinger/FrontEnd/TransactionHandler.cpp
index 8629671..a209cad 100644
--- a/services/surfaceflinger/FrontEnd/TransactionHandler.cpp
+++ b/services/surfaceflinger/FrontEnd/TransactionHandler.cpp
@@ -64,11 +64,61 @@
transactionsPendingBarrier = flushPendingTransactionQueues(transactions, flushState);
} while (lastTransactionsPendingBarrier != transactionsPendingBarrier);
+ applyUnsignaledBufferTransaction(transactions, flushState);
+
mPendingTransactionCount.fetch_sub(transactions.size());
ATRACE_INT("TransactionQueue", static_cast<int>(mPendingTransactionCount.load()));
return transactions;
}
+void TransactionHandler::applyUnsignaledBufferTransaction(
+ std::vector<TransactionState>& transactions, TransactionFlushState& flushState) {
+ // only apply an unsignaled buffer transaction if it's the first one
+ if (!transactions.empty()) {
+ return;
+ }
+
+ if (!flushState.queueWithUnsignaledBuffer) {
+ return;
+ }
+
+ auto it = mPendingTransactionQueues.find(flushState.queueWithUnsignaledBuffer);
+ LOG_ALWAYS_FATAL_IF(it == mPendingTransactionQueues.end(),
+ "Could not find queue with unsignaled buffer!");
+
+ auto& queue = it->second;
+ popTransactionFromPending(transactions, flushState, queue);
+ if (queue.empty()) {
+ it = mPendingTransactionQueues.erase(it);
+ }
+}
+
+void TransactionHandler::popTransactionFromPending(std::vector<TransactionState>& transactions,
+ TransactionFlushState& flushState,
+ std::queue<TransactionState>& queue) {
+ auto& transaction = queue.front();
+ // Transaction is ready move it from the pending queue.
+ flushState.firstTransaction = false;
+ removeFromStalledTransactions(transaction.id);
+ transactions.emplace_back(std::move(transaction));
+ queue.pop();
+
+ auto& readyToApplyTransaction = transactions.back();
+ readyToApplyTransaction.traverseStatesWithBuffers([&](const layer_state_t& state) {
+ const bool frameNumberChanged =
+ state.bufferData->flags.test(BufferData::BufferDataChange::frameNumberChanged);
+ if (frameNumberChanged) {
+ flushState.bufferLayersReadyToPresent.emplace_or_replace(state.surface.get(),
+ state.bufferData->frameNumber);
+ } else {
+ // Barrier function only used for BBQ which always includes a frame number.
+ // This value only used for barrier logic.
+ flushState.bufferLayersReadyToPresent
+ .emplace_or_replace(state.surface.get(), std::numeric_limits<uint64_t>::max());
+ }
+ });
+}
+
TransactionHandler::TransactionReadiness TransactionHandler::applyFilters(
TransactionFlushState& flushState) {
auto ready = TransactionReadiness::Ready;
@@ -79,8 +129,7 @@
case TransactionReadiness::NotReadyBarrier:
return perFilterReady;
- case TransactionReadiness::ReadyUnsignaled:
- case TransactionReadiness::ReadyUnsignaledSingle:
+ case TransactionReadiness::NotReadyUnsignaled:
// If one of the filters allows latching an unsignaled buffer, latch this ready
// state.
ready = perFilterReady;
@@ -97,17 +146,7 @@
int transactionsPendingBarrier = 0;
auto it = mPendingTransactionQueues.begin();
while (it != mPendingTransactionQueues.end()) {
- auto& queue = it->second;
- IBinder* queueToken = it->first.get();
-
- // if we have already flushed a transaction with an unsignaled buffer then stop queue
- // processing
- if (std::find(flushState.queuesWithUnsignaledBuffers.begin(),
- flushState.queuesWithUnsignaledBuffers.end(),
- queueToken) != flushState.queuesWithUnsignaledBuffers.end()) {
- continue;
- }
-
+ auto& [applyToken, queue] = *it;
while (!queue.empty()) {
auto& transaction = queue.front();
flushState.transaction = &transaction;
@@ -117,38 +156,14 @@
break;
} else if (ready == TransactionReadiness::NotReady) {
break;
- }
-
- // Transaction is ready move it from the pending queue.
- flushState.firstTransaction = false;
- removeFromStalledTransactions(transaction.id);
- transactions.emplace_back(std::move(transaction));
- queue.pop();
-
- // If the buffer is unsignaled, then we don't want to signal other transactions using
- // the buffer as a barrier.
- auto& readyToApplyTransaction = transactions.back();
- if (ready == TransactionReadiness::Ready) {
- readyToApplyTransaction.traverseStatesWithBuffers([&](const layer_state_t& state) {
- const bool frameNumberChanged = state.bufferData->flags.test(
- BufferData::BufferDataChange::frameNumberChanged);
- if (frameNumberChanged) {
- flushState.bufferLayersReadyToPresent
- .emplace_or_replace(state.surface.get(),
- state.bufferData->frameNumber);
- } else {
- // Barrier function only used for BBQ which always includes a frame number.
- // This value only used for barrier logic.
- flushState.bufferLayersReadyToPresent
- .emplace_or_replace(state.surface.get(),
- std::numeric_limits<uint64_t>::max());
- }
- });
- } else if (ready == TransactionReadiness::ReadyUnsignaledSingle) {
- // Track queues with a flushed unsingaled buffer.
- flushState.queuesWithUnsignaledBuffers.emplace_back(queueToken);
+ } else if (ready == TransactionReadiness::NotReadyUnsignaled) {
+ // We maybe able to latch this transaction if it's the only transaction
+ // ready to be applied.
+ flushState.queueWithUnsignaledBuffer = applyToken;
break;
}
+ // ready == TransactionReadiness::Ready
+ popTransactionFromPending(transactions, flushState, queue);
}
if (queue.empty()) {
diff --git a/services/surfaceflinger/FrontEnd/TransactionHandler.h b/services/surfaceflinger/FrontEnd/TransactionHandler.h
index 7fc825e..865835f 100644
--- a/services/surfaceflinger/FrontEnd/TransactionHandler.h
+++ b/services/surfaceflinger/FrontEnd/TransactionHandler.h
@@ -40,14 +40,20 @@
// Layer handles that have transactions with buffers that are ready to be applied.
ftl::SmallMap<IBinder* /* binder address */, uint64_t /* framenumber */, 15>
bufferLayersReadyToPresent = {};
- ftl::SmallVector<IBinder* /* queueToken */, 15> queuesWithUnsignaledBuffers;
+ // Tracks the queue with an unsignaled buffer. This is used to handle
+ // LatchUnsignaledConfig::AutoSingleLayer to ensure we only apply an unsignaled buffer
+ // if it's the only transaction that is ready to be applied.
+ sp<IBinder> queueWithUnsignaledBuffer = nullptr;
};
enum class TransactionReadiness {
- NotReady,
- NotReadyBarrier,
+ // Transaction is ready to be applied
Ready,
- ReadyUnsignaled,
- ReadyUnsignaledSingle,
+ // Transaction has unmet conditions (fence, present time, etc) and cannot be applied.
+ NotReady,
+ // Transaction is waiting on a barrier (another buffer to be latched first)
+ NotReadyBarrier,
+ // Transaction has an unsignaled fence but can be applied if it's the only transaction
+ NotReadyUnsignaled,
};
using TransactionFilter = std::function<TransactionReadiness(const TransactionFlushState&)>;
@@ -64,6 +70,9 @@
friend class ::android::TestableSurfaceFlinger;
int flushPendingTransactionQueues(std::vector<TransactionState>&, TransactionFlushState&);
+ void applyUnsignaledBufferTransaction(std::vector<TransactionState>&, TransactionFlushState&);
+ void popTransactionFromPending(std::vector<TransactionState>&, TransactionFlushState&,
+ std::queue<TransactionState>&);
TransactionReadiness applyFilters(TransactionFlushState&);
std::unordered_map<sp<IBinder>, std::queue<TransactionState>, IListenerHash>
mPendingTransactionQueues;
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index a271083..4be4d59 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4266,20 +4266,23 @@
return TraverseBuffersReturnValues::STOP_TRAVERSAL;
}
- // check fence status
- const bool allowLatchUnsignaled = shouldLatchUnsignaled(layer, s, transaction.states.size(),
- flushState.firstTransaction);
- ATRACE_FORMAT("%s allowLatchUnsignaled=%s", layer->getName().c_str(),
- allowLatchUnsignaled ? "true" : "false");
-
- const bool acquireFenceChanged = s.bufferData &&
+ // ignore the acquire fence if LatchUnsignaledConfig::Always is set.
+ const bool checkAcquireFence = enableLatchUnsignaledConfig != LatchUnsignaledConfig::Always;
+ const bool acquireFenceAvailable = s.bufferData &&
s.bufferData->flags.test(BufferData::BufferDataChange::fenceChanged) &&
s.bufferData->acquireFence;
- const bool fenceSignaled =
- (!acquireFenceChanged ||
- s.bufferData->acquireFence->getStatus() != Fence::Status::Unsignaled);
+ const bool fenceSignaled = !checkAcquireFence || !acquireFenceAvailable ||
+ s.bufferData->acquireFence->getStatus() != Fence::Status::Unsignaled;
if (!fenceSignaled) {
- if (!allowLatchUnsignaled) {
+ // check fence status
+ const bool allowLatchUnsignaled =
+ shouldLatchUnsignaled(layer, s, transaction.states.size(),
+ flushState.firstTransaction);
+ ATRACE_FORMAT("%s allowLatchUnsignaled=%s", layer->getName().c_str(),
+ allowLatchUnsignaled ? "true" : "false");
+ if (allowLatchUnsignaled) {
+ ready = TransactionReadiness::NotReadyUnsignaled;
+ } else {
ready = TransactionReadiness::NotReady;
auto& listener = s.bufferData->releaseBufferListener;
if (listener &&
@@ -4292,10 +4295,6 @@
}
return TraverseBuffersReturnValues::STOP_TRAVERSAL;
}
-
- ready = enableLatchUnsignaledConfig == LatchUnsignaledConfig::AutoSingleLayer
- ? TransactionReadiness::ReadyUnsignaledSingle
- : TransactionReadiness::ReadyUnsignaled;
}
return TraverseBuffersReturnValues::CONTINUE_TRAVERSAL;
});
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 74d00dd..fd6125c 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -174,10 +174,15 @@
// Latch unsignaled is permitted when a single layer is updated in a frame,
// and the update includes just a buffer update (i.e. no sync transactions
// or geometry changes).
+ // Latch unsignaled is also only permitted when a single transaction is ready
+ // to be applied. If we pass an unsignaled fence to HWC, HWC might miss presenting
+ // the frame if the fence does not fire in time. If we apply another transaction,
+ // we may penalize the other transaction unfairly.
AutoSingleLayer,
// All buffers are latched unsignaled. This behaviour is discouraged as it
// can break sync transactions, stall the display and cause undesired side effects.
+ // This is equivalent to ignoring the acquire fence when applying transactions.
Always,
};
diff --git a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
index d4e2357..03c4e71 100644
--- a/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
+++ b/services/surfaceflinger/tests/unittests/TransactionApplicationTest.cpp
@@ -294,7 +294,8 @@
return fence;
}
- ComposerState createComposerState(int layerId, sp<Fence> fence, uint64_t what) {
+ ComposerState createComposerState(int layerId, sp<Fence> fence, uint64_t what,
+ std::optional<sp<IBinder>> layerHandle = std::nullopt) {
ComposerState state;
state.state.bufferData =
std::make_shared<fake::BufferData>(/* bufferId */ 123L, /* width */ 1,
@@ -302,15 +303,20 @@
/* outUsage */ 0);
state.state.bufferData->acquireFence = std::move(fence);
state.state.layerId = layerId;
- state.state.surface =
+ state.state.surface = layerHandle.value_or(
sp<Layer>::make(LayerCreationArgs(mFlinger.flinger(), nullptr, "TestLayer", 0, {}))
- ->getHandle();
+ ->getHandle());
state.state.bufferData->flags = BufferData::BufferDataChange::fenceChanged;
state.state.what = what;
if (what & layer_state_t::eCropChanged) {
state.state.crop = Rect(1, 2, 3, 4);
}
+ if (what & layer_state_t::eFlagsChanged) {
+ state.state.flags = layer_state_t::eEnableBackpressure;
+ state.state.mask = layer_state_t::eEnableBackpressure;
+ }
+
return state;
}
@@ -601,6 +607,41 @@
setTransactionStates({unsignaledTransaction}, kExpectedTransactionsPending);
}
+TEST_F(LatchUnsignaledAutoSingleLayerTest, UnsignaledNotAppliedWhenThereAreSignaled_SignaledFirst) {
+ const sp<IBinder> kApplyToken1 =
+ IInterface::asBinder(TransactionCompletedListener::getIInstance());
+ const sp<IBinder> kApplyToken2 = sp<BBinder>::make();
+ const sp<IBinder> kApplyToken3 = sp<BBinder>::make();
+ const auto kLayerId1 = 1;
+ const auto kLayerId2 = 2;
+ const auto kExpectedTransactionsPending = 1u;
+
+ const auto signaledTransaction =
+ createTransactionInfo(kApplyToken1,
+ {
+ createComposerState(kLayerId1,
+ fence(Fence::Status::Signaled),
+ layer_state_t::eBufferChanged),
+ });
+ const auto signaledTransaction2 =
+ createTransactionInfo(kApplyToken2,
+ {
+ createComposerState(kLayerId1,
+ fence(Fence::Status::Signaled),
+ layer_state_t::eBufferChanged),
+ });
+ const auto unsignaledTransaction =
+ createTransactionInfo(kApplyToken3,
+ {
+ createComposerState(kLayerId2,
+ fence(Fence::Status::Unsignaled),
+ layer_state_t::eBufferChanged),
+ });
+
+ setTransactionStates({signaledTransaction, signaledTransaction2, unsignaledTransaction},
+ kExpectedTransactionsPending);
+}
+
class LatchUnsignaledDisabledTest : public LatchUnsignaledTest {
public:
void SetUp() override {
@@ -943,6 +984,43 @@
kExpectedTransactionsPending);
}
+TEST_F(LatchUnsignaledAlwaysTest, RespectsBackPressureFlag) {
+ const sp<IBinder> kApplyToken1 =
+ IInterface::asBinder(TransactionCompletedListener::getIInstance());
+ const sp<IBinder> kApplyToken2 = sp<BBinder>::make();
+ const auto kLayerId1 = 1;
+ const auto kExpectedTransactionsPending = 1u;
+ auto layer =
+ sp<Layer>::make(LayerCreationArgs(mFlinger.flinger(), nullptr, "TestLayer", 0, {}));
+ auto layerHandle = layer->getHandle();
+ const auto setBackPressureFlagTransaction =
+ createTransactionInfo(kApplyToken1,
+ {createComposerState(kLayerId1, fence(Fence::Status::Unsignaled),
+ layer_state_t::eBufferChanged |
+ layer_state_t::eFlagsChanged,
+ {layerHandle})});
+ setTransactionStates({setBackPressureFlagTransaction}, 0u);
+
+ const auto unsignaledTransaction =
+ createTransactionInfo(kApplyToken1,
+ {
+ createComposerState(kLayerId1,
+ fence(Fence::Status::Unsignaled),
+ layer_state_t::eBufferChanged,
+ {layerHandle}),
+ });
+ const auto unsignaledTransaction2 =
+ createTransactionInfo(kApplyToken1,
+ {
+ createComposerState(kLayerId1,
+ fence(Fence::Status::Unsignaled),
+ layer_state_t::eBufferChanged,
+ {layerHandle}),
+ });
+ setTransactionStates({unsignaledTransaction, unsignaledTransaction2},
+ kExpectedTransactionsPending);
+}
+
TEST_F(LatchUnsignaledAlwaysTest, LatchUnsignaledWhenEarlyOffset) {
const sp<IBinder> kApplyToken =
IInterface::asBinder(TransactionCompletedListener::getIInstance());