diff options
author | Mathias Agopian <mathias@google.com> | 2011-05-19 15:38:14 -0700 |
---|---|---|
committer | Mathias Agopian <mathias@google.com> | 2011-05-23 16:14:10 -0700 |
commit | 6f96080f333c70104424f42ac263c4c9c7a9dd2a (patch) | |
tree | fa24bb5ec97e25b9d0535ef18f860cb623f2ba3d | |
parent | 9e7636681eda094dd328ce9bf54d0b0166be1e67 (diff) | |
download | base-6f96080f333c70104424f42ac263c4c9c7a9dd2a.tar.gz |
Fix a race that could cause GL commands to be executed from the wrong thread.
Bug: 4483050
Change-Id: I37f0f3156059c208c6168ee6131d0e267d823188
-rw-r--r-- | services/surfaceflinger/Layer.cpp | 20 | ||||
-rw-r--r-- | services/surfaceflinger/Layer.h | 2 | ||||
-rw-r--r-- | services/surfaceflinger/LayerBase.cpp | 5 | ||||
-rw-r--r-- | services/surfaceflinger/LayerBase.h | 7 | ||||
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger.cpp | 120 | ||||
-rw-r--r-- | services/surfaceflinger/SurfaceFlinger.h | 13 |
6 files changed, 70 insertions, 97 deletions
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index c9dcef370a4d..4739cc3072c1 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -76,6 +76,10 @@ Layer::~Layer() } } +void Layer::destroy() const { + mFlinger->destroyLayer(this); +} + status_t Layer::setToken(const sp<UserClient>& userClient, SharedClient* sharedClient, int32_t token) { @@ -123,22 +127,6 @@ sp<LayerBaseClient::Surface> Layer::createSurface() const return mSurface; } -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(); - - EGLDisplay dpy(mFlinger->graphicPlane(0).getEGLDisplay()); - mBufferManager.destroy(dpy); - mSurface.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 7bb207a1fcdd..51b52f9eb10d 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -78,7 +78,6 @@ public: virtual bool needsFiltering() const; virtual bool isSecure() const { return mSecure; } virtual sp<Surface> createSurface() const; - virtual status_t ditch(); virtual void onRemoved(); virtual bool setBypass(bool enable); @@ -95,6 +94,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 21c36e18485e..3986fdef0f30 100644 --- a/services/surfaceflinger/LayerBase.cpp +++ b/services/surfaceflinger/LayerBase.cpp @@ -583,10 +583,7 @@ LayerBaseClient::Surface::~Surface() */ // destroy client resources - sp<LayerBaseClient> layer = getOwner(); - if (layer != 0) { - mFlinger->destroySurface(layer); - } + mFlinger->destroySurface(mOwner); } sp<LayerBaseClient> LayerBaseClient::Surface::getOwner() const { diff --git a/services/surfaceflinger/LayerBase.h b/services/surfaceflinger/LayerBase.h index afc5ec8d5073..e69cb6a26fa4 100644 --- a/services/surfaceflinger/LayerBase.h +++ b/services/surfaceflinger/LayerBase.h @@ -196,10 +196,6 @@ public: */ virtual bool isSecure() 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() { }; @@ -264,7 +260,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 5314cb09e7a4..a93d7568488c 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -358,6 +358,9 @@ bool SurfaceFlinger::threadLoop() { waitForEvent(); + // call Layer's destructor + handleDestroyLayers(); + // check for transactions if (UNLIKELY(mConsoleSignals)) { handleConsoleEvents(); @@ -467,47 +470,26 @@ void SurfaceFlinger::handleConsoleEvents() void SurfaceFlinger::handleTransaction(uint32_t transactionFlags) { - Vector< sp<LayerBase> > ditchedLayers; - - /* - * Perform and commit the transaction - */ - - { // scope for the lock - Mutex::Autolock _l(mStateLock); - const nsecs_t now = systemTime(); - mDebugInTransaction = now; + Mutex::Autolock _l(mStateLock); + const nsecs_t now = systemTime(); + mDebugInTransaction = now; - // 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. + // 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. - const uint32_t mask = eTransactionNeeded | eTraversalNeeded; - transactionFlags = getTransactionFlags(mask); - handleTransactionLocked(transactionFlags, ditchedLayers); + const uint32_t mask = eTransactionNeeded | eTraversalNeeded; + transactionFlags = getTransactionFlags(mask); + handleTransactionLocked(transactionFlags); - mLastTransactionTime = systemTime() - now; - mDebugInTransaction = 0; - // 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 ; i<count ; i++) { - if (ditchedLayers[i] != 0) { - //LOGD("ditching layer %p", ditchedLayers[i].get()); - ditchedLayers[i]->ditch(); - } - } + mLastTransactionTime = systemTime() - now; + mDebugInTransaction = 0; + // here the transaction has been committed } -void SurfaceFlinger::handleTransactionLocked( - uint32_t transactionFlags, Vector< sp<LayerBase> >& ditchedLayers) +void SurfaceFlinger::handleTransactionLocked(uint32_t transactionFlags) { const LayerVector& currentLayers(mCurrentState.layersSortedByZ); const size_t count = currentLayers.size(); @@ -579,7 +561,6 @@ void SurfaceFlinger::handleTransactionLocked( const sp<LayerBase>& layer(previousLayers[i]); if (currentLayers.indexOf( layer ) < 0) { // this layer is not visible anymore - ditchedLayers.add(layer); mDirtyRegionRemovedLayer.orSelf(layer->visibleRegionScreen); } } @@ -589,6 +570,31 @@ void SurfaceFlinger::handleTransactionLocked( commitTransaction(); } +void SurfaceFlinger::destroyLayer(LayerBase const* layer) +{ + Mutex::Autolock _l(mDestroyedLayerLock); + mDestroyedLayers.add(layer); + signalEvent(); +} + +void SurfaceFlinger::handleDestroyLayers() +{ + Vector<LayerBase const *> 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 ; i<count ; i++) { + //LOGD("destroying %s", destroyedLayers[i]->getName().string()); + delete destroyedLayers[i]; + } +} + sp<FreezeLock> SurfaceFlinger::getFreezeLock() const { return new FreezeLock(const_cast<SurfaceFlinger *>(this)); @@ -1329,38 +1335,18 @@ status_t SurfaceFlinger::removeSurface(const sp<Client>& client, SurfaceID sid) return err; } -status_t SurfaceFlinger::destroySurface(const sp<LayerBaseClient>& layer) +status_t SurfaceFlinger::destroySurface(const wp<LayerBaseClient>& layer) { // called by ~ISurface() when all references are gone - - class MessageDestroySurface : public MessageBase { - SurfaceFlinger* flinger; - sp<LayerBaseClient> layer; - public: - MessageDestroySurface( - SurfaceFlinger* flinger, const sp<LayerBaseClient>& layer) - : flinger(flinger), layer(layer) { } - virtual bool handler() { - sp<LayerBaseClient> 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); - LOGE_IF(err<0 && err != NAME_NOT_FOUND, - "error removing layer=%p (%s)", l.get(), strerror(-err)); - return true; - } - }; - - postMessageAsync( new MessageDestroySurface(this, layer) ); - return NO_ERROR; + status_t err = NO_ERROR; + sp<LayerBaseClient> l(layer.promote()); + if (l != NULL) { + Mutex::Autolock _l(mStateLock); + err = removeLayer_l(l); + 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 a76a1c118238..9fa98cf2edf3 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -218,6 +218,7 @@ public: status_t removeLayer(const sp<LayerBase>& layer); status_t addLayer(const sp<LayerBase>& layer); status_t invalidateLayerVisibility(const sp<LayerBase>& layer); + void destroyLayer(LayerBase const* layer); sp<Layer> getLayer(const sp<ISurface>& sur) const; @@ -255,7 +256,7 @@ private: uint32_t w, uint32_t h, uint32_t flags); status_t removeSurface(const sp<Client>& client, SurfaceID sid); - status_t destroySurface(const sp<LayerBaseClient>& layer); + status_t destroySurface(const wp<LayerBaseClient>& layer); status_t setClientState(const sp<Client>& 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<LayerBase> >& ditchedLayers); + void handleTransactionLocked(uint32_t transactionFlags); + void handleDestroyLayers(); void computeVisibleRegions( LayerVector& currentLayers, @@ -422,6 +422,11 @@ private: // these are thread safe mutable Barrier mReadyToRunBarrier; + + // protected by mDestroyedLayerLock; + mutable Mutex mDestroyedLayerLock; + Vector<LayerBase const *> mDestroyedLayers; + // atomic variables enum { eConsoleReleased = 1, |