From ca4d3602c07837d0b2ac6878685a8e327b5f30f0 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Thu, 19 May 2011 15:38:14 -0700 Subject: [PATCH] Fix a race that could cause GL commands to be executed from the wrong thread. Change-Id: Ia3d407f7bf2f5553f46cfdade70b7b0badb35beb --- services/surfaceflinger/Layer.cpp | 16 +-- services/surfaceflinger/Layer.h | 2 +- services/surfaceflinger/LayerBase.cpp | 5 +- services/surfaceflinger/LayerBase.h | 7 +- services/surfaceflinger/SurfaceFlinger.cpp | 142 +++++++++------------ services/surfaceflinger/SurfaceFlinger.h | 13 +- 6 files changed, 78 insertions(+), 107 deletions(-) diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index b2f95cd2a..bbf181400 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -79,6 +79,10 @@ Layer::~Layer() } } +void Layer::destroy() const { + mFlinger->destroyLayer(this); +} + status_t Layer::setToken(const sp& userClient, SharedClient* sharedClient, int32_t token) { @@ -147,18 +151,6 @@ sp Layer::createSurface() const return sur; } -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(); - - Mutex::Autolock _l(mLock); - mWidth = mHeight = 0; - return NO_ERROR; -} - status_t Layer::setBuffers( uint32_t w, uint32_t h, PixelFormat format, uint32_t flags) { diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 128f93d84..278d64e97 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -82,7 +82,6 @@ public: virtual bool isSecure() const { return mSecure; } virtual bool isProtected() const; virtual sp createSurface() const; - virtual status_t ditch(); virtual void onRemoved(); // only for debugging @@ -93,6 +92,7 @@ public: return mFreezeLock; } protected: + virtual void destroy() const; virtual void dump(String8& result, char* scratch, size_t size) const; private: diff --git a/services/surfaceflinger/LayerBase.cpp b/services/surfaceflinger/LayerBase.cpp index 6025ed492..022f25145 100644 --- a/services/surfaceflinger/LayerBase.cpp +++ b/services/surfaceflinger/LayerBase.cpp @@ -616,10 +616,7 @@ LayerBaseClient::Surface::~Surface() */ // destroy client resources - sp layer = getOwner(); - if (layer != 0) { - mFlinger->destroySurface(layer); - } + mFlinger->destroySurface(mOwner); } sp LayerBaseClient::Surface::getOwner() const { diff --git a/services/surfaceflinger/LayerBase.h b/services/surfaceflinger/LayerBase.h index 7162e4727..6c49a1972 100644 --- a/services/surfaceflinger/LayerBase.h +++ b/services/surfaceflinger/LayerBase.h @@ -202,10 +202,6 @@ public: */ virtual bool isProtected() const { return false; } - /** Called from the main thread, when the surface is removed from the - * draw list */ - virtual status_t ditch() { return NO_ERROR; } - /** called with the state lock when the surface is removed from the * current list */ virtual void onRemoved() { }; @@ -271,7 +267,8 @@ protected: volatile int32_t mInvalidate; -protected: +public: + // called from class SurfaceFlinger virtual ~LayerBase(); private: diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index e8f0328b3..b7a51a4e5 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -387,6 +387,9 @@ bool SurfaceFlinger::threadLoop() { waitForEvent(); + // call Layer's destructor + handleDestroyLayers(); + // check for transactions if (UNLIKELY(mConsoleSignals)) { handleConsoleEvents(); @@ -480,49 +483,27 @@ void SurfaceFlinger::handleConsoleEvents() void SurfaceFlinger::handleTransaction(uint32_t transactionFlags) { - Vector< sp > ditchedLayers; + Mutex::Autolock _l(mStateLock); + const nsecs_t now = systemTime(); + mDebugInTransaction = now; - /* - * Perform and commit the transaction - */ + // Here we're guaranteed that some transaction flags are set + // so we can call handleTransactionLocked() unconditionally. + // We call getTransactionFlags(), which will also clear the flags, + // with mStateLock held to guarantee that mCurrentState won't change + // until the transaction is committed. - { // scope for the lock - Mutex::Autolock _l(mStateLock); - const nsecs_t now = systemTime(); - mDebugInTransaction = now; + const uint32_t mask = eTransactionNeeded | eTraversalNeeded; + transactionFlags = getTransactionFlags(mask); + handleTransactionLocked(transactionFlags); - // Here we're guaranteed that some transaction flags are set - // so we can call handleTransactionLocked() unconditionally. - // We call getTransactionFlags(), which will also clear the flags, - // with mStateLock held to guarantee that mCurrentState won't change - // until the transaction is commited. - - const uint32_t mask = eTransactionNeeded | eTraversalNeeded; - transactionFlags = getTransactionFlags(mask); - handleTransactionLocked(transactionFlags, ditchedLayers); - - mLastTransactionTime = systemTime() - now; - mDebugInTransaction = 0; - invalidateHwcGeometry(); - // here the transaction has been committed - } - - /* - * Clean-up all layers that went away - * (do this without the lock held) - */ - - const size_t count = ditchedLayers.size(); - for (size_t i=0 ; iditch(); - } - } + mLastTransactionTime = systemTime() - now; + mDebugInTransaction = 0; + invalidateHwcGeometry(); + // here the transaction has been committed } -void SurfaceFlinger::handleTransactionLocked( - uint32_t transactionFlags, Vector< sp >& ditchedLayers) +void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags) { const LayerVector& currentLayers(mCurrentState.layersSortedByZ); const size_t count = currentLayers.size(); @@ -594,7 +575,6 @@ void SurfaceFlinger::handleTransactionLocked( const sp& layer(previousLayers[i]); if (currentLayers.indexOf( layer ) < 0) { // this layer is not visible anymore - ditchedLayers.add(layer); mDirtyRegionRemovedLayer.orSelf(layer->visibleRegionScreen); } } @@ -604,6 +584,31 @@ void SurfaceFlinger::handleTransactionLocked( commitTransaction(); } +void SurfaceFlinger::destroyLayer(LayerBase const* layer) +{ + Mutex::Autolock _l(mDestroyedLayerLock); + mDestroyedLayers.add(layer); + signalEvent(); +} + +void SurfaceFlinger::handleDestroyLayers() +{ + Vector destroyedLayers; + + { // scope for the lock + Mutex::Autolock _l(mDestroyedLayerLock); + destroyedLayers = mDestroyedLayers; + mDestroyedLayers.clear(); + } + + // call destructors without a lock held + const size_t count = destroyedLayers.size(); + for (size_t i=0 ; igetName().string()); + delete destroyedLayers[i]; + } +} + sp SurfaceFlinger::getFreezeLock() const { return new FreezeLock(const_cast(this)); @@ -1377,51 +1382,26 @@ status_t SurfaceFlinger::removeSurface(const sp& client, SurfaceID sid) return err; } -status_t SurfaceFlinger::destroySurface(const sp& layer) +status_t SurfaceFlinger::destroySurface(const wp& layer) { // called by ~ISurface() when all references are gone - - class MessageDestroySurface : public MessageBase { - SurfaceFlinger* flinger; - sp layer; - public: - MessageDestroySurface( - SurfaceFlinger* flinger, const sp& layer) - : flinger(flinger), layer(layer) { } - virtual bool handler() { - sp l(layer); - layer.clear(); // clear it outside of the lock; - Mutex::Autolock _l(flinger->mStateLock); - /* - * remove the layer from the current list -- chances are that it's - * not in the list anyway, because it should have been removed - * already upon request of the client (eg: window manager). - * However, a buggy client could have not done that. - * Since we know we don't have any more clients, we don't need - * to use the purgatory. - */ - status_t err = flinger->removeLayer_l(l); - if (err == NAME_NOT_FOUND) { - // The surface wasn't in the current list, which means it was - // removed already, which means it is in the purgatory, - // and need to be removed from there. - // This needs to happen from the main thread since its dtor - // must run from there (b/c of OpenGL ES). Additionally, we - // can't really acquire our internal lock from - // destroySurface() -- see postMessage() below. - ssize_t idx = flinger->mLayerPurgatory.remove(l); - LOGE_IF(idx < 0, - "layer=%p is not in the purgatory list", l.get()); - } - - LOGE_IF(err<0 && err != NAME_NOT_FOUND, - "error removing layer=%p (%s)", l.get(), strerror(-err)); - return true; + status_t err = NO_ERROR; + sp l(layer.promote()); + if (l != NULL) { + Mutex::Autolock _l(mStateLock); + err = removeLayer_l(l); + if (err == NAME_NOT_FOUND) { + // The surface wasn't in the current list, which means it was + // removed already, which means it is in the purgatory, + // and need to be removed from there. + ssize_t idx = mLayerPurgatory.remove(l); + LOGE_IF(idx < 0, + "layer=%p is not in the purgatory list", l.get()); } - }; - - postMessageAsync( new MessageDestroySurface(this, layer) ); - return NO_ERROR; + LOGE_IF(err<0 && err != NAME_NOT_FOUND, + "error removing layer=%p (%s)", l.get(), strerror(-err)); + } + return err; } status_t SurfaceFlinger::setClientState( diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 1e1694324..992861a0a 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -227,6 +227,7 @@ public: status_t addLayer(const sp& layer); status_t invalidateLayerVisibility(const sp& layer); void invalidateHwcGeometry(); + void destroyLayer(LayerBase const* layer); sp getLayer(const sp& sur) const; @@ -255,7 +256,7 @@ private: uint32_t w, uint32_t h, uint32_t flags); status_t removeSurface(const sp& client, SurfaceID sid); - status_t destroySurface(const sp& layer); + status_t destroySurface(const wp& layer); status_t setClientState(const sp& client, int32_t count, const layer_state_t* states); @@ -300,9 +301,8 @@ public: // hack to work around gcc 4.0.3 bug private: void handleConsoleEvents(); void handleTransaction(uint32_t transactionFlags); - void handleTransactionLocked( - uint32_t transactionFlags, - Vector< sp >& ditchedLayers); + void handleTransactionLocked(uint32_t transactionFlags); + void handleDestroyLayers(); void computeVisibleRegions( const LayerVector& currentLayers, @@ -424,6 +424,11 @@ private: // these are thread safe mutable Barrier mReadyToRunBarrier; + + // protected by mDestroyedLayerLock; + mutable Mutex mDestroyedLayerLock; + Vector mDestroyedLayers; + // atomic variables enum { eConsoleReleased = 1,