From 898c4c91be8e11b6d5388c623ae80f12ac25fd27 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 18 May 2010 17:06:55 -0700 Subject: 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" --- libs/surfaceflinger_client/SharedBufferStack.cpp | 95 +++++++++++++--------- libs/surfaceflinger_client/Surface.cpp | 19 ++--- .../SurfaceComposerClient.cpp | 10 +-- .../SharedBufferStack/SharedBufferStackTest.cpp | 12 ++- 4 files changed, 79 insertions(+), 57 deletions(-) (limited to 'libs/surfaceflinger_client') diff --git a/libs/surfaceflinger_client/SharedBufferStack.cpp b/libs/surfaceflinger_client/SharedBufferStack.cpp index dab8ed8..5deeabb 100644 --- a/libs/surfaceflinger_client/SharedBufferStack.cpp +++ b/libs/surfaceflinger_client/SharedBufferStack.cpp @@ -44,7 +44,7 @@ SharedClient::~SharedClient() { // these functions are used by the clients status_t SharedClient::validate(size_t i) const { - if (uint32_t(i) >= uint32_t(NUM_LAYERS_MAX)) + if (uint32_t(i) >= uint32_t(SharedBufferStack::NUM_LAYERS_MAX)) return BAD_INDEX; return surfaces[i].status; } @@ -144,10 +144,10 @@ Region SharedBufferStack::getDirtyRegion(int buffer) const // ---------------------------------------------------------------------------- SharedBufferBase::SharedBufferBase(SharedClient* sharedClient, - int surface, int num, int32_t identity) + int surface, int32_t identity) : mSharedClient(sharedClient), mSharedStack(sharedClient->surfaces + surface), - mNumBuffers(num), mIdentity(identity) + mIdentity(identity) { } @@ -179,34 +179,16 @@ String8 SharedBufferBase::dump(char const* prefix) const char buffer[SIZE]; String8 result; SharedBufferStack& stack( *mSharedStack ); - int tail = computeTail(); snprintf(buffer, SIZE, - "%s[ head=%2d, available=%2d, queued=%2d, tail=%2d ] " + "%s[ head=%2d, available=%2d, queued=%2d ] " "reallocMask=%08x, inUse=%2d, identity=%d, status=%d", - prefix, stack.head, stack.available, stack.queued, tail, + prefix, stack.head, stack.available, stack.queued, stack.reallocMask, stack.inUse, stack.identity, stack.status); result.append(buffer); - - snprintf(buffer, SIZE, " { "); - result.append(buffer); - - for (int i=0 ; i= NUM_BUFFER_MAX) { + if (uint32_t(head) >= SharedBufferStack::NUM_BUFFER_MAX) { // if stack.head is messed up, we cannot allow the server to // crash (since stack.head is mapped on the client side) stack.status = BAD_VALUE; @@ -318,7 +300,7 @@ SharedBufferServer::RetireUpdate::RetireUpdate( } ssize_t SharedBufferServer::RetireUpdate::operator()() { int32_t head = stack.head; - if (uint32_t(head) >= NUM_BUFFER_MAX) + if (uint32_t(head) >= SharedBufferStack::NUM_BUFFER_MAX) return BAD_VALUE; // Preventively lock the current buffer before updating queued. @@ -361,14 +343,20 @@ ssize_t SharedBufferServer::StatusUpdate::operator()() { SharedBufferClient::SharedBufferClient(SharedClient* sharedClient, int surface, int num, int32_t identity) - : SharedBufferBase(sharedClient, surface, num, identity), - tail(0), undoDequeueTail(0) + : SharedBufferBase(sharedClient, surface, identity), + mNumBuffers(num), tail(0), undoDequeueTail(0) { SharedBufferStack& stack( *mSharedStack ); tail = computeTail(); queued_head = stack.head; } +int32_t SharedBufferClient::computeTail() const +{ + SharedBufferStack& stack( *mSharedStack ); + return (mNumBuffers + stack.head - stack.available + 1) % mNumBuffers; +} + ssize_t SharedBufferClient::dequeue() { SharedBufferStack& stack( *mSharedStack ); @@ -377,7 +365,9 @@ ssize_t SharedBufferClient::dequeue() LOGW("dequeue: tail=%d, head=%d, avail=%d, queued=%d", tail, stack.head, stack.available, stack.queued); } - + + RWLock::AutoRLock _rd(mLock); + const nsecs_t dequeueTime = systemTime(SYSTEM_TIME_THREAD); //LOGD("[%d] about to dequeue a buffer", @@ -407,6 +397,8 @@ ssize_t SharedBufferClient::dequeue() status_t SharedBufferClient::undoDequeue(int buf) { + RWLock::AutoRLock _rd(mLock); + // TODO: we can only undo the previous dequeue, we should // enforce that in the api UndoDequeueUpdate update(this); @@ -419,6 +411,8 @@ status_t SharedBufferClient::undoDequeue(int buf) status_t SharedBufferClient::lock(int buf) { + RWLock::AutoRLock _rd(mLock); + SharedBufferStack& stack( *mSharedStack ); LockCondition condition(this, buf); status_t err = waitForCondition(condition); @@ -427,6 +421,8 @@ status_t SharedBufferClient::lock(int buf) status_t SharedBufferClient::queue(int buf) { + RWLock::AutoRLock _rd(mLock); + SharedBufferStack& stack( *mSharedStack ); queued_head = (queued_head + 1) % mNumBuffers; @@ -460,21 +456,29 @@ status_t SharedBufferClient::setDirtyRegion(int buf, const Region& reg) return stack.setDirtyRegion(buf, reg); } -status_t SharedBufferClient::setBufferCount(int bufferCount) +status_t SharedBufferClient::setBufferCount( + int bufferCount, const SetBufferCountCallback& ipc) { SharedBufferStack& stack( *mSharedStack ); - if (uint32_t(bufferCount) >= NUM_BUFFER_MAX) + if (uint32_t(bufferCount) >= SharedBufferStack::NUM_BUFFER_MAX) return BAD_VALUE; - mNumBuffers = bufferCount; - queued_head = (stack.head + stack.queued) % mNumBuffers; - return NO_ERROR; + + RWLock::AutoWLock _wr(mLock); + + status_t err = ipc(bufferCount); + if (err == NO_ERROR) { + mNumBuffers = bufferCount; + queued_head = (stack.head + stack.queued) % mNumBuffers; + } + return err; } // ---------------------------------------------------------------------------- SharedBufferServer::SharedBufferServer(SharedClient* sharedClient, int surface, int num, int32_t identity) - : SharedBufferBase(sharedClient, surface, num, identity) + : SharedBufferBase(sharedClient, surface, identity), + mNumBuffers(num) { mSharedStack->init(identity); mSharedStack->head = num-1; @@ -490,10 +494,12 @@ SharedBufferServer::SharedBufferServer(SharedClient* sharedClient, ssize_t SharedBufferServer::retireAndLock() { + RWLock::AutoRLock _l(mLock); + RetireUpdate update(this, mNumBuffers); ssize_t buf = updateCondition( update ); if (buf >= 0) { - if (uint32_t(buf) >= NUM_BUFFER_MAX) + if (uint32_t(buf) >= SharedBufferStack::NUM_BUFFER_MAX) return BAD_VALUE; SharedBufferStack& stack( *mSharedStack ); buf = stack.index[buf]; @@ -520,6 +526,8 @@ void SharedBufferServer::setStatus(status_t status) status_t SharedBufferServer::reallocate() { + RWLock::AutoRLock _l(mLock); + SharedBufferStack& stack( *mSharedStack ); uint32_t mask = (1<= NUM_BUFFER_MAX) + if (uint32_t(newNumBuffers) >= SharedBufferStack::NUM_BUFFER_MAX) return BAD_VALUE; + RWLock::AutoWLock _l(mLock); + // for now we're not supporting shrinking const int numBuffers = mNumBuffers; if (newNumBuffers < numBuffers) @@ -569,7 +584,7 @@ status_t SharedBufferServer::resize(int newNumBuffers) // read the head, make sure it's valid int32_t head = stack.head; - if (uint32_t(head) >= NUM_BUFFER_MAX) + if (uint32_t(head) >= SharedBufferStack::NUM_BUFFER_MAX) return BAD_VALUE; int base = numBuffers; diff --git a/libs/surfaceflinger_client/Surface.cpp b/libs/surfaceflinger_client/Surface.cpp index afbeafb..4d5c0b6 100644 --- a/libs/surfaceflinger_client/Surface.cpp +++ b/libs/surfaceflinger_client/Surface.cpp @@ -678,19 +678,18 @@ int Surface::setBufferCount(int bufferCount) sp s(mSurface); if (s == 0) return NO_INIT; - // FIXME: this needs to be synchronized dequeue/queue + class SetBufferCountIPC : public SharedBufferClient::SetBufferCountCallback { + sp surface; + virtual status_t operator()(int bufferCount) const { + return surface->setBufferCount(bufferCount); + } + public: + SetBufferCountIPC(const sp& surface) : surface(surface) { } + } ipc(s); - status_t err = s->setBufferCount(bufferCount); + status_t err = mSharedBufferClient->setBufferCount(bufferCount, ipc); LOGE_IF(err, "ISurface::setBufferCount(%d) returned %s", bufferCount, strerror(-err)); - if (err == NO_ERROR) { - err = mSharedBufferClient->getStatus(); - LOGE_IF(err, "Surface (identity=%d) state = %d", mIdentity, err); - if (!err) { - // update our local copy of the buffer count - mSharedBufferClient->setBufferCount(bufferCount); - } - } return err; } diff --git a/libs/surfaceflinger_client/SurfaceComposerClient.cpp b/libs/surfaceflinger_client/SurfaceComposerClient.cpp index 85167da..9ac73d2 100644 --- a/libs/surfaceflinger_client/SurfaceComposerClient.cpp +++ b/libs/surfaceflinger_client/SurfaceComposerClient.cpp @@ -250,7 +250,7 @@ void SurfaceComposerClient::dispose() status_t SurfaceComposerClient::getDisplayInfo( DisplayID dpy, DisplayInfo* info) { - if (uint32_t(dpy)>=NUM_DISPLAY_MAX) + if (uint32_t(dpy)>=SharedBufferStack::NUM_DISPLAY_MAX) return BAD_VALUE; volatile surface_flinger_cblk_t const * cblk = get_cblk(); @@ -268,7 +268,7 @@ status_t SurfaceComposerClient::getDisplayInfo( ssize_t SurfaceComposerClient::getDisplayWidth(DisplayID dpy) { - if (uint32_t(dpy)>=NUM_DISPLAY_MAX) + if (uint32_t(dpy)>=SharedBufferStack::NUM_DISPLAY_MAX) return BAD_VALUE; volatile surface_flinger_cblk_t const * cblk = get_cblk(); volatile display_cblk_t const * dcblk = cblk->displays + dpy; @@ -277,7 +277,7 @@ ssize_t SurfaceComposerClient::getDisplayWidth(DisplayID dpy) ssize_t SurfaceComposerClient::getDisplayHeight(DisplayID dpy) { - if (uint32_t(dpy)>=NUM_DISPLAY_MAX) + if (uint32_t(dpy)>=SharedBufferStack::NUM_DISPLAY_MAX) return BAD_VALUE; volatile surface_flinger_cblk_t const * cblk = get_cblk(); volatile display_cblk_t const * dcblk = cblk->displays + dpy; @@ -286,7 +286,7 @@ ssize_t SurfaceComposerClient::getDisplayHeight(DisplayID dpy) ssize_t SurfaceComposerClient::getDisplayOrientation(DisplayID dpy) { - if (uint32_t(dpy)>=NUM_DISPLAY_MAX) + if (uint32_t(dpy)>=SharedBufferStack::NUM_DISPLAY_MAX) return BAD_VALUE; volatile surface_flinger_cblk_t const * cblk = get_cblk(); volatile display_cblk_t const * dcblk = cblk->displays + dpy; @@ -345,7 +345,7 @@ sp SurfaceComposerClient::createSurface( sp surface = mClient->createSurface(&data, pid, name, display, w, h, format, flags); if (surface != 0) { - if (uint32_t(data.token) < NUM_LAYERS_MAX) { + if (uint32_t(data.token) < SharedBufferStack::NUM_LAYERS_MAX) { result = new SurfaceControl(this, surface, data, w, h, format, flags); } } diff --git a/libs/surfaceflinger_client/tests/SharedBufferStack/SharedBufferStackTest.cpp b/libs/surfaceflinger_client/tests/SharedBufferStack/SharedBufferStackTest.cpp index f490a65..f409f48 100644 --- a/libs/surfaceflinger_client/tests/SharedBufferStack/SharedBufferStackTest.cpp +++ b/libs/surfaceflinger_client/tests/SharedBufferStack/SharedBufferStackTest.cpp @@ -54,8 +54,16 @@ int main(int argc, char** argv) printf("resize test\n"); - s.resize(6); - c.setBufferCount(6); + class SetBufferCountIPC : public SharedBufferClient::SetBufferCountCallback { + SharedBufferServer& s; + virtual status_t operator()(int bufferCount) const { + return s.resize(bufferCount); + } + public: + SetBufferCountIPC(SharedBufferServer& s) : s(s) { } + } resize(s); + + c.setBufferCount(6, resize); int list3[6] = {3, 2, 1, 4, 5, 0}; test0(s, c, 6, list3); -- cgit v1.1