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

Commit 4ba0c2e2 authored by Vishnu Nair's avatar Vishnu Nair
Browse files

Blast: Use a unique id to track buffers 

When buffer queue is configured in shared buffer mode, the client
can queue the same buffer over and over again and the consumer
can acquire the same buffer repeatedly. BBQ tracks acquired buffers
by graphic buffer id and SCC tracks release buffer callbacks by
graphic buffer ids. This breaks if a buffer is reused before release.
Fix this by using graphic buffer id and framenumber to track acquired
buffers and buffer release callbacks.

Test: atest CtsDeqpTestCases:dEQP-VK.wsi.android.shared_presentable_image.scale_none.identity.inherit.demand CtsDeqpTestCases:dEQP-VK.wsi.android.colorspace#basic
Bug: 191220669

Change-Id: Ibd54df9fa6780c26cd8de769bf9e43163abbed5a
parent b3c68852

libs/gui/BLASTBufferQueue.cpp

+21 −19
Original line number Diff line number Diff line
@@ -132,8 +132,7 @@ void BLASTBufferItemConsumer::onSidebandStreamChanged() { 


BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const sp<SurfaceControl>& surface,

                                   int width, int height, int32_t format)

      : mName(name),

        mSurfaceControl(surface),

      : mSurfaceControl(surface),

        mSize(width, height),

        mRequestedSize(mSize),

        mFormat(format),
@@ -150,6 +149,7 @@ BLASTBufferQueue::BLASTBufferQueue(const std::string& name, const sp<SurfaceCont 
                                                              GraphicBuffer::USAGE_HW_TEXTURE,

                                                      1, false);

    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++;
@@ -313,24 +313,24 @@ void BLASTBufferQueue::transactionCallback(nsecs_t /*latchTime*/, const sp<Fence 
// BBQ. This is because if the BBQ is destroyed, then the buffers will be released by the client.

// So we pass in a weak pointer to the BBQ and if it still alive, then we release the buffer.

// Otherwise, this is a no-op.

static void releaseBufferCallbackThunk(wp<BLASTBufferQueue> context, uint64_t graphicBufferId,

static void releaseBufferCallbackThunk(wp<BLASTBufferQueue> context, const ReleaseCallbackId& id,

                                       const sp<Fence>& releaseFence, uint32_t transformHint,

                                       uint32_t currentMaxAcquiredBufferCount) {

    sp<BLASTBufferQueue> blastBufferQueue = context.promote();

    ALOGV("releaseBufferCallbackThunk graphicBufferId=%" PRIu64 " blastBufferQueue=%s",

          graphicBufferId, blastBufferQueue ? "alive" : "dead");

    if (blastBufferQueue) {

        blastBufferQueue->releaseBufferCallback(graphicBufferId, releaseFence, transformHint,

        blastBufferQueue->releaseBufferCallback(id, releaseFence, transformHint,

                                                currentMaxAcquiredBufferCount);

    } else {

        ALOGV("releaseBufferCallbackThunk %s blastBufferQueue is dead", id.to_string().c_str());

    }

}



void BLASTBufferQueue::releaseBufferCallback(uint64_t graphicBufferId,

void BLASTBufferQueue::releaseBufferCallback(const ReleaseCallbackId& id,

                                             const sp<Fence>& releaseFence, uint32_t transformHint,

                                             uint32_t currentMaxAcquiredBufferCount) {

    ATRACE_CALL();

    std::unique_lock _lock{mMutex};

    BQA_LOGV("releaseBufferCallback graphicBufferId=%" PRIu64, graphicBufferId);

    BQA_LOGV("releaseBufferCallback %s", id.to_string().c_str());



    if (mSurfaceControl != nullptr) {

        mTransformHint = transformHint;
@@ -343,25 +343,26 @@ void BLASTBufferQueue::releaseBufferCallback(uint64_t graphicBufferId, 
    // on a lower refresh rate than the max supported. We only do that for EGL

    // clients as others don't care about latency

    const bool isEGL = [&] {

        const auto it = mSubmitted.find(graphicBufferId);

        const auto it = mSubmitted.find(id);

        return it != mSubmitted.end() && it->second.mApi == NATIVE_WINDOW_API_EGL;

    }();



    const auto numPendingBuffersToHold =

            isEGL ? std::max(0u, mMaxAcquiredBuffers - currentMaxAcquiredBufferCount) : 0;

    mPendingRelease.emplace_back(ReleasedBuffer{graphicBufferId, releaseFence});

    mPendingRelease.emplace_back(ReleasedBuffer{id, releaseFence});



    // Release all buffers that are beyond the ones that we need to hold

    while (mPendingRelease.size() > numPendingBuffersToHold) {

        const auto releaseBuffer = mPendingRelease.front();

        mPendingRelease.pop_front();

        auto it = mSubmitted.find(releaseBuffer.bufferId);

        auto it = mSubmitted.find(releaseBuffer.callbackId);

        if (it == mSubmitted.end()) {

            BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %" PRIu64,

                     graphicBufferId);

            BQA_LOGE("ERROR: releaseBufferCallback without corresponding submitted buffer %s",

                     releaseBuffer.callbackId.to_string().c_str());

            return;

        }

        mNumAcquired--;

        BQA_LOGV("released %s", id.to_string().c_str());

        mBufferItemConsumer->releaseBuffer(it->second, releaseBuffer.releaseFence);

        mSubmitted.erase(it);

        processNextBufferLocked(false /* useNextTransaction */);
@@ -428,7 +429,9 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { 
    }



    mNumAcquired++;

    mSubmitted[buffer->getId()] = bufferItem;

    mLastAcquiredFrameNumber = bufferItem.mFrameNumber;

    ReleaseCallbackId releaseCallbackId(buffer->getId(), mLastAcquiredFrameNumber);

    mSubmitted[releaseCallbackId] = bufferItem;



    bool needsDisconnect = false;

    mBufferItemConsumer->getConnectionEvents(bufferItem.mFrameNumber, &needsDisconnect);
@@ -442,7 +445,6 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { 
    incStrong((void*)transactionCallbackThunk);



    Rect crop = computeCrop(bufferItem);

    mLastAcquiredFrameNumber = bufferItem.mFrameNumber;

    mLastBufferInfo.update(true /* hasBuffer */, bufferItem.mGraphicBuffer->getWidth(),

                           bufferItem.mGraphicBuffer->getHeight(), bufferItem.mTransform,

                           bufferItem.mScalingMode, crop);
@@ -451,7 +453,7 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { 
            std::bind(releaseBufferCallbackThunk, wp<BLASTBufferQueue>(this) /* callbackContext */,

                      std::placeholders::_1, std::placeholders::_2, std::placeholders::_3,

                      std::placeholders::_4);

    t->setBuffer(mSurfaceControl, buffer, releaseBufferCallback);

    t->setBuffer(mSurfaceControl, buffer, releaseCallbackId, releaseBufferCallback);

    t->setDataspace(mSurfaceControl, static_cast<ui::Dataspace>(bufferItem.mDataSpace));

    t->setHdrMetadata(mSurfaceControl, bufferItem.mHdrMetadata);

    t->setSurfaceDamageRegion(mSurfaceControl, bufferItem.mSurfaceDamage);
@@ -510,11 +512,11 @@ void BLASTBufferQueue::processNextBufferLocked(bool useNextTransaction) { 


    BQA_LOGV("processNextBufferLocked size=%dx%d mFrameNumber=%" PRIu64

             " applyTransaction=%s mTimestamp=%" PRId64 "%s mPendingTransactions.size=%d"

             " graphicBufferId=%" PRIu64,

             " graphicBufferId=%" PRIu64 "%s",

             mSize.width, mSize.height, bufferItem.mFrameNumber, toString(applyTransaction),

             bufferItem.mTimestamp, bufferItem.mIsAutoTimestamp ? "(auto)" : "",

             static_cast<uint32_t>(mPendingTransactions.size()),

             bufferItem.mGraphicBuffer->getId());

             static_cast<uint32_t>(mPendingTransactions.size()), bufferItem.mGraphicBuffer->getId(),

             bufferItem.mAutoRefresh ? " mAutoRefresh" : "");

}



Rect BLASTBufferQueue::computeCrop(const BufferItem& item) {

libs/gui/ITransactionCompletedListener.cpp

+19 −5
Original line number Diff line number Diff line
@@ -125,7 +125,7 @@ status_t SurfaceStats::writeToParcel(Parcel* output) const { 
    for (const auto& data : jankData) {

        SAFE_PARCEL(output->writeParcelable, data);

    }

    SAFE_PARCEL(output->writeUint64, previousBufferId);

    SAFE_PARCEL(output->writeParcelable, previousReleaseCallbackId);

    return NO_ERROR;

}
@@ -149,7 +149,7 @@ status_t SurfaceStats::readFromParcel(const Parcel* input) { 
        SAFE_PARCEL(input->readParcelable, &data);

        jankData.push_back(data);

    }

    SAFE_PARCEL(input->readUint64, &previousBufferId);

    SAFE_PARCEL(input->readParcelable, &previousReleaseCallbackId);

    return NO_ERROR;

}
@@ -253,11 +253,11 @@ public: 
                                                                  stats);

    }



    void onReleaseBuffer(uint64_t graphicBufferId, sp<Fence> releaseFence, uint32_t transformHint,

                         uint32_t currentMaxAcquiredBufferCount) override {

    void onReleaseBuffer(ReleaseCallbackId callbackId, sp<Fence> releaseFence,

                         uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount) override {

        callRemoteAsync<decltype(

                &ITransactionCompletedListener::onReleaseBuffer)>(Tag::ON_RELEASE_BUFFER,

                                                                  graphicBufferId, releaseFence,

                                                                  callbackId, releaseFence,

                                                                  transformHint,

                                                                  currentMaxAcquiredBufferCount);

    }
@@ -308,4 +308,18 @@ status_t CallbackId::readFromParcel(const Parcel* input) { 
    return NO_ERROR;

}



status_t ReleaseCallbackId::writeToParcel(Parcel* output) const {

    SAFE_PARCEL(output->writeUint64, bufferId);

    SAFE_PARCEL(output->writeUint64, framenumber);

    return NO_ERROR;

}



status_t ReleaseCallbackId::readFromParcel(const Parcel* input) {

    SAFE_PARCEL(input->readUint64, &bufferId);

    SAFE_PARCEL(input->readUint64, &framenumber);

    return NO_ERROR;

}



const ReleaseCallbackId ReleaseCallbackId::INVALID_ID = ReleaseCallbackId(0, 0);



}; // namespace android

libs/gui/SurfaceComposerClient.cpp

+25 −18
Original line number Diff line number Diff line
@@ -197,15 +197,16 @@ void TransactionCompletedListener::removeJankListener(const sp<JankDataListener> 
    }

}



void TransactionCompletedListener::setReleaseBufferCallback(uint64_t graphicBufferId,

void TransactionCompletedListener::setReleaseBufferCallback(const ReleaseCallbackId& callbackId,

                                                            ReleaseBufferCallback listener) {

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

    mReleaseBufferCallbacks[graphicBufferId] = listener;

    mReleaseBufferCallbacks[callbackId] = listener;

}



void TransactionCompletedListener::removeReleaseBufferCallback(uint64_t graphicBufferId) {

void TransactionCompletedListener::removeReleaseBufferCallback(

        const ReleaseCallbackId& callbackId) {

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

    mReleaseBufferCallbacks.erase(graphicBufferId);

    mReleaseBufferCallbacks.erase(callbackId);

}



void TransactionCompletedListener::addSurfaceStatsListener(void* context, void* cookie,
@@ -319,14 +320,15 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener 
                // and call them. This is a performance optimization when we have a transaction

                // callback and a release buffer callback happening at the same time to avoid an

                // additional ipc call from the server.

                if (surfaceStats.previousBufferId) {

                if (surfaceStats.previousReleaseCallbackId != ReleaseCallbackId::INVALID_ID) {

                    ReleaseBufferCallback callback;

                    {

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

                        callback = popReleaseBufferCallbackLocked(surfaceStats.previousBufferId);

                        callback = popReleaseBufferCallbackLocked(

                                surfaceStats.previousReleaseCallbackId);

                    }

                    if (callback) {

                        callback(surfaceStats.previousBufferId,

                        callback(surfaceStats.previousReleaseCallbackId,

                                 surfaceStats.previousReleaseFence

                                         ? surfaceStats.previousReleaseFence

                                         : Fence::NO_FENCE,
@@ -362,25 +364,26 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener 
    }

}



void TransactionCompletedListener::onReleaseBuffer(uint64_t graphicBufferId, sp<Fence> releaseFence,

                                                   uint32_t transformHint,

void TransactionCompletedListener::onReleaseBuffer(ReleaseCallbackId callbackId,

                                                   sp<Fence> releaseFence, uint32_t transformHint,

                                                   uint32_t currentMaxAcquiredBufferCount) {

    ReleaseBufferCallback callback;

    {

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

        callback = popReleaseBufferCallbackLocked(graphicBufferId);

        callback = popReleaseBufferCallbackLocked(callbackId);

    }

    if (!callback) {

        ALOGE("Could not call release buffer callback, buffer not found %" PRIu64, graphicBufferId);

        ALOGE("Could not call release buffer callback, buffer not found %s",

              callbackId.to_string().c_str());

        return;

    }

    callback(graphicBufferId, releaseFence, transformHint, currentMaxAcquiredBufferCount);

    callback(callbackId, releaseFence, transformHint, currentMaxAcquiredBufferCount);

}



ReleaseBufferCallback TransactionCompletedListener::popReleaseBufferCallbackLocked(

        uint64_t graphicBufferId) {

        const ReleaseCallbackId& callbackId) {

    ReleaseBufferCallback callback;

    auto itr = mReleaseBufferCallbacks.find(graphicBufferId);

    auto itr = mReleaseBufferCallbacks.find(callbackId);

    if (itr == mReleaseBufferCallbacks.end()) {

        return nullptr;

    }
@@ -1258,7 +1261,7 @@ SurfaceComposerClient::Transaction::setTransformToDisplayInverse(const sp<Surfac 
}



SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffer(

        const sp<SurfaceControl>& sc, const sp<GraphicBuffer>& buffer,

        const sp<SurfaceControl>& sc, const sp<GraphicBuffer>& buffer, const ReleaseCallbackId& id,

        ReleaseBufferCallback callback) {

    layer_state_t* s = getLayerState(sc);

    if (!s) {
@@ -1271,7 +1274,7 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe 
    if (mIsAutoTimestamp) {

        mDesiredPresentTime = systemTime();

    }

    setReleaseBufferCallback(s, callback);

    setReleaseBufferCallback(s, id, callback);



    registerSurfaceControlForCallback(sc);
@@ -1286,10 +1289,13 @@ void SurfaceComposerClient::Transaction::removeReleaseBufferCallback(layer_state 


    s->what &= ~static_cast<uint64_t>(layer_state_t::eReleaseBufferListenerChanged);

    s->releaseBufferListener = nullptr;

    TransactionCompletedListener::getInstance()->removeReleaseBufferCallback(s->buffer->getId());

    auto listener = TransactionCompletedListener::getInstance();

    listener->removeReleaseBufferCallback(s->releaseCallbackId);

    s->releaseCallbackId = ReleaseCallbackId::INVALID_ID;

}



void SurfaceComposerClient::Transaction::setReleaseBufferCallback(layer_state_t* s,

                                                                  const ReleaseCallbackId& id,

                                                                  ReleaseBufferCallback callback) {

    if (!callback) {

        return;
@@ -1303,8 +1309,9 @@ void SurfaceComposerClient::Transaction::setReleaseBufferCallback(layer_state_t* 


    s->what |= layer_state_t::eReleaseBufferListenerChanged;

    s->releaseBufferListener = TransactionCompletedListener::getIInstance();

    s->releaseCallbackId = id;

    auto listener = TransactionCompletedListener::getInstance();

    listener->setReleaseBufferCallback(s->buffer->getId(), callback);

    listener->setReleaseBufferCallback(id, callback);

}



SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setAcquireFence(

libs/gui/include/gui/BLASTBufferQueue.h

+4 −3
Original line number Diff line number Diff line
@@ -89,7 +89,7 @@ public: 


    void transactionCallback(nsecs_t latchTime, const sp<Fence>& presentFence,

            const std::vector<SurfaceControlStats>& stats);

    void releaseBufferCallback(uint64_t graphicBufferId, const sp<Fence>& releaseFence,

    void releaseBufferCallback(const ReleaseCallbackId& id, const sp<Fence>& releaseFence,

                               uint32_t transformHint, uint32_t currentMaxAcquiredBufferCount);

    void setNextTransaction(SurfaceComposerClient::Transaction *t);

    void mergeWithNextTransaction(SurfaceComposerClient::Transaction* t, uint64_t frameNumber);
@@ -144,13 +144,14 @@ private: 


    // 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<uint64_t /* bufferId */, BufferItem> mSubmitted GUARDED_BY(mMutex);

    std::unordered_map<ReleaseCallbackId, BufferItem, ReleaseBufferCallbackIdHash> mSubmitted

            GUARDED_BY(mMutex);



    // Keep a queue of the released buffers instead of immediately releasing

    // the buffers back to the buffer queue. This would be controlled by SF

    // setting the max acquired buffer count.

    struct ReleasedBuffer {

        uint64_t bufferId;

        ReleaseCallbackId callbackId;

        sp<Fence> releaseFence;

    };

    std::deque<ReleasedBuffer> mPendingRelease GUARDED_BY(mMutex);

libs/gui/include/gui/ITransactionCompletedListener.h

+34 −4
Original line number Diff line number Diff line
@@ -53,6 +53,36 @@ struct CallbackIdHash { 
    std::size_t operator()(const CallbackId& key) const { return std::hash<int64_t>()(key.id); }

};



class ReleaseCallbackId : public Parcelable {

public:

    static const ReleaseCallbackId INVALID_ID;



    uint64_t bufferId;

    uint64_t framenumber;

    ReleaseCallbackId() {}

    ReleaseCallbackId(uint64_t bufferId, uint64_t framenumber)

          : bufferId(bufferId), framenumber(framenumber) {}

    status_t writeToParcel(Parcel* output) const override;

    status_t readFromParcel(const Parcel* input) override;



    bool operator==(const ReleaseCallbackId& rhs) const {

        return bufferId == rhs.bufferId && framenumber == rhs.framenumber;

    }

    bool operator!=(const ReleaseCallbackId& rhs) const { return !operator==(rhs); }

    std::string to_string() const {

        if (*this == INVALID_ID) return "INVALID_ID";



        return "bufferId:" + std::to_string(bufferId) +

                " framenumber:" + std::to_string(framenumber);

    }

};



struct ReleaseBufferCallbackIdHash {

    std::size_t operator()(const ReleaseCallbackId& key) const {

        return std::hash<uint64_t>()(key.bufferId);

    }

};



class FrameEventHistoryStats : public Parcelable {

public:

    status_t writeToParcel(Parcel* output) const override;
@@ -103,7 +133,7 @@ public: 
    SurfaceStats(const sp<IBinder>& sc, nsecs_t time, const sp<Fence>& prevReleaseFence,

                 uint32_t hint, uint32_t currentMaxAcquiredBuffersCount,

                 FrameEventHistoryStats frameEventStats, std::vector<JankData> jankData,

                 uint64_t previousBufferId)

                 ReleaseCallbackId previousReleaseCallbackId)

          : surfaceControl(sc),

            acquireTime(time),

            previousReleaseFence(prevReleaseFence),
@@ -111,7 +141,7 @@ public: 
            currentMaxAcquiredBufferCount(currentMaxAcquiredBuffersCount),

            eventStats(frameEventStats),

            jankData(std::move(jankData)),

            previousBufferId(previousBufferId) {}

            previousReleaseCallbackId(previousReleaseCallbackId) {}



    sp<IBinder> surfaceControl;

    nsecs_t acquireTime = -1;
@@ -120,7 +150,7 @@ public: 
    uint32_t currentMaxAcquiredBufferCount = 0;

    FrameEventHistoryStats eventStats;

    std::vector<JankData> jankData;

    uint64_t previousBufferId;

    ReleaseCallbackId previousReleaseCallbackId;

};



class TransactionStats : public Parcelable {
@@ -161,7 +191,7 @@ public: 


    virtual void onTransactionCompleted(ListenerStats stats) = 0;



    virtual void onReleaseBuffer(uint64_t graphicBufferId, sp<Fence> releaseFence,

    virtual void onReleaseBuffer(ReleaseCallbackId callbackId, sp<Fence> releaseFence,

                                 uint32_t transformHint,

                                 uint32_t currentMaxAcquiredBufferCount) = 0;

};