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

Commit 898c4c91 authored by Mathias Agopian's avatar Mathias Agopian
Browse files

fix the threading issue for setBuffercount() 

this change introduces R/W locks in the right places.
on the server-side, it guarantees that setBufferCount()
is synchronized with "retire" and "resize".
on the client-side, it guarantees that setBufferCount()
is synchronized with "dequeue", "lockbuffer" and "queue"
parent 66c46a6b

include/private/surfaceflinger/SharedBufferStack.h

+29 −14
Original line number Diff line number Diff line
@@ -55,12 +55,6 @@ namespace android { 
 * 

 */



// When changing these values, the COMPILE_TIME_ASSERT at the end of this

// file need to be updated.

const unsigned int NUM_LAYERS_MAX  = 31;

const unsigned int NUM_BUFFER_MAX  = 16;

const unsigned int NUM_DISPLAY_MAX = 4;



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



class Region;
@@ -82,6 +76,12 @@ class SharedBufferStack 
    friend class SharedBufferServer;



public:

    // When changing these values, the COMPILE_TIME_ASSERT at the end of this

    // file need to be updated.

    static const unsigned int NUM_LAYERS_MAX  = 31;

    static const unsigned int NUM_BUFFER_MAX  = 16;

    static const unsigned int NUM_DISPLAY_MAX = 4;



    struct Statistics { // 4 longs

        typedef int32_t usecs_t;

        usecs_t  totalTime;
@@ -147,7 +147,7 @@ private: 
    // FIXME: this should be replaced by a lock-less primitive

    Mutex lock;

    Condition cv;

    SharedBufferStack surfaces[ NUM_LAYERS_MAX ];

    SharedBufferStack surfaces[ SharedBufferStack::NUM_LAYERS_MAX ];

};



// ============================================================================
@@ -155,7 +155,7 @@ private: 
class SharedBufferBase

{

public:

    SharedBufferBase(SharedClient* sharedClient, int surface, int num,

    SharedBufferBase(SharedClient* sharedClient, int surface,

            int32_t identity);

    ~SharedBufferBase();

    uint32_t getIdentity();
@@ -166,9 +166,7 @@ public: 
protected:

    SharedClient* const mSharedClient;

    SharedBufferStack* const mSharedStack;

    int mNumBuffers;

    const int mIdentity;

    int32_t computeTail() const;



    friend struct Update;

    friend struct QueueUpdate;
@@ -217,7 +215,15 @@ public: 
    bool needNewBuffer(int buffer) const;

    status_t setDirtyRegion(int buffer, const Region& reg);

    status_t setCrop(int buffer, const Rect& reg);

    status_t setBufferCount(int bufferCount);

    



    class SetBufferCountCallback {

        friend class SharedBufferClient;

        virtual status_t operator()(int bufferCount) const = 0;

    protected:

        virtual ~SetBufferCountCallback() { }

    };

    status_t setBufferCount(int bufferCount, const SetBufferCountCallback& ipc);



private:

    friend struct Condition;
@@ -249,11 +255,16 @@ private: 
        inline const char* name() const { return "LockCondition"; }

    };



    int32_t computeTail() const;



    mutable RWLock mLock;

    int mNumBuffers;



    int32_t tail;

    int32_t undoDequeueTail;

    int32_t queued_head;

    // statistics...

    nsecs_t mDequeueTime[NUM_BUFFER_MAX];

    nsecs_t mDequeueTime[SharedBufferStack::NUM_BUFFER_MAX];

};



// ----------------------------------------------------------------------------
@@ -287,7 +298,8 @@ private: 
        size_t mCapacity;

        uint32_t mList;

    public:

        BufferList(size_t c = NUM_BUFFER_MAX) : mCapacity(c), mList(0) { }

        BufferList(size_t c = SharedBufferStack::NUM_BUFFER_MAX)

            : mCapacity(c), mList(0) { }

        status_t add(int value);

        status_t remove(int value);
@@ -324,6 +336,9 @@ private: 
        }

    };



    // this protects mNumBuffers and mBufferList

    mutable RWLock mLock;

    int mNumBuffers;

    BufferList mBufferList;



    struct UnlockUpdate : public UpdateBase {
@@ -373,7 +388,7 @@ struct surface_flinger_cblk_t // 4KB max 
    uint8_t         connected;

    uint8_t         reserved[3];

    uint32_t        pad[7];

    display_cblk_t  displays[NUM_DISPLAY_MAX];

    display_cblk_t  displays[SharedBufferStack::NUM_DISPLAY_MAX];

};



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

libs/surfaceflinger/Barrier.h

+0 −4
Original line number Diff line number Diff line
@@ -29,10 +29,6 @@ public: 
    inline Barrier() : state(CLOSED) { }

    inline ~Barrier() { }

    void open() {

        // gcc memory barrier, this makes sure all memory writes

        // have been issued by gcc. On an SMP system we'd need a real

        // h/w barrier.

        asm volatile ("":::"memory");

        Mutex::Autolock _l(lock);

        state = OPENED;

        cv.broadcast();

libs/surfaceflinger/Layer.cpp

+84 −53
Original line number Diff line number Diff line
@@ -60,7 +60,7 @@ Layer::Layer(SurfaceFlinger* flinger, DisplayID display, 
    // no OpenGL operation is possible here, since we might not be

    // in the OpenGL thread.

    lcblk = new SharedBufferServer(

            client->ctrlblk, i, mBufferManager.getBufferCount(),

            client->ctrlblk, i, mBufferManager.getDefaultBufferCount(),

            getIdentity());



   mBufferManager.setActiveBufferIndex( lcblk->getFrontBuffer() );
@@ -68,7 +68,10 @@ Layer::Layer(SurfaceFlinger* flinger, DisplayID display, 


Layer::~Layer()

{

    destroy();

    // FIXME: must be called from the main UI thread

    EGLDisplay dpy(mFlinger->graphicPlane(0).getEGLDisplay());

    mBufferManager.destroy(dpy);



    // the actual buffers will be destroyed here

    delete lcblk;

}
@@ -81,17 +84,6 @@ void Layer::onRemoved() 
    lcblk->setStatus(NO_INIT);

}



void Layer::destroy()

{

    EGLDisplay dpy(mFlinger->graphicPlane(0).getEGLDisplay());

    mBufferManager.destroy(dpy);



    mSurface.clear();



    Mutex::Autolock _l(mLock);

    mWidth = mHeight = 0;

}



sp<LayerBaseClient::Surface> Layer::createSurface() const

{

    return mSurface;
@@ -99,9 +91,17 @@ sp<LayerBaseClient::Surface> Layer::createSurface() const 


status_t Layer::ditch()

{

    // NOTE: Called from the main UI thread



    // the layer is not on screen anymore. free as much resources as possible

    mFreezeLock.clear();

    destroy();



    EGLDisplay dpy(mFlinger->graphicPlane(0).getEGLDisplay());

    mBufferManager.destroy(dpy);

    mSurface.clear();



    Mutex::Autolock _l(mLock);

    mWidth = mHeight = 0;

    return NO_ERROR;

}
@@ -211,7 +211,7 @@ void Layer::onDraw(const Region& clip) const 


status_t Layer::setBufferCount(int bufferCount)

{

    // this ensures our client doesn't go away while we're accessing

    // Ensures our client doesn't go away while we're accessing

    // the shared area.

    sp<Client> ourClient(client.promote());

    if (ourClient == 0) {
@@ -219,15 +219,10 @@ status_t Layer::setBufferCount(int bufferCount) 
        return DEAD_OBJECT;

    }



    status_t err;



    // FIXME: resize() below is NOT thread-safe, we need to synchronize

    // the users of lcblk in our process (ie: retire), and we assume the

    // client is not mucking with the SharedStack, which is only enforced

    // by construction, therefore we need to protect ourselves against

    // buggy and malicious client (as always)



    err = lcblk->resize(bufferCount);

    // NOTE: lcblk->resize() is protected by an internal lock

    status_t err = lcblk->resize(bufferCount);

    if (err == NO_ERROR)

        mBufferManager.resize(bufferCount);



    return err;

}
@@ -544,12 +539,20 @@ void Layer::dump(String8& result, char* buffer, size_t SIZE) const 
// ---------------------------------------------------------------------------



Layer::BufferManager::BufferManager(TextureManager& tm)

    : mTextureManager(tm), mActiveBuffer(0), mFailover(false)

    : mNumBuffers(NUM_BUFFERS), mTextureManager(tm),

      mActiveBuffer(0), mFailover(false)

{

}



size_t Layer::BufferManager::getBufferCount() const {

    return NUM_BUFFERS;

Layer::BufferManager::~BufferManager()

{

}



status_t Layer::BufferManager::resize(size_t size)

{

    Mutex::Autolock _l(mLock);

    mNumBuffers = size;

    return NO_ERROR;

}



// only for debugging
@@ -568,51 +571,55 @@ size_t Layer::BufferManager::getActiveBufferIndex() const { 
}



Texture Layer::BufferManager::getActiveTexture() const {

    return mFailover ? mFailoverTexture : mBufferData[mActiveBuffer].texture;

    Texture res;

    if (mFailover) {

        res = mFailoverTexture;

    } else {

        static_cast<Image&>(res) = mBufferData[mActiveBuffer].texture;

    }

    return res;

}



sp<GraphicBuffer> Layer::BufferManager::getActiveBuffer() const {

    const size_t activeBuffer = mActiveBuffer;

    BufferData const * const buffers = mBufferData;

    Mutex::Autolock _l(mLock);

    return mBufferData[mActiveBuffer].buffer;

    return buffers[activeBuffer].buffer;

}



sp<GraphicBuffer> Layer::BufferManager::detachBuffer(size_t index)

{

    BufferData* const buffers = mBufferData;

    sp<GraphicBuffer> buffer;

    Mutex::Autolock _l(mLock);

    buffer = mBufferData[index].buffer;

    mBufferData[index].buffer = 0;

    buffer = buffers[index].buffer;

    buffers[index].buffer = 0;

    return buffer;

}



status_t Layer::BufferManager::attachBuffer(size_t index,

        const sp<GraphicBuffer>& buffer)

{

    BufferData* const buffers = mBufferData;

    Mutex::Autolock _l(mLock);

    mBufferData[index].buffer = buffer;

    mBufferData[index].texture.dirty = true;

    return NO_ERROR;

}



status_t Layer::BufferManager::destroyTexture(Texture* tex, EGLDisplay dpy)

{

    if (tex->name != -1U) {

        glDeleteTextures(1, &tex->name);

        tex->name = -1U;

    }

    if (tex->image != EGL_NO_IMAGE_KHR) {

        eglDestroyImageKHR(dpy, tex->image);

        tex->image = EGL_NO_IMAGE_KHR;

    }

    buffers[index].buffer = buffer;

    buffers[index].texture.dirty = true;

    return NO_ERROR;

}



status_t Layer::BufferManager::destroy(EGLDisplay dpy)

{

    BufferData* const buffers = mBufferData;

    size_t num;

    { // scope for the lock

        Mutex::Autolock _l(mLock);

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

        destroyTexture(&mBufferData[i].texture, dpy);

        mBufferData[i].buffer = 0;

        num = mNumBuffers;

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

            buffers[i].buffer = 0;

        }

    }

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

        destroyTexture(&buffers[i].texture, dpy);

    }

    destroyTexture(&mFailoverTexture, dpy);

    return NO_ERROR;
@@ -622,7 +629,7 @@ status_t Layer::BufferManager::initEglImage(EGLDisplay dpy, 
        const sp<GraphicBuffer>& buffer)

{

    size_t index = mActiveBuffer;

    Texture& texture(mBufferData[index].texture);

    Image& texture(mBufferData[index].texture);

    status_t err = mTextureManager.initEglImage(&texture, dpy, buffer);

    // if EGLImage fails, we switch to regular texture mode, and we

    // free all resources associated with using EGLImages.
@@ -631,7 +638,8 @@ status_t Layer::BufferManager::initEglImage(EGLDisplay dpy, 
        destroyTexture(&mFailoverTexture, dpy);

    } else {

        mFailover = true;

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

        const size_t num = mNumBuffers;

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

            destroyTexture(&mBufferData[i].texture, dpy);

        }

    }
@@ -644,6 +652,19 @@ status_t Layer::BufferManager::loadTexture( 
    return mTextureManager.loadTexture(&mFailoverTexture, dirty, t);

}



status_t Layer::BufferManager::destroyTexture(Image* tex, EGLDisplay dpy)

{

    if (tex->name != -1U) {

        glDeleteTextures(1, &tex->name);

        tex->name = -1U;

    }

    if (tex->image != EGL_NO_IMAGE_KHR) {

        eglDestroyImageKHR(dpy, tex->image);

        tex->image = EGL_NO_IMAGE_KHR;

    }

    return NO_ERROR;

}



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



Layer::SurfaceLayer::SurfaceLayer(const sp<SurfaceFlinger>& flinger,
@@ -661,6 +682,11 @@ sp<GraphicBuffer> Layer::SurfaceLayer::requestBuffer(int index, int usage) 
    sp<GraphicBuffer> buffer;

    sp<Layer> owner(getOwner());

    if (owner != 0) {

        /*

         * requestBuffer() cannot be called from the main thread

         * as it could cause a dead-lock, since it may have to wait

         * on conditions updated my the main thread.

         */

        buffer = owner->requestBuffer(index, usage);

    }

    return buffer;
@@ -671,6 +697,11 @@ status_t Layer::SurfaceLayer::setBufferCount(int bufferCount) 
    status_t err = DEAD_OBJECT;

    sp<Layer> owner(getOwner());

    if (owner != 0) {

        /*

         * setBufferCount() cannot be called from the main thread

         * as it could cause a dead-lock, since it may have to wait

         * on conditions updated my the main thread.

         */

        err = owner->setBufferCount(bufferCount);

    }

    return err;

libs/surfaceflinger/Layer.h

+14 −6
Original line number Diff line number Diff line
@@ -90,7 +90,6 @@ private: 


    sp<GraphicBuffer> requestBuffer(int index, int usage);

    status_t setBufferCount(int bufferCount);

    void destroy();



    class SurfaceLayer : public LayerBaseClient::Surface {

    public:
@@ -120,25 +119,32 @@ private: 
                static const size_t NUM_BUFFERS = 2;

                struct BufferData {

                    sp<GraphicBuffer>   buffer;

                    Texture             texture;

                    Image               texture;

                };

                // this lock protect mBufferData[].buffer but since there

                // is very little contention, we have only one like for

                // the whole array, we also use it to protect mNumBuffers.

                mutable Mutex mLock;

                BufferData          mBufferData[NUM_BUFFERS];

                BufferData          mBufferData[SharedBufferStack::NUM_BUFFER_MAX];

                size_t              mNumBuffers;

                Texture             mFailoverTexture;

                TextureManager&     mTextureManager;

                ssize_t             mActiveBuffer;

                bool                mFailover;

                static status_t destroyTexture(Texture* tex, EGLDisplay dpy);

                static status_t destroyTexture(Image* tex, EGLDisplay dpy);



            public:

                static size_t getDefaultBufferCount() { return NUM_BUFFERS; }

                BufferManager(TextureManager& tm);



                size_t getBufferCount() const;

                ~BufferManager();



                // detach/attach buffer from/to given index

                sp<GraphicBuffer> detachBuffer(size_t index);

                status_t attachBuffer(size_t index, const sp<GraphicBuffer>& buffer);



                // resize the number of active buffers

                status_t resize(size_t size);



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

                // must be called from GL thread
@@ -170,6 +176,8 @@ private: 
            TextureManager mTextureManager;

            BufferManager mBufferManager;



            // this lock protects mWidth and mHeight which are accessed from

            // the main thread and requestBuffer's binder transaction thread.

            mutable Mutex mLock;

            uint32_t    mWidth;

            uint32_t    mHeight;

libs/surfaceflinger/LayerBlur.cpp

+3 −1
Original line number Diff line number Diff line
@@ -95,7 +95,9 @@ void LayerBlur::unlockPageFlip(const Transform& planeTransform, Region& outDirty 
                    mCacheDirty = false;

                } else {

                    if (!mAutoRefreshPending) {

                        mFlinger->signalDelayedEvent(ms2ns(500));

                        mFlinger->postMessageAsync(

                                new MessageBase(MessageQueue::INVALIDATE),

                                ms2ns(500));

                        mAutoRefreshPending = true;

                    }

                }