Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 6c6dd3b2 authored by Patrick Williams's avatar Patrick Williams
Browse files

Cache and uncache buffers in the same transaction 

This removes a race condition where clients may attempt to cache a
buffer before an associated uncache buffer request has completed,
leading to full buffer cache errors in SurfaceFlinger.

GLSurfaceViewTest#testPauseResumeWithoutDelay is failing on barbet and
test logs show frequent `ClientCache::add - cache is full` messages.
This CL fixes the "cache is full" error but does not fix the broken
test.

This reverts commit 1088bbf9.

Reason for revert: b/269141832

Change-Id: I4ba8cf18a367ae6ca94992aabd695d8984fea76f
parent 1088bbf9

libs/gui/ISurfaceComposer.cpp

+15 −8
Original line number Diff line number Diff line
@@ -63,7 +63,8 @@ public: 
                                 Vector<ComposerState>& state, const Vector<DisplayState>& displays,

                                 uint32_t flags, const sp<IBinder>& applyToken,

                                 const InputWindowCommands& commands, int64_t desiredPresentTime,

                                 bool isAutoTimestamp, const client_cache_t& uncacheBuffer,

                                 bool isAutoTimestamp,

                                 const std::vector<client_cache_t>& uncacheBuffers,

                                 bool hasListenerCallbacks,

                                 const std::vector<ListenerCallbacks>& listenerCallbacks,

                                 uint64_t transactionId) override {
@@ -87,8 +88,11 @@ public: 
        SAFE_PARCEL(commands.write, data);

        SAFE_PARCEL(data.writeInt64, desiredPresentTime);

        SAFE_PARCEL(data.writeBool, isAutoTimestamp);

        SAFE_PARCEL(data.writeUint32, static_cast<uint32_t>(uncacheBuffers.size()));

        for (const client_cache_t& uncacheBuffer : uncacheBuffers) {

            SAFE_PARCEL(data.writeStrongBinder, uncacheBuffer.token.promote());

            SAFE_PARCEL(data.writeUint64, uncacheBuffer.id);

        }

        SAFE_PARCEL(data.writeBool, hasListenerCallbacks);



        SAFE_PARCEL(data.writeVectorSize, listenerCallbacks);
@@ -158,11 +162,14 @@ status_t BnSurfaceComposer::onTransact( 
            SAFE_PARCEL(data.readInt64, &desiredPresentTime);

            SAFE_PARCEL(data.readBool, &isAutoTimestamp);



            client_cache_t uncachedBuffer;

            SAFE_PARCEL_READ_SIZE(data.readUint32, &count, data.dataSize());

            std::vector<client_cache_t> uncacheBuffers(count);

            sp<IBinder> tmpBinder;

            for (size_t i = 0; i < count; i++) {

                SAFE_PARCEL(data.readNullableStrongBinder, &tmpBinder);

            uncachedBuffer.token = tmpBinder;

            SAFE_PARCEL(data.readUint64, &uncachedBuffer.id);

                uncacheBuffers[i].token = tmpBinder;

                SAFE_PARCEL(data.readUint64, &uncacheBuffers[i].id);

            }



            bool hasListenerCallbacks = false;

            SAFE_PARCEL(data.readBool, &hasListenerCallbacks);
@@ -182,7 +189,7 @@ status_t BnSurfaceComposer::onTransact( 


            return setTransactionState(frameTimelineInfo, state, displays, stateFlags, applyToken,

                                       inputWindowCommands, desiredPresentTime, isAutoTimestamp,

                                       uncachedBuffer, hasListenerCallbacks, listenerCallbacks,

                                       uncacheBuffers, hasListenerCallbacks, listenerCallbacks,

                                       transactionId);

        }

        default: {

libs/gui/SurfaceComposerClient.cpp

+45 −17
Original line number Diff line number Diff line
@@ -565,11 +565,13 @@ public: 
        return NO_ERROR;

    }



    uint64_t cache(const sp<GraphicBuffer>& buffer) {

    uint64_t cache(const sp<GraphicBuffer>& buffer,

                   std::optional<client_cache_t>& outUncacheBuffer) {

        std::lock_guard<std::mutex> lock(mMutex);



        if (mBuffers.size() >= BUFFER_CACHE_MAX_SIZE) {

            evictLeastRecentlyUsedBuffer();

            outUncacheBuffer = findLeastRecentlyUsedBuffer();

            mBuffers.erase(outUncacheBuffer->id);

        }



        buffer->addDeathCallback(removeDeadBufferCallback, nullptr);
@@ -580,16 +582,13 @@ public: 


    void uncache(uint64_t cacheId) {

        std::lock_guard<std::mutex> lock(mMutex);

        uncacheLocked(cacheId);

    }



    void uncacheLocked(uint64_t cacheId) REQUIRES(mMutex) {

        mBuffers.erase(cacheId);

        if (mBuffers.erase(cacheId)) {

            SurfaceComposerClient::doUncacheBufferTransaction(cacheId);

        }

    }



private:

    void evictLeastRecentlyUsedBuffer() REQUIRES(mMutex) {

    client_cache_t findLeastRecentlyUsedBuffer() REQUIRES(mMutex) {

        auto itr = mBuffers.begin();

        uint64_t minCounter = itr->second;

        auto minBuffer = itr;
@@ -603,7 +602,8 @@ private: 
            }

            itr++;

        }

        uncacheLocked(minBuffer->first);



        return {.token = getToken(), .id = minBuffer->first};

    }



    uint64_t getCounter() REQUIRES(mMutex) {
@@ -741,6 +741,18 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel 
    InputWindowCommands inputWindowCommands;

    inputWindowCommands.read(*parcel);



    count = static_cast<size_t>(parcel->readUint32());

    if (count > parcel->dataSize()) {

        return BAD_VALUE;

    }

    std::vector<client_cache_t> uncacheBuffers(count);

    for (size_t i = 0; i < count; i++) {

        sp<IBinder> tmpBinder;

        SAFE_PARCEL(parcel->readStrongBinder, &tmpBinder);

        uncacheBuffers[i].token = tmpBinder;

        SAFE_PARCEL(parcel->readUint64, &uncacheBuffers[i].id);

    }



    // Parsing was successful. Update the object.

    mId = transactionId;

    mTransactionNestCount = transactionNestCount;
@@ -755,6 +767,7 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel 
    mComposerStates = composerStates;

    mInputWindowCommands = inputWindowCommands;

    mApplyToken = applyToken;

    mUncacheBuffers = std::move(uncacheBuffers);

    return NO_ERROR;

}
@@ -806,6 +819,13 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const 
    }



    mInputWindowCommands.write(*parcel);



    SAFE_PARCEL(parcel->writeUint32, static_cast<uint32_t>(mUncacheBuffers.size()));

    for (const client_cache_t& uncacheBuffer : mUncacheBuffers) {

        SAFE_PARCEL(parcel->writeStrongBinder, uncacheBuffer.token.promote());

        SAFE_PARCEL(parcel->writeUint64, uncacheBuffer.id);

    }



    return NO_ERROR;

}
@@ -873,6 +893,10 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Tr 
        }

    }



    for (const auto& cacheId : other.mUncacheBuffers) {

        mUncacheBuffers.push_back(cacheId);

    }



    mInputWindowCommands.merge(other.mInputWindowCommands);



    mMayContainBuffer |= other.mMayContainBuffer;
@@ -891,6 +915,7 @@ void SurfaceComposerClient::Transaction::clear() { 
    mDisplayStates.clear();

    mListenerCallbacks.clear();

    mInputWindowCommands.clear();

    mUncacheBuffers.clear();

    mMayContainBuffer = false;

    mTransactionNestCount = 0;

    mAnimation = false;
@@ -913,10 +938,10 @@ void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { 
    uncacheBuffer.token = BufferCache::getInstance().getToken();

    uncacheBuffer.id = cacheId;

    Vector<ComposerState> composerStates;

    status_t status =

            sf->setTransactionState(FrameTimelineInfo{}, composerStates, {},

                                    ISurfaceComposer::eOneWay, Transaction::getDefaultApplyToken(),

                                    {}, systemTime(), true, uncacheBuffer, false, {}, generateId());

    status_t status = sf->setTransactionState(FrameTimelineInfo{}, composerStates, {},

                                              ISurfaceComposer::eOneWay,

                                              Transaction::getDefaultApplyToken(), {}, systemTime(),

                                              true, {uncacheBuffer}, false, {}, generateId());

    if (status != NO_ERROR) {

        ALOGE_AND_TRACE("SurfaceComposerClient::doUncacheBufferTransaction - %s",

                        strerror(-status));
@@ -954,7 +979,11 @@ void SurfaceComposerClient::Transaction::cacheBuffers() { 
            s->bufferData->buffer = nullptr;

        } else {

            // Cache-miss. Include the buffer and send the new cacheId.

            cacheId = BufferCache::getInstance().cache(s->bufferData->buffer);

            std::optional<client_cache_t> uncacheBuffer;

            cacheId = BufferCache::getInstance().cache(s->bufferData->buffer, uncacheBuffer);

            if (uncacheBuffer) {

                mUncacheBuffers.push_back(*uncacheBuffer);

            }

        }

        s->bufferData->flags |= BufferData::BufferDataChange::cachedBufferChanged;

        s->bufferData->cachedBuffer.token = BufferCache::getInstance().getToken();
@@ -1087,8 +1116,7 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous, bool oneWay 
    sp<ISurfaceComposer> sf(ComposerService::getComposerService());

    sf->setTransactionState(mFrameTimelineInfo, composerStates, displayStates, flags, applyToken,

                            mInputWindowCommands, mDesiredPresentTime, mIsAutoTimestamp,

                            {} /*uncacheBuffer - only set in doUncacheBufferTransaction*/,

                            hasListenerCallbacks, listenerCallbacks, mId);

                            mUncacheBuffers, hasListenerCallbacks, listenerCallbacks, mId);

    mId = generateId();



    // Clear the current states and flags

libs/gui/include/gui/ISurfaceComposer.h

+3 −2
Original line number Diff line number Diff line
@@ -113,8 +113,9 @@ public: 
            const FrameTimelineInfo& frameTimelineInfo, Vector<ComposerState>& state,

            const Vector<DisplayState>& displays, uint32_t flags, const sp<IBinder>& applyToken,

            const InputWindowCommands& inputWindowCommands, int64_t desiredPresentTime,

            bool isAutoTimestamp, const client_cache_t& uncacheBuffer, bool hasListenerCallbacks,

            const std::vector<ListenerCallbacks>& listenerCallbacks, uint64_t transactionId) = 0;

            bool isAutoTimestamp, const std::vector<client_cache_t>& uncacheBuffer,

            bool hasListenerCallbacks, const std::vector<ListenerCallbacks>& listenerCallbacks,

            uint64_t transactionId) = 0;

};



// ----------------------------------------------------------------------------

libs/gui/include/gui/SurfaceComposerClient.h

+1 −0
Original line number Diff line number Diff line
@@ -402,6 +402,7 @@ public: 
        SortedVector<DisplayState> mDisplayStates;

        std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>

                mListenerCallbacks;

        std::vector<client_cache_t> mUncacheBuffers;



        uint64_t mId;

libs/gui/tests/Surface_test.cpp

+1 −1
Original line number Diff line number Diff line
@@ -701,7 +701,7 @@ public: 
                                 const sp<IBinder>& /*applyToken*/,

                                 const InputWindowCommands& /*inputWindowCommands*/,

                                 int64_t /*desiredPresentTime*/, bool /*isAutoTimestamp*/,

                                 const client_cache_t& /*cachedBuffer*/,

                                 const std::vector<client_cache_t>& /*cachedBuffer*/,

                                 bool /*hasListenerCallbacks*/,

                                 const std::vector<ListenerCallbacks>& /*listenerCallbacks*/,

                                 uint64_t /*transactionId*/) override {