Add producerId so we know when the BBQ producer has been changed.
If BBQ producer changes but the SC remains the same, the frame numbers
for the SC will get reset. This causes issues if there's a barrier layer
set because the barrier is waiting for a particular frame number before
applying the transaction. Since the frame numbers have been reset, the
barrier will be greater than the incoming frame numbers. The change adds
a producerId to the buffer being sent so it can check if the producerId
is older than what's currently set on the Layer. If there's a barriers
set from the old producer, the buffer can be released and not applied
and will stop SF from waiting indefinitely.
Bug: 251971691
Test: Builds, hard to repro
Signed-off-by: Liu Lijun<liulijun@xiaomi.corp-partner.google.com>
Change-Id: If37171de4693a73f36f8de43e29c129b352eb55f
diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 66c0041..9d82c14 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -20,6 +20,7 @@
#define ATRACE_TAG ATRACE_TAG_GRAPHICS
//#define LOG_NDEBUG 0
+#include <cutils/atomic.h>
#include <gui/BLASTBufferQueue.h>
#include <gui/BufferItemConsumer.h>
#include <gui/BufferQueueConsumer.h>
@@ -157,11 +158,11 @@
GraphicBuffer::USAGE_HW_COMPOSER |
GraphicBuffer::USAGE_HW_TEXTURE,
1, false, this);
- static int32_t id = 0;
- mName = name + "#" + std::to_string(id);
- auto consumerName = mName + "(BLAST Consumer)" + std::to_string(id);
- mQueuedBufferTrace = "QueuedBuffer - " + mName + "BLAST#" + std::to_string(id);
- id++;
+ static std::atomic<uint32_t> nextId = 0;
+ mProducerId = nextId++;
+ mName = name + "#" + std::to_string(mProducerId);
+ auto consumerName = mName + "(BLAST Consumer)" + std::to_string(mProducerId);
+ mQueuedBufferTrace = "QueuedBuffer - " + mName + "BLAST#" + std::to_string(mProducerId);
mBufferItemConsumer->setName(String8(consumerName.c_str()));
mBufferItemConsumer->setFrameAvailableListener(this);
@@ -572,7 +573,8 @@
std::bind(releaseBufferCallbackThunk, wp<BLASTBufferQueue>(this) /* callbackContext */,
std::placeholders::_1, std::placeholders::_2, std::placeholders::_3);
sp<Fence> fence = bufferItem.mFence ? new Fence(bufferItem.mFence->dup()) : Fence::NO_FENCE;
- t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, releaseBufferCallback);
+ t->setBuffer(mSurfaceControl, buffer, fence, bufferItem.mFrameNumber, mProducerId,
+ releaseBufferCallback);
t->setDataspace(mSurfaceControl, static_cast<ui::Dataspace>(bufferItem.mDataSpace));
t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata);
t->setSurfaceDamageRegion(mSurfaceControl, bufferItem.mSurfaceDamage);
diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index 7772a65..a6276e5 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -984,6 +984,7 @@
SAFE_PARCEL(output->writeUint64, cachedBuffer.id);
SAFE_PARCEL(output->writeBool, hasBarrier);
SAFE_PARCEL(output->writeUint64, barrierFrameNumber);
+ SAFE_PARCEL(output->writeUint32, producerId);
return NO_ERROR;
}
@@ -1022,6 +1023,7 @@
SAFE_PARCEL(input->readBool, &hasBarrier);
SAFE_PARCEL(input->readUint64, &barrierFrameNumber);
+ SAFE_PARCEL(input->readUint32, &producerId);
return NO_ERROR;
}
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index a345938..5088604 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -1632,7 +1632,7 @@
SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffer(
const sp<SurfaceControl>& sc, const sp<GraphicBuffer>& buffer,
const std::optional<sp<Fence>>& fence, const std::optional<uint64_t>& optFrameNumber,
- ReleaseBufferCallback callback) {
+ uint32_t producerId, ReleaseBufferCallback callback) {
layer_state_t* s = getLayerState(sc);
if (!s) {
mStatus = BAD_INDEX;
@@ -1651,6 +1651,7 @@
bufferData->buffer = buffer;
uint64_t frameNumber = sc->resolveFrameNumber(optFrameNumber);
bufferData->frameNumber = frameNumber;
+ bufferData->producerId = producerId;
bufferData->flags |= BufferData::BufferDataChange::frameNumberChanged;
if (fence) {
bufferData->acquireFence = *fence;
diff --git a/libs/gui/include/gui/BLASTBufferQueue.h b/libs/gui/include/gui/BLASTBufferQueue.h
index 8d07162..b9e0647 100644
--- a/libs/gui/include/gui/BLASTBufferQueue.h
+++ b/libs/gui/include/gui/BLASTBufferQueue.h
@@ -162,6 +162,11 @@
int32_t mNumFrameAvailable GUARDED_BY(mMutex) = 0;
int32_t mNumAcquired GUARDED_BY(mMutex) = 0;
+ // A value used to identify if a producer has been changed for the same SurfaceControl.
+ // This is needed to know when the frame number has been reset to make sure we don't
+ // latch stale buffers and that we don't wait on barriers from an old producer.
+ uint32_t mProducerId = 0;
+
// Keep a reference to the submitted buffers so we can release when surfaceflinger drops the
// buffer or the buffer has been presented and a new buffer is ready to be presented.
std::unordered_map<ReleaseCallbackId, BufferItem, ReleaseBufferCallbackIdHash> mSubmitted
diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h
index 03a2582..ddaf473 100644
--- a/libs/gui/include/gui/LayerState.h
+++ b/libs/gui/include/gui/LayerState.h
@@ -111,6 +111,7 @@
uint64_t frameNumber = 0;
bool hasBarrier = false;
uint64_t barrierFrameNumber = 0;
+ uint32_t producerId = 0;
// Listens to when the buffer is safe to be released. This is used for blast
// layers only. The callback includes a release fence as well as the graphic
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 809ea5a..ffd5af1 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -536,7 +536,7 @@
Transaction& setBuffer(const sp<SurfaceControl>& sc, const sp<GraphicBuffer>& buffer,
const std::optional<sp<Fence>>& fence = std::nullopt,
const std::optional<uint64_t>& frameNumber = std::nullopt,
- ReleaseBufferCallback callback = nullptr);
+ uint32_t producerId = 0, ReleaseBufferCallback callback = nullptr);
std::shared_ptr<BufferData> getAndClearBuffer(const sp<SurfaceControl>& sc);
/**
diff --git a/services/surfaceflinger/FrontEnd/TransactionHandler.h b/services/surfaceflinger/FrontEnd/TransactionHandler.h
index a06b870..7fc825e 100644
--- a/services/surfaceflinger/FrontEnd/TransactionHandler.h
+++ b/services/surfaceflinger/FrontEnd/TransactionHandler.h
@@ -34,7 +34,7 @@
class TransactionHandler {
public:
struct TransactionFlushState {
- const TransactionState* transaction;
+ TransactionState* transaction;
bool firstTransaction = true;
nsecs_t queueProcessTime = 0;
// Layer handles that have transactions with buffers that are ready to be applied.
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 66c2fb6..8c484f0 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -228,9 +228,7 @@
if (mBufferInfo.mBuffer != nullptr) {
callReleaseBufferCallback(mDrawingState.releaseBufferListener,
mBufferInfo.mBuffer->getBuffer(), mBufferInfo.mFrameNumber,
- mBufferInfo.mFence,
- mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(
- mOwnerUid));
+ mBufferInfo.mFence);
}
if (!isClone()) {
// The original layer and the clone layer share the same texture. Therefore, only one of
@@ -2732,12 +2730,13 @@
void Layer::callReleaseBufferCallback(const sp<ITransactionCompletedListener>& listener,
const sp<GraphicBuffer>& buffer, uint64_t framenumber,
- const sp<Fence>& releaseFence,
- uint32_t currentMaxAcquiredBufferCount) {
+ const sp<Fence>& releaseFence) {
if (!listener) {
return;
}
ATRACE_FORMAT_INSTANT("callReleaseBufferCallback %s - %" PRIu64, getDebugName(), framenumber);
+ uint32_t currentMaxAcquiredBufferCount =
+ mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(mOwnerUid);
listener->onReleaseBuffer({buffer->getId(), framenumber},
releaseFence ? releaseFence : Fence::NO_FENCE,
currentMaxAcquiredBufferCount);
@@ -2988,9 +2987,7 @@
// call any release buffer callbacks if set.
callReleaseBufferCallback(mDrawingState.releaseBufferListener,
mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber,
- mDrawingState.acquireFence,
- mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(
- mOwnerUid));
+ mDrawingState.acquireFence);
decrementPendingBufferCount();
if (mDrawingState.bufferSurfaceFrameTX != nullptr &&
mDrawingState.bufferSurfaceFrameTX->getPresentState() != PresentState::Presented) {
@@ -3000,13 +2997,12 @@
} else if (EARLY_RELEASE_ENABLED && mLastClientCompositionFence != nullptr) {
callReleaseBufferCallback(mDrawingState.releaseBufferListener,
mDrawingState.buffer->getBuffer(), mDrawingState.frameNumber,
- mLastClientCompositionFence,
- mFlinger->getMaxAcquiredBufferCountForCurrentRefreshRate(
- mOwnerUid));
+ mLastClientCompositionFence);
mLastClientCompositionFence = nullptr;
}
}
+ mDrawingState.producerId = bufferData.producerId;
mDrawingState.frameNumber = frameNumber;
mDrawingState.releaseBufferListener = bufferData.releaseBufferListener;
mDrawingState.buffer = std::move(buffer);
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 07c01d8..bf8cc7e 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -141,6 +141,8 @@
uint64_t frameNumber;
ui::Transform transform;
+
+ uint32_t producerId = 0;
uint32_t bufferTransform;
bool transformToDisplayInverse;
Region transparentRegionHint;
@@ -838,6 +840,10 @@
std::unordered_set<Layer*>& visited);
bool willPresentCurrentTransaction() const;
+ void callReleaseBufferCallback(const sp<ITransactionCompletedListener>& listener,
+ const sp<GraphicBuffer>& buffer, uint64_t framenumber,
+ const sp<Fence>& releaseFence);
+
protected:
// For unit tests
friend class TestableSurfaceFlinger;
@@ -1047,6 +1053,10 @@
const sp<Fence>& releaseFence,
uint32_t currentMaxAcquiredBufferCount);
+ // Returns true if the transformed buffer size does not match the layer size and we need
+ // to apply filtering.
+ bool bufferNeedsFiltering() const;
+
// Returns true if there is a valid color to fill.
bool fillsColor() const;
// Returns true if this layer has a blur value.
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 68ab776..e74656e 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -4058,16 +4058,30 @@
sp<Layer> layer = LayerHandle::getLayer(s.surface);
const auto& transaction = *flushState.transaction;
// check for barrier frames
- if (s.bufferData->hasBarrier &&
- ((layer->getDrawingState().frameNumber) < s.bufferData->barrierFrameNumber)) {
- const bool willApplyBarrierFrame =
- flushState.bufferLayersReadyToPresent.contains(s.surface.get()) &&
- (flushState.bufferLayersReadyToPresent.get(s.surface.get()) >=
- s.bufferData->barrierFrameNumber);
- if (!willApplyBarrierFrame) {
- ATRACE_NAME("NotReadyBarrier");
- ready = TransactionReadiness::NotReadyBarrier;
- return false;
+ if (s.bufferData->hasBarrier) {
+ // The current producerId is already a newer producer than the buffer that has a
+ // barrier. This means the incoming buffer is older and we can release it here. We
+ // don't wait on the barrier since we know that's stale information.
+ if (layer->getDrawingState().producerId > s.bufferData->producerId) {
+ layer->callReleaseBufferCallback(s.bufferData->releaseBufferListener,
+ s.bufferData->buffer, s.bufferData->frameNumber,
+ s.bufferData->acquireFence);
+ // Delete the entire state at this point and not just release the buffer because
+ // everything associated with the Layer in this Transaction is now out of date.
+ ATRACE_NAME("DeleteStaleBuffer");
+ return TraverseBuffersReturnValues::DELETE_AND_CONTINUE_TRAVERSAL;
+ }
+
+ if (layer->getDrawingState().frameNumber < s.bufferData->barrierFrameNumber) {
+ const bool willApplyBarrierFrame =
+ flushState.bufferLayersReadyToPresent.contains(s.surface.get()) &&
+ ((flushState.bufferLayersReadyToPresent.get(s.surface.get()) >=
+ s.bufferData->barrierFrameNumber));
+ if (!willApplyBarrierFrame) {
+ ATRACE_NAME("NotReadyBarrier");
+ ready = TransactionReadiness::NotReadyBarrier;
+ return TraverseBuffersReturnValues::STOP_TRAVERSAL;
+ }
}
}
@@ -4078,7 +4092,7 @@
if (layer->backpressureEnabled() && hasPendingBuffer && transaction.isAutoTimestamp) {
ATRACE_NAME("hasPendingBuffer");
ready = TransactionReadiness::NotReady;
- return false;
+ return TraverseBuffersReturnValues::STOP_TRAVERSAL;
}
// check fence status
@@ -4105,14 +4119,14 @@
"Buffer processing hung up due to stuck "
"fence. Indicates GPU hang");
}
- return false;
+ return TraverseBuffersReturnValues::STOP_TRAVERSAL;
}
ready = enableLatchUnsignaledConfig == LatchUnsignaledConfig::AutoSingleLayer
? TransactionReadiness::ReadyUnsignaledSingle
: TransactionReadiness::ReadyUnsignaled;
}
- return true;
+ return TraverseBuffersReturnValues::CONTINUE_TRAVERSAL;
});
ATRACE_INT("TransactionReadiness", static_cast<int>(ready));
return ready;
diff --git a/services/surfaceflinger/TransactionState.h b/services/surfaceflinger/TransactionState.h
index 5025c49..6c5a8b2 100644
--- a/services/surfaceflinger/TransactionState.h
+++ b/services/surfaceflinger/TransactionState.h
@@ -27,6 +27,12 @@
namespace android {
+enum TraverseBuffersReturnValues {
+ CONTINUE_TRAVERSAL,
+ STOP_TRAVERSAL,
+ DELETE_AND_CONTINUE_TRAVERSAL,
+};
+
// Extends the client side composer state by resolving buffer.
class ResolvedComposerState : public ComposerState {
public:
@@ -75,12 +81,18 @@
}
template <typename Visitor>
- void traverseStatesWithBuffersWhileTrue(Visitor&& visitor) const {
- for (const auto& state : states) {
- if (state.state.hasBufferChanges() && state.state.hasValidBuffer() &&
- state.state.surface) {
- if (!visitor(state.state)) return;
+ void traverseStatesWithBuffersWhileTrue(Visitor&& visitor) {
+ for (auto state = states.begin(); state != states.end();) {
+ if (state->state.hasBufferChanges() && state->state.hasValidBuffer() &&
+ state->state.surface) {
+ int result = visitor(state->state);
+ if (result == STOP_TRAVERSAL) return;
+ if (result == DELETE_AND_CONTINUE_TRAVERSAL) {
+ state = states.erase(state);
+ continue;
+ }
}
+ state++;
}
}
diff --git a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
index 16076ea..c23fb9b 100644
--- a/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
+++ b/services/surfaceflinger/tests/ReleaseBufferCallback_test.cpp
@@ -85,7 +85,8 @@
sp<Fence> fence, CallbackHelper& callback, const ReleaseCallbackId& id,
ReleaseBufferCallbackHelper& releaseCallback) {
Transaction t;
- t.setBuffer(layer, buffer, fence, id.framenumber, releaseCallback.getCallback());
+ t.setBuffer(layer, buffer, fence, id.framenumber, 0 /* producerId */,
+ releaseCallback.getCallback());
t.addTransactionCompletedCallback(callback.function, callback.getContext());
t.apply();
}
@@ -301,7 +302,7 @@
Transaction t;
t.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
t.addTransactionCompletedCallback(transactionCallback.function,
transactionCallback.getContext());
t.setDesiredPresentTime(time);
@@ -317,7 +318,7 @@
sp<GraphicBuffer> secondBuffer = getBuffer();
ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
t.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
t.addTransactionCompletedCallback(transactionCallback.function,
transactionCallback.getContext());
t.setDesiredPresentTime(time);
@@ -362,7 +363,7 @@
Transaction transaction1;
transaction1.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
transaction1.addTransactionCompletedCallback(callback1.function, callback1.getContext());
// Set a different TransactionCompletedListener to mimic a second process
@@ -397,14 +398,14 @@
// Create transaction with a buffer.
Transaction transaction;
transaction.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
sp<GraphicBuffer> secondBuffer = getBuffer();
ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
// Call setBuffer on the same transaction with a different buffer.
transaction.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
}
@@ -419,7 +420,7 @@
// Create transaction with a buffer.
Transaction transaction1;
transaction1.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
sp<GraphicBuffer> secondBuffer = getBuffer();
ReleaseCallbackId secondBufferCallbackId(secondBuffer->getId(), generateFrameNumber());
@@ -427,7 +428,7 @@
// Create a second transaction with a new buffer for the same layer.
Transaction transaction2;
transaction2.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
// merge transaction1 into transaction2 so ensure we get a proper buffer release callback.
transaction1.merge(std::move(transaction2));
@@ -450,7 +451,7 @@
Transaction transaction1;
transaction1.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
// Sent a second buffer to allow the first buffer to get released.
sp<GraphicBuffer> secondBuffer = getBuffer();
@@ -458,7 +459,7 @@
Transaction transaction2;
transaction2.setBuffer(layer, secondBuffer, std::nullopt, secondBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
// Set a different TransactionCompletedListener to mimic a second process
TransactionCompletedListener::setInstance(secondCompletedListener);
@@ -479,10 +480,11 @@
// Create transaction with a buffer.
Transaction transaction;
transaction.setBuffer(layer, firstBuffer, std::nullopt, firstBufferCallbackId.framenumber,
- releaseCallback->getCallback());
+ 0 /* producerId */, releaseCallback->getCallback());
// Call setBuffer on the same transaction with a null buffer.
- transaction.setBuffer(layer, nullptr, std::nullopt, 0, releaseCallback->getCallback());
+ transaction.setBuffer(layer, nullptr, std::nullopt, 0, 0 /* producerId */,
+ releaseCallback->getCallback());
ASSERT_NO_FATAL_FAILURE(waitForReleaseBufferCallback(*releaseCallback, firstBufferCallbackId));
}