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

Commit c604993d authored by Jim Shargo's avatar Jim Shargo
Browse files

Surface: don't hold mMutex when queuing buffers 

This is the behavior of the underlying IGBP, but can cause deadlocks in
certain situations dependend on by some clients.

BYPASS_IGBP_IGBC_API_REASON=warren buffers

Bug: 340933794
Flag: com.android.graphics.libgui.flags.wb_platform_api_improvements
Test: new test in `atest libgui_test`
Change-Id: I5a02e6e12980d211e70ae4db8cbd2344e323d613
parent 0f499e82

libs/gui/Surface.cpp

+113 −0
Original line number Diff line number Diff line
@@ -1168,6 +1168,117 @@ void Surface::onBufferQueuedLocked(int slot, sp<Fence> fence, 
    }

}



#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(WB_PLATFORM_API_IMPROVEMENTS)



int Surface::queueBuffer(android_native_buffer_t* buffer, int fenceFd) {

    ATRACE_CALL();

    ALOGV("Surface::queueBuffer");



    IGraphicBufferProducer::QueueBufferOutput output;

    IGraphicBufferProducer::QueueBufferInput input;

    int slot;

    sp<Fence> fence;

    {

        Mutex::Autolock lock(mMutex);



        slot = getSlotFromBufferLocked(buffer);

        if (slot < 0) {

            if (fenceFd >= 0) {

                close(fenceFd);

            }

            return slot;

        }

        if (mSharedBufferSlot == slot && mSharedBufferHasBeenQueued) {

            if (fenceFd >= 0) {

                close(fenceFd);

            }

            return OK;

        }



        getQueueBufferInputLocked(buffer, fenceFd, mTimestamp, &input);

        applyGrallocMetadataLocked(buffer, input);

        fence = input.fence;

    }

    nsecs_t now = systemTime();

    // Drop the lock temporarily while we touch the underlying producer. In the case of a local

    // BufferQueue, the following should be allowable:

    //

    //    Surface::queueBuffer

    // -> IConsumerListener::onFrameAvailable callback triggers automatically

    // ->   implementation calls IGraphicBufferConsumer::acquire/release immediately

    // -> SurfaceListener::onBufferRelesed callback triggers automatically

    // ->   implementation calls Surface::dequeueBuffer

    status_t err = mGraphicBufferProducer->queueBuffer(slot, input, &output);

    {

        Mutex::Autolock lock(mMutex);



        mLastQueueDuration = systemTime() - now;

        if (err != OK) {

            ALOGE("queueBuffer: error queuing buffer, %d", err);

        }



        onBufferQueuedLocked(slot, fence, output);

    }



    return err;

}



int Surface::queueBuffers(const std::vector<BatchQueuedBuffer>& buffers) {

    ATRACE_CALL();

    ALOGV("Surface::queueBuffers");



    size_t numBuffers = buffers.size();

    std::vector<IGraphicBufferProducer::QueueBufferInput> queueBufferInputs(numBuffers);

    std::vector<IGraphicBufferProducer::QueueBufferOutput> queueBufferOutputs;

    std::vector<int> bufferSlots(numBuffers, -1);

    std::vector<sp<Fence>> bufferFences(numBuffers);



    int err;

    {

        Mutex::Autolock lock(mMutex);



        if (mSharedBufferMode) {

            ALOGE("%s: batched operation is not supported in shared buffer mode", __FUNCTION__);

            return INVALID_OPERATION;

        }



        for (size_t batchIdx = 0; batchIdx < numBuffers; batchIdx++) {

            int i = getSlotFromBufferLocked(buffers[batchIdx].buffer);

            if (i < 0) {

                if (buffers[batchIdx].fenceFd >= 0) {

                    close(buffers[batchIdx].fenceFd);

                }

                return i;

            }

            bufferSlots[batchIdx] = i;



            IGraphicBufferProducer::QueueBufferInput input;

            getQueueBufferInputLocked(buffers[batchIdx].buffer, buffers[batchIdx].fenceFd,

                                      buffers[batchIdx].timestamp, &input);

            bufferFences[batchIdx] = input.fence;

            queueBufferInputs[batchIdx] = input;

        }

    }

    nsecs_t now = systemTime();

    err = mGraphicBufferProducer->queueBuffers(queueBufferInputs, &queueBufferOutputs);

    {

        Mutex::Autolock lock(mMutex);

        mLastQueueDuration = systemTime() - now;

        if (err != OK) {

            ALOGE("%s: error queuing buffer, %d", __FUNCTION__, err);

        }



        for (size_t batchIdx = 0; batchIdx < numBuffers; batchIdx++) {

            onBufferQueuedLocked(bufferSlots[batchIdx], bufferFences[batchIdx],

                                 queueBufferOutputs[batchIdx]);

        }

    }



    return err;

}



#else



int Surface::queueBuffer(android_native_buffer_t* buffer, int fenceFd) {

    ATRACE_CALL();

    ALOGV("Surface::queueBuffer");
@@ -1255,6 +1366,8 @@ int Surface::queueBuffers(const std::vector<BatchQueuedBuffer>& buffers) { 
    return err;

}



#endif // COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(WB_PLATFORM_API_IMPROVEMENTS)



void Surface::querySupportedTimestampsLocked() const {

    // mMutex must be locked when calling this method.

libs/gui/tests/Surface_test.cpp

+76 −0
Original line number Diff line number Diff line
@@ -23,12 +23,17 @@ 
#include <android/gui/IDisplayEventConnection.h>

#include <android/gui/ISurfaceComposer.h>

#include <android/hardware/configstore/1.0/ISurfaceFlingerConfigs.h>

#include <android/hardware_buffer.h>

#include <binder/ProcessState.h>

#include <com_android_graphics_libgui_flags.h>

#include <configstore/Utils.h>

#include <gui/AidlStatusUtil.h>

#include <gui/BufferItemConsumer.h>

#include <gui/BufferQueue.h>

#include <gui/CpuConsumer.h>

#include <gui/IConsumerListener.h>

#include <gui/IGraphicBufferConsumer.h>

#include <gui/IGraphicBufferProducer.h>

#include <gui/ISurfaceComposer.h>

#include <gui/Surface.h>

#include <gui/SurfaceComposerClient.h>
@@ -36,6 +41,7 @@ 
#include <private/gui/ComposerService.h>

#include <private/gui/ComposerServiceAIDL.h>

#include <sys/types.h>

#include <system/window.h>

#include <ui/BufferQueueDefs.h>

#include <ui/DisplayMode.h>

#include <ui/GraphicBuffer.h>
@@ -2312,6 +2318,76 @@ TEST_F(SurfaceTest, AllowAllocation) { 
    EXPECT_EQ(OK, surface->allowAllocation(true));

    EXPECT_EQ(OK, surface->dequeueBuffer(&buffer, &fence));

}



TEST_F(SurfaceTest, QueueAcquireReleaseDequeue_CalledInStack_DoesNotDeadlock) {

    class DequeuingSurfaceListener : public SurfaceListener {

    public:

        DequeuingSurfaceListener(const wp<Surface>& surface) : mSurface(surface) {}



        virtual void onBufferReleased() override {

            sp<Surface> surface = mSurface.promote();

            ASSERT_NE(nullptr, surface);

            EXPECT_EQ(OK, surface->dequeueBuffer(&mBuffer, &mFence));

        }



        virtual bool needsReleaseNotify() override { return true; }

        virtual void onBuffersDiscarded(const std::vector<sp<GraphicBuffer>>&) override {}

        virtual void onBufferDetached(int) override {}



        sp<GraphicBuffer> mBuffer;

        sp<Fence> mFence;



    private:

        wp<Surface> mSurface;

    };



    class ImmediateReleaseConsumerListener : public BufferItemConsumer::FrameAvailableListener {

    public:

        ImmediateReleaseConsumerListener(const wp<BufferItemConsumer>& consumer)

              : mConsumer(consumer) {}



        virtual void onFrameAvailable(const BufferItem&) override {

            sp<BufferItemConsumer> consumer = mConsumer.promote();

            ASSERT_NE(nullptr, consumer);



            mCalls += 1;



            BufferItem buffer;

            EXPECT_EQ(OK, consumer->acquireBuffer(&buffer, 0));

            EXPECT_EQ(OK, consumer->releaseBuffer(buffer));

        }



        size_t mCalls = 0;



    private:

        wp<BufferItemConsumer> mConsumer;

    };



    sp<IGraphicBufferProducer> bqProducer;

    sp<IGraphicBufferConsumer> bqConsumer;

    BufferQueue::createBufferQueue(&bqProducer, &bqConsumer);



    sp<BufferItemConsumer> consumer = sp<BufferItemConsumer>::make(bqConsumer, 3);

    sp<Surface> surface = sp<Surface>::make(bqProducer);

    sp<ImmediateReleaseConsumerListener> consumerListener =

            sp<ImmediateReleaseConsumerListener>::make(consumer);

    consumer->setFrameAvailableListener(consumerListener);



    sp<DequeuingSurfaceListener> surfaceListener = sp<DequeuingSurfaceListener>::make(surface);

    EXPECT_EQ(OK, surface->connect(NATIVE_WINDOW_API_CPU, false, surfaceListener));



    EXPECT_EQ(OK, surface->setMaxDequeuedBufferCount(2));



    sp<GraphicBuffer> buffer;

    sp<Fence> fence;

    EXPECT_EQ(OK, surface->dequeueBuffer(&buffer, &fence));

    EXPECT_EQ(OK, surface->queueBuffer(buffer, fence));



    EXPECT_EQ(1u, consumerListener->mCalls);

    EXPECT_NE(nullptr, surfaceListener->mBuffer);



    EXPECT_EQ(OK, surface->disconnect(NATIVE_WINDOW_API_CPU));

}

#endif // COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(WB_PLATFORM_API_IMPROVEMENTS)



} // namespace android