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

Commit bdd0230f authored by Sungtak Lee's avatar Sungtak Lee
Browse files

C2BqBuffer: resolve 3 way deadlock 

Do not hold lock when IPC call is expected from HAL.

C2SurfaceSyncObj is shared lock between framework and HAL. HAL process
can have only one thread to handle IPC from HAL to framework.
Therefore Holding C2SurfaceSyncObj from HAL during IPC call could
trigger deadlock. The exact scenario is as follows.

Thread #1:(HAL -> framework IPC) HIDL call onInputBuffersReleased()
            calls to feedInputBufferIfAvailable(). Since this is using
            HAL IPC thread, this will block Thread #3. This is waiting
            for mOuput mutex which is held by Thread #2.
Thread #2:(framework) discardBuffer() holds mOutput mutex which blocks
            Thread #1. But this is waiting for C2SurfaceSyncObj which is
            held by Thread #3.
Thread #3:(HAL) Dtor of C2BufferQueueBlockPoolData is holding
            C2SurfaceSyncObj, therefore this will block #2. This thread
            is waiting for HIDL IPC thread to be free in order for
            'igbp->cancel()', but HIDL IPC thread is already occupied by
            Thread #1.

Bug: 246707566
Test: atest android.media.decoder.cts.AdaptivePlaybackTest
Test: atest android.media.decoder.cts.DecoderTest
Change-Id: I6a9540d3b4d03806cd40bb4f89a234a6b77758a9
Merged-In: I6a9540d3b4d03806cd40bb4f89a234a6b77758a9
parent 3ee67613

media/codec2/vndk/include/C2SurfaceSyncObj.h

+3 −2
Original line number Original line Diff line number Diff line
@@ -72,12 +72,13 @@ struct C2SyncVariables { 
    /**

    /**

     * Notify a buffer is queued. Return whether the upcoming dequeue operation

     * Notify a buffer is queued. Return whether the upcoming dequeue operation

     * is not blocked. if it's blocked and waitId is non-null, waitId is returned

     * is not blocked. if it's blocked and waitId is non-null, waitId is returned

     * to be used for waiting.

     * to be used for waiting. Notify(wake-up) waitors only when 'notify' is

     * true.

     *

     *

     * \retval false    dequeue operation is blocked now.

     * \retval false    dequeue operation is blocked now.

     * \retval true     dequeue operation is possible.

     * \retval true     dequeue operation is possible.

     */

     */

    bool notifyQueuedLocked(uint32_t *waitId = nullptr);

    bool notifyQueuedLocked(uint32_t *waitId = nullptr, bool notify = true);





    /**

    /**

     * Notify a buffer is dequeued.

     * Notify a buffer is dequeued.

media/codec2/vndk/platform/C2BqBuffer.cpp

+10 −12
Original line number Original line Diff line number Diff line
@@ -436,8 +436,8 @@ private: 
            if (status == -ETIME) {

            if (status == -ETIME) {

                // fence is not signalled yet.

                // fence is not signalled yet.

                if (syncVar) {

                if (syncVar) {

                    syncVar->lock();

                    (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                    (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                    syncVar->lock();

                    dequeueable = syncVar->notifyQueuedLocked(&waitId);

                    dequeueable = syncVar->notifyQueuedLocked(&waitId);

                    syncVar->unlock();

                    syncVar->unlock();

                    if (c2Fence) {

                    if (c2Fence) {
@@ -452,8 +452,8 @@ private: 
            if (status != android::NO_ERROR) {

            if (status != android::NO_ERROR) {

                ALOGD("buffer fence wait error %d", status);

                ALOGD("buffer fence wait error %d", status);

                if (syncVar) {

                if (syncVar) {

                    syncVar->lock();

                    (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                    (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                    syncVar->lock();

                    syncVar->notifyQueuedLocked();

                    syncVar->notifyQueuedLocked();

                    syncVar->unlock();

                    syncVar->unlock();

                    if (c2Fence) {

                    if (c2Fence) {
@@ -502,8 +502,8 @@ private: 
            } else if (status != android::NO_ERROR) {

            } else if (status != android::NO_ERROR) {

                slotBuffer.clear();

                slotBuffer.clear();

                if (syncVar) {

                if (syncVar) {

                    syncVar->lock();

                    (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                    (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                    syncVar->lock();

                    syncVar->notifyQueuedLocked();

                    syncVar->notifyQueuedLocked();

                    syncVar->unlock();

                    syncVar->unlock();

                    if (c2Fence) {

                    if (c2Fence) {
@@ -550,8 +550,8 @@ private: 
            // Block was not created. call requestBuffer# again next time.

            // Block was not created. call requestBuffer# again next time.

            slotBuffer.clear();

            slotBuffer.clear();

            if (syncVar) {

            if (syncVar) {

                syncVar->lock();

                (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                (void)mProducer->cancelBuffer(slot, hFenceWrapper.getHandle()).isOk();

                syncVar->lock();

                syncVar->notifyQueuedLocked();

                syncVar->notifyQueuedLocked();

                syncVar->unlock();

                syncVar->unlock();

                if (c2Fence) {

                if (c2Fence) {
@@ -813,11 +813,10 @@ C2BufferQueueBlockPoolData::~C2BufferQueueBlockPoolData() { 
        if (mGeneration == mCurrentGeneration && mBqId == mCurrentBqId && !mOwner.expired()) {

        if (mGeneration == mCurrentGeneration && mBqId == mCurrentBqId && !mOwner.expired()) {

            C2SyncVariables *syncVar = mSyncMem ? mSyncMem->mem() : nullptr;

            C2SyncVariables *syncVar = mSyncMem ? mSyncMem->mem() : nullptr;

            if (syncVar) {

            if (syncVar) {

                syncVar->lock();

                if (syncVar->getSyncStatusLocked() == C2SyncVariables::STATUS_ACTIVE) {

                mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();

                mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();

                    syncVar->notifyQueuedLocked();

                syncVar->lock();

                }

                syncVar->notifyQueuedLocked(nullptr,

                        syncVar->getSyncStatusLocked() == C2SyncVariables::STATUS_ACTIVE);

                syncVar->unlock();

                syncVar->unlock();

            } else {

            } else {

                mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();

                mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();
@@ -826,11 +825,10 @@ C2BufferQueueBlockPoolData::~C2BufferQueueBlockPoolData() { 
    } else if (!mOwner.expired()) {

    } else if (!mOwner.expired()) {

        C2SyncVariables *syncVar = mSyncMem ? mSyncMem->mem() : nullptr;

        C2SyncVariables *syncVar = mSyncMem ? mSyncMem->mem() : nullptr;

        if (syncVar) {

        if (syncVar) {

            syncVar->lock();

            if (syncVar->getSyncStatusLocked() != C2SyncVariables::STATUS_SWITCHING) {

            mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();

            mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();

                syncVar->notifyQueuedLocked();

            syncVar->lock();

            }

            syncVar->notifyQueuedLocked(nullptr,

                    syncVar->getSyncStatusLocked() != C2SyncVariables::STATUS_SWITCHING);

            syncVar->unlock();

            syncVar->unlock();

        } else {

        } else {

            mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();

            mIgbp->cancelBuffer(mBqSlot, hidl_handle{}).isOk();

media/codec2/vndk/platform/C2SurfaceSyncObj.cpp

+4 −2
Original line number Original line Diff line number Diff line
@@ -177,12 +177,14 @@ bool C2SyncVariables::isDequeueableLocked(uint32_t *waitId) { 
    return true;

    return true;

}

}





bool C2SyncVariables::notifyQueuedLocked(uint32_t *waitId) {

bool C2SyncVariables::notifyQueuedLocked(uint32_t *waitId, bool notify) {

    // Note. thundering herds may occur. Edge trigged signalling.

    // Note. thundering herds may occur. Edge trigged signalling.

    // But one waiter will guarantee to dequeue. others may wait again.

    // But one waiter will guarantee to dequeue. others may wait again.

    // Minimize futex syscall(trap) for the main use case(one waiter case).

    // Minimize futex syscall(trap) for the main use case(one waiter case).

    if (mMaxDequeueCount == mCurDequeueCount--) {

    if (mMaxDequeueCount == mCurDequeueCount--) {

        if (notify) {

            broadcast();

            broadcast();

        }

        return true;

        return true;

    }

    }