From 461296a52224f58ea4ffe7235d744260cb3ad01d Mon Sep 17 00:00:00 2001 From: Ady Abraham Date: Fri, 21 Jan 2022 11:11:31 -0800 Subject: SF: pass acquire fence on BLAST callbacks When latching unsignaled buffers, the acquire fence is not signaled by the time BLAST callback is invoked. In that case pass a fence instead. For latch signaled, we still pass the acquire time itself to avoid sending file descriptors over binder. Bug: 198190384 Test: SF unit tests Change-Id: Ic7ad9b603b60dbf46a62eaf6b76bfbdeeebf6cec --- libs/gui/ITransactionCompletedListener.cpp | 21 +++++++++++++++++++-- libs/gui/SurfaceComposerClient.cpp | 4 ++-- .../gui/include/gui/ITransactionCompletedListener.h | 13 +++++++------ libs/gui/include/gui/SurfaceComposerClient.h | 7 ++++--- libs/gui/include/gui/test/CallbackUtils.h | 11 +++++++---- services/surfaceflinger/BufferStateLayer.cpp | 14 ++++++++++---- services/surfaceflinger/BufferStateLayer.h | 2 +- .../surfaceflinger/TransactionCallbackInvoker.cpp | 2 +- .../surfaceflinger/TransactionCallbackInvoker.h | 2 +- 9 files changed, 52 insertions(+), 24 deletions(-) diff --git a/libs/gui/ITransactionCompletedListener.cpp b/libs/gui/ITransactionCompletedListener.cpp index aa7ebc9eb3..f7392d4969 100644 --- a/libs/gui/ITransactionCompletedListener.cpp +++ b/libs/gui/ITransactionCompletedListener.cpp @@ -111,7 +111,14 @@ status_t JankData::readFromParcel(const Parcel* input) { status_t SurfaceStats::writeToParcel(Parcel* output) const { SAFE_PARCEL(output->writeStrongBinder, surfaceControl); - SAFE_PARCEL(output->writeInt64, acquireTime); + if (const auto* acquireFence = std::get_if>(&acquireTimeOrFence)) { + SAFE_PARCEL(output->writeBool, true); + SAFE_PARCEL(output->write, **acquireFence); + } else { + SAFE_PARCEL(output->writeBool, false); + SAFE_PARCEL(output->writeInt64, std::get(acquireTimeOrFence)); + } + if (previousReleaseFence) { SAFE_PARCEL(output->writeBool, true); SAFE_PARCEL(output->write, *previousReleaseFence); @@ -131,8 +138,18 @@ status_t SurfaceStats::writeToParcel(Parcel* output) const { status_t SurfaceStats::readFromParcel(const Parcel* input) { SAFE_PARCEL(input->readStrongBinder, &surfaceControl); - SAFE_PARCEL(input->readInt64, &acquireTime); + bool hasFence = false; + SAFE_PARCEL(input->readBool, &hasFence); + if (hasFence) { + acquireTimeOrFence = sp::make(); + SAFE_PARCEL(input->read, *std::get>(acquireTimeOrFence)); + } else { + nsecs_t acquireTime; + SAFE_PARCEL(input->readInt64, &acquireTime); + acquireTimeOrFence = acquireTime; + } + SAFE_PARCEL(input->readBool, &hasFence); if (hasFence) { previousReleaseFence = new Fence(); diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 91b2fb1c3b..f78726c5a2 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -296,7 +296,7 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener surfaceControlStats .emplace_back(callbacksMap[callbackId] .surfaceControls[surfaceStats.surfaceControl], - transactionStats.latchTime, surfaceStats.acquireTime, + transactionStats.latchTime, surfaceStats.acquireTimeOrFence, transactionStats.presentFence, surfaceStats.previousReleaseFence, surfaceStats.transformHint, surfaceStats.eventStats); @@ -321,7 +321,7 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener surfaceControlStats .emplace_back(callbacksMap[callbackId] .surfaceControls[surfaceStats.surfaceControl], - transactionStats.latchTime, surfaceStats.acquireTime, + transactionStats.latchTime, surfaceStats.acquireTimeOrFence, transactionStats.presentFence, surfaceStats.previousReleaseFence, surfaceStats.transformHint, surfaceStats.eventStats); diff --git a/libs/gui/include/gui/ITransactionCompletedListener.h b/libs/gui/include/gui/ITransactionCompletedListener.h index 0df5822597..a791c665e1 100644 --- a/libs/gui/include/gui/ITransactionCompletedListener.h +++ b/libs/gui/include/gui/ITransactionCompletedListener.h @@ -30,6 +30,7 @@ #include #include #include +#include namespace android { @@ -130,12 +131,12 @@ public: status_t readFromParcel(const Parcel* input) override; SurfaceStats() = default; - SurfaceStats(const sp& sc, nsecs_t time, const sp& prevReleaseFence, - uint32_t hint, uint32_t currentMaxAcquiredBuffersCount, - FrameEventHistoryStats frameEventStats, std::vector jankData, - ReleaseCallbackId previousReleaseCallbackId) + SurfaceStats(const sp& sc, std::variant> acquireTimeOrFence, + const sp& prevReleaseFence, uint32_t hint, + uint32_t currentMaxAcquiredBuffersCount, FrameEventHistoryStats frameEventStats, + std::vector jankData, ReleaseCallbackId previousReleaseCallbackId) : surfaceControl(sc), - acquireTime(time), + acquireTimeOrFence(std::move(acquireTimeOrFence)), previousReleaseFence(prevReleaseFence), transformHint(hint), currentMaxAcquiredBufferCount(currentMaxAcquiredBuffersCount), @@ -144,7 +145,7 @@ public: previousReleaseCallbackId(previousReleaseCallbackId) {} sp surfaceControl; - nsecs_t acquireTime = -1; + std::variant> acquireTimeOrFence = -1; sp previousReleaseFence; uint32_t transformHint = 0; uint32_t currentMaxAcquiredBufferCount = 0; diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index 61eeab35e5..731ac65cd8 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -58,12 +58,13 @@ class Region; using gui::IRegionSamplingListener; struct SurfaceControlStats { - SurfaceControlStats(const sp& sc, nsecs_t latchTime, nsecs_t acquireTime, + SurfaceControlStats(const sp& sc, nsecs_t latchTime, + std::variant> acquireTimeOrFence, const sp& presentFence, const sp& prevReleaseFence, uint32_t hint, FrameEventHistoryStats eventStats) : surfaceControl(sc), latchTime(latchTime), - acquireTime(acquireTime), + acquireTimeOrFence(std::move(acquireTimeOrFence)), presentFence(presentFence), previousReleaseFence(prevReleaseFence), transformHint(hint), @@ -71,7 +72,7 @@ struct SurfaceControlStats { sp surfaceControl; nsecs_t latchTime = -1; - nsecs_t acquireTime = -1; + std::variant> acquireTimeOrFence = -1; sp presentFence; sp previousReleaseFence; uint32_t transformHint = 0; diff --git a/libs/gui/include/gui/test/CallbackUtils.h b/libs/gui/include/gui/test/CallbackUtils.h index 64032087eb..62d1496ccd 100644 --- a/libs/gui/include/gui/test/CallbackUtils.h +++ b/libs/gui/include/gui/test/CallbackUtils.h @@ -134,12 +134,15 @@ private: void verifySurfaceControlStats(const SurfaceControlStats& surfaceControlStats, nsecs_t latchTime) const { - const auto& [surfaceControl, latch, acquireTime, presentFence, previousReleaseFence, - transformHint, frameEvents] = surfaceControlStats; + const auto& [surfaceControl, latch, acquireTimeOrFence, presentFence, + previousReleaseFence, transformHint, frameEvents] = surfaceControlStats; - ASSERT_EQ(acquireTime > 0, mBufferResult == ExpectedResult::Buffer::ACQUIRED) + ASSERT_TRUE(std::holds_alternative(acquireTimeOrFence)); + ASSERT_EQ(std::get(acquireTimeOrFence) > 0, + mBufferResult == ExpectedResult::Buffer::ACQUIRED) << "bad acquire time"; - ASSERT_LE(acquireTime, latchTime) << "acquire time should be <= latch time"; + ASSERT_LE(std::get(acquireTimeOrFence), latchTime) + << "acquire time should be <= latch time"; if (mPreviousBufferResult == ExpectedResult::PreviousBuffer::RELEASED) { ASSERT_NE(previousReleaseFence, nullptr) diff --git a/services/surfaceflinger/BufferStateLayer.cpp b/services/surfaceflinger/BufferStateLayer.cpp index fccd8f149a..02e444df8e 100644 --- a/services/surfaceflinger/BufferStateLayer.cpp +++ b/services/surfaceflinger/BufferStateLayer.cpp @@ -416,8 +416,14 @@ bool BufferStateLayer::setBuffer(std::shared_ptr& ? bufferData.acquireFence : Fence::NO_FENCE; mDrawingState.acquireFenceTime = std::make_unique(mDrawingState.acquireFence); - // The acquire fences of BufferStateLayers have already signaled before they are set - mCallbackHandleAcquireTime = mDrawingState.acquireFenceTime->getSignalTime(); + if (mDrawingState.acquireFenceTime->getSignalTime() == Fence::SIGNAL_TIME_PENDING) { + // We latched this buffer unsiganled, so we need to pass the acquire fence + // on the callback instead of just the acquire time, since it's unknown at + // this point. + mCallbackHandleAcquireTimeOrFence = mDrawingState.acquireFence; + } else { + mCallbackHandleAcquireTimeOrFence = mDrawingState.acquireFenceTime->getSignalTime(); + } mDrawingState.modified = true; setTransactionFlags(eTransactionNeeded); @@ -527,7 +533,7 @@ bool BufferStateLayer::setTransactionCompletedListeners( // If this layer will be presented in this frame if (willPresent) { // If this transaction set an acquire fence on this layer, set its acquire time - handle->acquireTime = mCallbackHandleAcquireTime; + handle->acquireTimeOrFence = mCallbackHandleAcquireTimeOrFence; handle->frameNumber = mDrawingState.frameNumber; // Store so latched time and release fence can be set @@ -540,7 +546,7 @@ bool BufferStateLayer::setTransactionCompletedListeners( } mReleasePreviousBuffer = false; - mCallbackHandleAcquireTime = -1; + mCallbackHandleAcquireTimeOrFence = -1; return willPresent; } diff --git a/services/surfaceflinger/BufferStateLayer.h b/services/surfaceflinger/BufferStateLayer.h index 42562126ae..669eaadfec 100644 --- a/services/surfaceflinger/BufferStateLayer.h +++ b/services/surfaceflinger/BufferStateLayer.h @@ -145,7 +145,7 @@ private: // Stores the last set acquire fence signal time used to populate the callback handle's acquire // time. - nsecs_t mCallbackHandleAcquireTime = -1; + std::variant> mCallbackHandleAcquireTimeOrFence = -1; std::deque> mPendingJankClassifications; // An upper bound on the number of SurfaceFrames in the pending classifications deque. diff --git a/services/surfaceflinger/TransactionCallbackInvoker.cpp b/services/surfaceflinger/TransactionCallbackInvoker.cpp index b705d9cf7b..e1f348fdba 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.cpp +++ b/services/surfaceflinger/TransactionCallbackInvoker.cpp @@ -167,7 +167,7 @@ status_t TransactionCallbackInvoker::addCallbackHandle(const sp& handle->gpuCompositionDoneFence->getSnapshot().fence, handle->compositorTiming, handle->refreshStartTime, handle->dequeueReadyTime); - transactionStats->surfaceStats.emplace_back(surfaceControl, handle->acquireTime, + transactionStats->surfaceStats.emplace_back(surfaceControl, handle->acquireTimeOrFence, handle->previousReleaseFence, handle->transformHint, handle->currentMaxAcquiredBufferCount, diff --git a/services/surfaceflinger/TransactionCallbackInvoker.h b/services/surfaceflinger/TransactionCallbackInvoker.h index 5ef54757d7..a68cd87313 100644 --- a/services/surfaceflinger/TransactionCallbackInvoker.h +++ b/services/surfaceflinger/TransactionCallbackInvoker.h @@ -47,7 +47,7 @@ public: std::string name; sp previousReleaseFence; std::vector> previousReleaseFences; - nsecs_t acquireTime = -1; + std::variant> acquireTimeOrFence = -1; nsecs_t latchTime = -1; uint32_t transformHint = 0; uint32_t currentMaxAcquiredBufferCount = 0; -- cgit v1.2.3-59-g8ed1b