Remove RenderEngine::flush from latchBuffer()
Fence fd will be owned by SurfaceFlinger to be propagated down to
latchBuffer for merging sync fences. We can't store the fd at the
Layer-level & flush on every draw() invocation for some reason (I don't
fully understand what the gl driver does under the hood, but from what
it looks like file descriptors are reused when the command stream is
flushed too often which causes a memory leak when a buffer's sync fence
is merged), so we'll let SF backend store the flush fence for the
previous frame.
Bug: 116277151
Change-Id: I7901b0178aa0f11505650bf5e1df6f085a5d93bf
Test: SurfaceFlinger_test, libsurfaceflinger_unittest, go/wm-smoke
diff --git a/services/surfaceflinger/BufferLayer.cpp b/services/surfaceflinger/BufferLayer.cpp
index 89394cd..036ad7c 100644
--- a/services/surfaceflinger/BufferLayer.cpp
+++ b/services/surfaceflinger/BufferLayer.cpp
@@ -327,7 +327,8 @@
return true;
}
-Region BufferLayer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime) {
+Region BufferLayer::latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime,
+ const sp<Fence>& releaseFence) {
ATRACE_CALL();
std::optional<Region> sidebandStreamDirtyRegion = latchSidebandStream(recomputeVisibleRegions);
@@ -368,7 +369,7 @@
return dirtyRegion;
}
- status_t err = updateTexImage(recomputeVisibleRegions, latchTime);
+ status_t err = updateTexImage(recomputeVisibleRegions, latchTime, releaseFence);
if (err != NO_ERROR) {
return dirtyRegion;
}
diff --git a/services/surfaceflinger/BufferLayer.h b/services/surfaceflinger/BufferLayer.h
index 45906ff..36ec3fc 100644
--- a/services/surfaceflinger/BufferLayer.h
+++ b/services/surfaceflinger/BufferLayer.h
@@ -91,7 +91,11 @@
// the visible regions need to be recomputed (this is a fairly heavy
// operation, so this should be set only if needed). Typically this is used
// to figure out if the content or size of a surface has changed.
- Region latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime) override;
+ // If there was a GL composition step rendering the previous frame, then
+ // releaseFence will be populated with a native fence that fires when
+ // composition has completed.
+ Region latchBuffer(bool& recomputeVisibleRegions, nsecs_t latchTime,
+ const sp<Fence>& releaseFence) override;
bool isBufferLatched() const override { return mRefreshPending; }
@@ -135,7 +139,8 @@
virtual void setFilteringEnabled(bool enabled) = 0;
virtual status_t bindTextureImage() const = 0;
- virtual status_t updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime) = 0;
+ virtual status_t updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime,
+ const sp<Fence>& flushFence) = 0;
virtual status_t updateActiveBuffer() = 0;
virtual status_t updateFrameNumber(nsecs_t latchTime) = 0;
diff --git a/services/surfaceflinger/BufferLayerConsumer.cpp b/services/surfaceflinger/BufferLayerConsumer.cpp
index 91948ae..f499f06 100644
--- a/services/surfaceflinger/BufferLayerConsumer.cpp
+++ b/services/surfaceflinger/BufferLayerConsumer.cpp
@@ -147,7 +147,8 @@
status_t BufferLayerConsumer::updateTexImage(BufferRejecter* rejecter, const DispSync& dispSync,
bool* autoRefresh, bool* queuedBuffer,
- uint64_t maxFrameNumber) {
+ uint64_t maxFrameNumber,
+ const sp<Fence>& releaseFence) {
ATRACE_CALL();
BLC_LOGV("updateTexImage");
Mutex::Autolock lock(mMutex);
@@ -198,7 +199,7 @@
}
// Release the previous buffer.
- err = updateAndReleaseLocked(item, &mPendingRelease);
+ err = updateAndReleaseLocked(item, &mPendingRelease, releaseFence);
if (err != NO_ERROR) {
return err;
}
@@ -278,14 +279,15 @@
}
status_t BufferLayerConsumer::updateAndReleaseLocked(const BufferItem& item,
- PendingRelease* pendingRelease) {
+ PendingRelease* pendingRelease,
+ const sp<Fence>& releaseFence) {
status_t err = NO_ERROR;
int slot = item.mSlot;
// Do whatever sync ops we need to do before releasing the old slot.
if (slot != mCurrentTexture) {
- err = syncForReleaseLocked();
+ err = syncForReleaseLocked(releaseFence);
if (err != NO_ERROR) {
// Release the buffer we just acquired. It's not safe to
// release the old buffer, so instead we just drop the new frame.
@@ -367,19 +369,19 @@
return doFenceWaitLocked();
}
-status_t BufferLayerConsumer::syncForReleaseLocked() {
+status_t BufferLayerConsumer::syncForReleaseLocked(const sp<Fence>& releaseFence) {
BLC_LOGV("syncForReleaseLocked");
if (mCurrentTexture != BufferQueue::INVALID_BUFFER_SLOT) {
- if (mRE.useNativeFenceSync()) {
- base::unique_fd fenceFd = mRE.flush();
- if (fenceFd == -1) {
+ if (mRE.useNativeFenceSync() && releaseFence != Fence::NO_FENCE) {
+ // TODO(alecmouri): fail further upstream if the fence is invalid
+ if (!releaseFence->isValid()) {
BLC_LOGE("syncForReleaseLocked: failed to flush RenderEngine");
return UNKNOWN_ERROR;
}
- sp<Fence> fence(new Fence(std::move(fenceFd)));
- status_t err = addReleaseFenceLocked(mCurrentTexture,
- mCurrentTextureImage->graphicBuffer(), fence);
+ status_t err =
+ addReleaseFenceLocked(mCurrentTexture, mCurrentTextureImage->graphicBuffer(),
+ releaseFence);
if (err != OK) {
BLC_LOGE("syncForReleaseLocked: error adding release fence: "
"%s (%d)",
diff --git a/services/surfaceflinger/BufferLayerConsumer.h b/services/surfaceflinger/BufferLayerConsumer.h
index 257a4e5..dc00487 100644
--- a/services/surfaceflinger/BufferLayerConsumer.h
+++ b/services/surfaceflinger/BufferLayerConsumer.h
@@ -94,7 +94,8 @@
// used to reject the newly acquired buffer. It also does not bind the
// RenderEngine texture until bindTextureImage is called.
status_t updateTexImage(BufferRejecter* rejecter, const DispSync& dispSync, bool* autoRefresh,
- bool* queuedBuffer, uint64_t maxFrameNumber);
+ bool* queuedBuffer, uint64_t maxFrameNumber,
+ const sp<Fence>& releaseFence);
// See BufferLayerConsumer::bindTextureImageLocked().
status_t bindTextureImage();
@@ -208,7 +209,8 @@
// completion of the method will instead be returned to the caller, so that
// it may call releaseBufferLocked itself later.
status_t updateAndReleaseLocked(const BufferItem& item,
- PendingRelease* pendingRelease = nullptr);
+ PendingRelease* pendingRelease = nullptr,
+ const sp<Fence>& releaseFence = Fence::NO_FENCE);
// Binds mTexName and the current buffer to TEXTURE_EXTERNAL target. Uses
// mCurrentTexture if it's set, mCurrentTextureImage if not. If the
@@ -282,7 +284,7 @@
// current slot from RenderEngine. If needed it will set the current
// slot's fence to guard against a producer accessing the buffer before
// the outstanding accesses have completed.
- status_t syncForReleaseLocked();
+ status_t syncForReleaseLocked(const sp<Fence>& releaseFence);
// The default consumer usage flags that BufferLayerConsumer always sets on its
// BufferQueue instance; these will be OR:d with any additional flags passed
diff --git a/services/surfaceflinger/BufferQueueLayer.cpp b/services/surfaceflinger/BufferQueueLayer.cpp
index 521cdbf..8bb4574 100644
--- a/services/surfaceflinger/BufferQueueLayer.cpp
+++ b/services/surfaceflinger/BufferQueueLayer.cpp
@@ -236,7 +236,8 @@
return mConsumer->bindTextureImage();
}
-status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime) {
+status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime,
+ const sp<Fence>& releaseFence) {
// This boolean is used to make sure that SurfaceFlinger's shadow copy
// of the buffer queue isn't modified when the buffer queue is returning
// BufferItem's that weren't actually queued. This can happen in shared
@@ -247,7 +248,7 @@
getTransformToDisplayInverse(), mFreezeGeometryUpdates);
status_t updateResult =
mConsumer->updateTexImage(&r, *mFlinger->mPrimaryDispSync, &mAutoRefresh, &queuedBuffer,
- mLastFrameNumberReceived);
+ mLastFrameNumberReceived, releaseFence);
if (updateResult == BufferQueue::PRESENT_LATER) {
// Producer doesn't want buffer to be displayed yet. Signal a
// layer update so we check again at the next opportunity.
diff --git a/services/surfaceflinger/BufferQueueLayer.h b/services/surfaceflinger/BufferQueueLayer.h
index 400f412..f60a9c4 100644
--- a/services/surfaceflinger/BufferQueueLayer.h
+++ b/services/surfaceflinger/BufferQueueLayer.h
@@ -90,7 +90,8 @@
void setFilteringEnabled(bool enabled) override;
status_t bindTextureImage() const override;
- status_t updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime) override;
+ status_t updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime,
+ const sp<Fence>& releaseFence) override;
status_t updateActiveBuffer() override;
status_t updateFrameNumber(nsecs_t latchTime) override;
diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp
index da9dcfb..730262d 100644
--- a/services/surfaceflinger/BufferStateLayer.cpp
+++ b/services/surfaceflinger/BufferStateLayer.cpp
@@ -378,7 +378,8 @@
return NO_ERROR;
}
-status_t BufferStateLayer::updateTexImage(bool& /*recomputeVisibleRegions*/, nsecs_t latchTime) {
+status_t BufferStateLayer::updateTexImage(bool& /*recomputeVisibleRegions*/, nsecs_t latchTime,
+ const sp<Fence>& releaseFence) {
const State& s(getDrawingState());
if (!s.buffer) {
@@ -420,16 +421,13 @@
}
// Handle sync fences
- if (SyncFeatures::getInstance().useNativeFenceSync()) {
- base::unique_fd fenceFd = engine.flush();
- if (fenceFd == -1) {
- ALOGE("failed to flush RenderEngine");
+ if (SyncFeatures::getInstance().useNativeFenceSync() && releaseFence != Fence::NO_FENCE) {
+ // TODO(alecmouri): Fail somewhere upstream if the fence is invalid.
+ if (!releaseFence->isValid()) {
mTimeStats.clearLayerRecord(getName().c_str());
return UNKNOWN_ERROR;
}
- sp<Fence> fence(new Fence(std::move(fenceFd)));
-
// Check status of fences first because merging is expensive.
// Merging an invalid fence with any other fence results in an
// invalid fence.
@@ -440,10 +438,10 @@
return BAD_VALUE;
}
- auto incomingStatus = fence->getStatus();
+ auto incomingStatus = releaseFence->getStatus();
if (incomingStatus == Fence::Status::Invalid) {
ALOGE("New fence has invalid state");
- mDrawingState.acquireFence = fence;
+ mDrawingState.acquireFence = releaseFence;
mTimeStats.clearLayerRecord(getName().c_str());
return BAD_VALUE;
}
@@ -453,12 +451,13 @@
if (currentStatus == incomingStatus) {
char fenceName[32] = {};
snprintf(fenceName, 32, "%.28s:%d", mName.string(), mFrameNumber);
- sp<Fence> mergedFence = Fence::merge(fenceName, mDrawingState.acquireFence, fence);
+ sp<Fence> mergedFence =
+ Fence::merge(fenceName, mDrawingState.acquireFence, releaseFence);
if (!mergedFence.get()) {
ALOGE("failed to merge release fences");
// synchronization is broken, the best we can do is hope fences
// signal in order so the new fence will act like a union
- mDrawingState.acquireFence = fence;
+ mDrawingState.acquireFence = releaseFence;
mTimeStats.clearLayerRecord(getName().c_str());
return BAD_VALUE;
}
@@ -471,7 +470,7 @@
// by this point, they will have both signaled and only the timestamp
// will be slightly off; any dependencies after this point will
// already have been met.
- mDrawingState.acquireFence = fence;
+ mDrawingState.acquireFence = releaseFence;
}
} else {
// Bind the new buffer to the GL texture.
diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h
index ac3aad1..57bba63 100644
--- a/services/surfaceflinger/BufferStateLayer.h
+++ b/services/surfaceflinger/BufferStateLayer.h
@@ -115,7 +115,8 @@
void setFilteringEnabled(bool enabled) override;
status_t bindTextureImage() const override;
- status_t updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime) override;
+ status_t updateTexImage(bool& recomputeVisibleRegions, nsecs_t latchTime,
+ const sp<Fence>& releaseFence) override;
status_t updateActiveBuffer() override;
status_t updateFrameNumber(nsecs_t latchTime) override;
diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h
index 74f0a63..3273ce3 100644
--- a/services/surfaceflinger/Layer.h
+++ b/services/surfaceflinger/Layer.h
@@ -445,7 +445,8 @@
* operation, so this should be set only if needed). Typically this is used
* to figure out if the content or size of a surface has changed.
*/
- virtual Region latchBuffer(bool& /*recomputeVisibleRegions*/, nsecs_t /*latchTime*/) {
+ virtual Region latchBuffer(bool& /*recomputeVisibleRegions*/, nsecs_t /*latchTime*/,
+ const sp<Fence>& /*releaseFence*/) {
return {};
}
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 157cbea..b2d26de 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -1553,6 +1553,18 @@
getBE().mHwc->hasClientComposition(display->getId());
}
+ // Setup RenderEngine sync fences if native sync is supported.
+ if (getBE().mRenderEngine->useNativeFenceSync()) {
+ if (mHadClientComposition) {
+ base::unique_fd flushFence(getRenderEngine().flush());
+ ALOGE_IF(flushFence < 0, "Failed to flush RenderEngine!");
+ getBE().flushFence = new Fence(std::move(flushFence));
+ } else {
+ // Cleanup for hygiene.
+ getBE().flushFence = Fence::NO_FENCE;
+ }
+ }
+
mVsyncModulator.onRefreshed(mHadClientComposition);
getBE().mEndOfFrameCompositionInfo = std::move(getBE().mCompositionInfo);
@@ -3056,7 +3068,7 @@
});
for (auto& layer : mLayersWithQueuedFrames) {
- const Region dirty(layer->latchBuffer(visibleRegions, latchTime));
+ const Region dirty(layer->latchBuffer(visibleRegions, latchTime, getBE().flushFence));
layer->useSurfaceDamage();
invalidateLayerStack(layer, dirty);
if (layer->isBufferLatched()) {
@@ -3064,6 +3076,11 @@
}
}
+ // Clear the renderengine fence here...
+ // downstream code assumes that a cleared fence == NO_FENCE, so reassign to
+ // clear instead of sp::clear.
+ getBE().flushFence = Fence::NO_FENCE;
+
mVisibleRegionsDirty |= visibleRegions;
// If we will need to wake up at some time in the future to deal with a
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index e2be544..6c70b57 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -199,6 +199,9 @@
nsecs_t mTotalTime;
std::atomic<nsecs_t> mLastSwapTime;
+ // Synchronization fence from a GL composition.
+ sp<Fence> flushFence = Fence::NO_FENCE;
+
// Double- vs. triple-buffering stats
struct BufferingStats {
BufferingStats()
diff --git a/services/surfaceflinger/tests/unittests/CompositionTest.cpp b/services/surfaceflinger/tests/unittests/CompositionTest.cpp
index a5ac127..442f314 100644
--- a/services/surfaceflinger/tests/unittests/CompositionTest.cpp
+++ b/services/surfaceflinger/tests/unittests/CompositionTest.cpp
@@ -479,7 +479,7 @@
EXPECT_CALL(*test->mRenderEngine, createImage())
.WillOnce(Return(ByMove(std::unique_ptr<renderengine::Image>(test->mReImage))));
bool ignoredRecomputeVisibleRegions;
- layer->latchBuffer(ignoredRecomputeVisibleRegions, 0);
+ layer->latchBuffer(ignoredRecomputeVisibleRegions, 0, Fence::NO_FENCE);
Mock::VerifyAndClear(test->mRenderEngine);
Mock::VerifyAndClear(test->mReImage);
}