From cac353808ec1048333d7fd2f3d596fb4db567aa3 Mon Sep 17 00:00:00 2001 From: Dan Stoza Date: Wed, 27 Jan 2016 12:21:06 -0800 Subject: [PATCH] SF: Modify transaction/buffer sync logic Reorganizes the transaction/buffer synchronization logic to only consider the head of the incoming BufferQueue instead of all buffers, and fixes some potential race conditions. Bug: 25951593 Change-Id: Ib2b08e7be571eb8bbd8c364fb85acf0e046b439c --- services/surfaceflinger/Layer.cpp | 172 +++++++++++++++++++++----------------- services/surfaceflinger/Layer.h | 22 ++--- 2 files changed, 109 insertions(+), 85 deletions(-) diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index d4847080c6..d39075f825 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -73,6 +73,7 @@ Layer::Layer(SurfaceFlinger* flinger, const sp& client, mCurrentTransform(0), mCurrentScalingMode(NATIVE_WINDOW_SCALING_MODE_FREEZE), mCurrentOpacity(true), + mCurrentFrameNumber(0), mRefreshPending(false), mFrameLatencyNeeded(false), mFiltering(false), @@ -147,6 +148,9 @@ void Layer::onFirstRef() { } Layer::~Layer() { + for (auto& point : mRemoteSyncPoints) { + point->setTransactionApplied(); + } mFlinger->deleteTextureAsync(mTextureName); mFrameTracker.logAndResetStats(mName); } @@ -163,20 +167,6 @@ void Layer::onLayerDisplayed(const sp& /* hw */, } } -void Layer::markSyncPointsAvailable(const BufferItem& item) { - auto pointIter = mLocalSyncPoints.begin(); - while (pointIter != mLocalSyncPoints.end()) { - if ((*pointIter)->getFrameNumber() == item.mFrameNumber) { - auto syncPoint = *pointIter; - pointIter = mLocalSyncPoints.erase(pointIter); - Mutex::Autolock lock(mAvailableFrameMutex); - mAvailableFrames.push_back(std::move(syncPoint)); - } else { - ++pointIter; - } - } -} - void Layer::onFrameAvailable(const BufferItem& item) { // Add this buffer from our internal queue tracker { // Autolock scope @@ -205,8 +195,6 @@ void Layer::onFrameAvailable(const BufferItem& item) { mQueueItemCondition.broadcast(); } - markSyncPointsAvailable(item); - mFlinger->signalLayerUpdate(); } @@ -233,8 +221,6 @@ void Layer::onFrameReplaced(const BufferItem& item) { mLastFrameNumberReceived = item.mFrameNumber; mQueueItemCondition.broadcast(); } - - markSyncPointsAvailable(item); } void Layer::onSidebandStreamChanged() { @@ -803,22 +789,25 @@ uint32_t Layer::getProducerStickyTransform() const { return static_cast(producerStickyTransform); } -void Layer::addSyncPoint(std::shared_ptr point) { - uint64_t headFrameNumber = 0; - { - Mutex::Autolock lock(mQueueItemLock); - if (!mQueueItems.empty()) { - headFrameNumber = mQueueItems[0].mFrameNumber; - } else { - headFrameNumber = mLastFrameNumberReceived; - } +uint64_t Layer::getHeadFrameNumber() const { + Mutex::Autolock lock(mQueueItemLock); + if (!mQueueItems.empty()) { + return mQueueItems[0].mFrameNumber; + } else { + return mCurrentFrameNumber; } +} - if (point->getFrameNumber() <= headFrameNumber) { - point->setFrameAvailable(); - } else { - mLocalSyncPoints.push_back(std::move(point)); +bool Layer::addSyncPoint(const std::shared_ptr& point) { + if (point->getFrameNumber() <= mCurrentFrameNumber) { + // Don't bother with a SyncPoint, since we've already latched the + // relevant frame + return false; } + + Mutex::Autolock lock(mLocalSyncPointMutex); + mLocalSyncPoints.push_back(point); + return true; } void Layer::setFiltering(bool filtering) { @@ -940,8 +929,6 @@ void Layer::pushPendingState() { return; } - Mutex::Autolock lock(mPendingStateMutex); - // If this transaction is waiting on the receipt of a frame, generate a sync // point and send it to the remote layer. if (mCurrentState.handle != nullptr) { @@ -956,8 +943,13 @@ void Layer::pushPendingState() { } else { auto syncPoint = std::make_shared( mCurrentState.frameNumber); - handleLayer->addSyncPoint(syncPoint); - mRemoteSyncPoints.push_back(std::move(syncPoint)); + if (handleLayer->addSyncPoint(syncPoint)) { + mRemoteSyncPoints.push_back(std::move(syncPoint)); + } else { + // We already missed the frame we're supposed to synchronize + // on, so go ahead and apply the state update + mCurrentState.handle = nullptr; + } } // Wake us up to check if the frame has been received @@ -969,15 +961,13 @@ void Layer::pushPendingState() { void Layer::popPendingState() { auto oldFlags = mCurrentState.flags; mCurrentState = mPendingStates[0]; - mCurrentState.flags = (oldFlags & ~mCurrentState.mask) | + mCurrentState.flags = (oldFlags & ~mCurrentState.mask) | (mCurrentState.flags & mCurrentState.mask); mPendingStates.removeAt(0); } bool Layer::applyPendingStates() { - Mutex::Autolock lock(mPendingStateMutex); - bool stateUpdateAvailable = false; while (!mPendingStates.empty()) { if (mPendingStates[0].handle != nullptr) { @@ -991,6 +981,17 @@ bool Layer::applyPendingStates() { continue; } + if (mRemoteSyncPoints.front()->getFrameNumber() != + mPendingStates[0].frameNumber) { + ALOGE("[%s] Unexpected sync point frame number found", + mName.string()); + + // Signal our end of the sync point and then dispose of it + mRemoteSyncPoints.front()->setTransactionApplied(); + mRemoteSyncPoints.pop_front(); + continue; + } + if (mRemoteSyncPoints.front()->frameIsAvailable()) { // Apply the state update popPendingState(); @@ -1019,9 +1020,12 @@ bool Layer::applyPendingStates() { } void Layer::notifyAvailableFrames() { - Mutex::Autolock lock(mAvailableFrameMutex); - for (auto frame : mAvailableFrames) { - frame->setFrameAvailable(); + auto headFrameNumber = getHeadFrameNumber(); + Mutex::Autolock lock(mLocalSyncPointMutex); + for (auto& point : mLocalSyncPoints) { + if (headFrameNumber >= point->getFrameNumber()) { + point->setFrameAvailable(); + } } } @@ -1462,38 +1466,41 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) Reject r(mDrawingState, getCurrentState(), recomputeVisibleRegions, getProducerStickyTransform() != 0); - uint64_t maxFrameNumber = 0; - uint64_t headFrameNumber = 0; - { - Mutex::Autolock lock(mQueueItemLock); - maxFrameNumber = mLastFrameNumberReceived; - if (!mQueueItems.empty()) { - headFrameNumber = mQueueItems[0].mFrameNumber; - } - } - bool availableFramesEmpty = true; + // Check all of our local sync points to ensure that all transactions + // which need to have been applied prior to the frame which is about to + // be latched have signaled + + auto headFrameNumber = getHeadFrameNumber(); + bool matchingFramesFound = false; + bool allTransactionsApplied = true; { - Mutex::Autolock lock(mAvailableFrameMutex); - availableFramesEmpty = mAvailableFrames.empty(); - } - if (!availableFramesEmpty) { - Mutex::Autolock lock(mAvailableFrameMutex); - bool matchingFramesFound = false; - bool allTransactionsApplied = true; - for (auto& frame : mAvailableFrames) { - if (headFrameNumber != frame->getFrameNumber()) { + Mutex::Autolock lock(mLocalSyncPointMutex); + for (auto& point : mLocalSyncPoints) { + if (point->getFrameNumber() > headFrameNumber) { break; } + matchingFramesFound = true; - allTransactionsApplied &= frame->transactionIsApplied(); - } - if (matchingFramesFound && !allTransactionsApplied) { - mFlinger->signalLayerUpdate(); - return outDirtyRegion; + + if (!point->frameIsAvailable()) { + // We haven't notified the remote layer that the frame for + // this point is available yet. Notify it now, and then + // abort this attempt to latch. + point->setFrameAvailable(); + allTransactionsApplied = false; + break; + } + + allTransactionsApplied &= point->transactionIsApplied(); } } + if (matchingFramesFound && !allTransactionsApplied) { + mFlinger->signalLayerUpdate(); + return outDirtyRegion; + } + // 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 single @@ -1501,7 +1508,7 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) bool queuedBuffer = false; status_t updateResult = mSurfaceFlingerConsumer->updateTexImage(&r, mFlinger->mPrimaryDispSync, &mSingleBufferMode, &queuedBuffer, - maxFrameNumber); + mLastFrameNumberReceived); 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. @@ -1560,15 +1567,6 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) mFlinger->signalLayerUpdate(); } - if (!availableFramesEmpty) { - Mutex::Autolock lock(mAvailableFrameMutex); - auto frameNumber = mSurfaceFlingerConsumer->getFrameNumber(); - while (!mAvailableFrames.empty() && - frameNumber == mAvailableFrames.front()->getFrameNumber()) { - mAvailableFrames.pop_front(); - } - } - if (updateResult != NO_ERROR) { // something happened! recomputeVisibleRegions = true; @@ -1617,6 +1615,30 @@ Region Layer::latchBuffer(bool& recomputeVisibleRegions) recomputeVisibleRegions = true; } + mCurrentFrameNumber = mSurfaceFlingerConsumer->getFrameNumber(); + + // Remove any sync points corresponding to the buffer which was just + // latched + { + Mutex::Autolock lock(mLocalSyncPointMutex); + auto point = mLocalSyncPoints.begin(); + while (point != mLocalSyncPoints.end()) { + if (!(*point)->frameIsAvailable() || + !(*point)->transactionIsApplied()) { + // This sync point must have been added since we started + // latching. Don't drop it yet. + ++point; + continue; + } + + if ((*point)->getFrameNumber() <= mCurrentFrameNumber) { + point = mLocalSyncPoints.erase(point); + } else { + ++point; + } + } + } + // FIXME: postedRegion should be dirty & bounds Region dirtyRegion(Rect(s.active.w, s.active.h)); diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 9e3c4db434..d91e94ef67 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -356,10 +356,6 @@ private: virtual void onFrameReplaced(const BufferItem& item) override; virtual void onSidebandStreamChanged() override; - // Move frames made available by item in to a list which will - // be signalled at the beginning of the next transaction - virtual void markSyncPointsAvailable(const BufferItem& item); - void commitTransaction(); // needsLinearFiltering - true if this surface's state requires filtering @@ -413,19 +409,24 @@ private: std::atomic mTransactionIsApplied; }; + // SyncPoints which will be signaled when the correct frame is at the head + // of the queue and dropped after the frame has been latched. Protected by + // mLocalSyncPointMutex. + Mutex mLocalSyncPointMutex; std::list> mLocalSyncPoints; - // Guarded by mPendingStateMutex + // SyncPoints which will be signaled and then dropped when the transaction + // is applied std::list> mRemoteSyncPoints; - void addSyncPoint(std::shared_ptr point); + uint64_t getHeadFrameNumber() const; + + // Returns false if the relevant frame has already been latched + bool addSyncPoint(const std::shared_ptr& point); void pushPendingState(); void popPendingState(); bool applyPendingStates(); - - Mutex mAvailableFrameMutex; - std::list> mAvailableFrames; public: void notifyAvailableFrames(); private: @@ -461,6 +462,7 @@ private: uint32_t mCurrentTransform; uint32_t mCurrentScalingMode; bool mCurrentOpacity; + std::atomic mCurrentFrameNumber; bool mRefreshPending; bool mFrameLatencyNeeded; // Whether filtering is forced on or not @@ -488,7 +490,7 @@ private: mutable Mutex mQueueItemLock; Condition mQueueItemCondition; Vector mQueueItems; - uint64_t mLastFrameNumberReceived; + std::atomic mLastFrameNumberReceived; bool mUpdateTexImageFailed; // This is only modified from the main thread bool mSingleBufferMode; -- 2.11.0