From 9a11206fe793363c0e8897b478cbe6ef8c52b543 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Fri, 17 Apr 2009 19:36:26 -0700 Subject: more Surface lifetime management Surfaces are now destroyed once all references from the clients are gone, but they go through a partial destruction as soon as the window manager requests it. This last part is still buggy. see comments in SurfaceFlinger::destroySurface() --- libs/surfaceflinger/BootAnimation.cpp | 1 + libs/surfaceflinger/Layer.cpp | 17 ++++++-- libs/surfaceflinger/Layer.h | 5 ++- libs/surfaceflinger/LayerBase.cpp | 27 ++++++++----- libs/surfaceflinger/LayerBase.h | 24 ++++++++---- libs/surfaceflinger/LayerBuffer.cpp | 21 +++++++--- libs/surfaceflinger/LayerBuffer.h | 12 +++--- libs/surfaceflinger/SurfaceFlinger.cpp | 72 +++++++++++++++++++++++++++++----- libs/surfaceflinger/SurfaceFlinger.h | 6 ++- 9 files changed, 141 insertions(+), 44 deletions(-) diff --git a/libs/surfaceflinger/BootAnimation.cpp b/libs/surfaceflinger/BootAnimation.cpp index 519b112..ee36b67 100644 --- a/libs/surfaceflinger/BootAnimation.cpp +++ b/libs/surfaceflinger/BootAnimation.cpp @@ -179,6 +179,7 @@ bool BootAnimation::threadLoop() { eglMakeCurrent(mDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT); eglDestroyContext(mDisplay, mContext); eglDestroySurface(mDisplay, mSurface); + mFlingerSurface.clear(); mFlingerSurfaceControl.clear(); return r; } diff --git a/libs/surfaceflinger/Layer.cpp b/libs/surfaceflinger/Layer.cpp index 980b78b..5fdec3f 100644 --- a/libs/surfaceflinger/Layer.cpp +++ b/libs/surfaceflinger/Layer.cpp @@ -87,6 +87,12 @@ sp Layer::createSurface() const return mSurface; } +status_t Layer::ditch() +{ + mSurface.clear(); + return NO_ERROR; +} + status_t Layer::setBuffers( Client* client, uint32_t w, uint32_t h, PixelFormat format, uint32_t flags) @@ -119,7 +125,7 @@ status_t Layer::setBuffers( Client* client, return err; } } - mSurface = new SurfaceLayer(clientIndex(), this); + mSurface = new SurfaceLayer(mFlinger, clientIndex(), this); return NO_ERROR; } @@ -626,8 +632,13 @@ void Layer::finishPageFlip() // --------------------------------------------------------------------------- -Layer::SurfaceLayer::SurfaceLayer(SurfaceID id, const sp& owner) - : Surface(id, owner->getIdentity(), owner) +Layer::SurfaceLayer::SurfaceLayer(const sp& flinger, + SurfaceID id, const sp& owner) + : Surface(flinger, id, owner->getIdentity(), owner) +{ +} + +Layer::SurfaceLayer::~SurfaceLayer() { } diff --git a/libs/surfaceflinger/Layer.h b/libs/surfaceflinger/Layer.h index dc03d35..3f3953f 100644 --- a/libs/surfaceflinger/Layer.h +++ b/libs/surfaceflinger/Layer.h @@ -81,6 +81,7 @@ public: virtual bool needsBlending() const { return mNeedsBlending; } virtual bool isSecure() const { return mSecure; } virtual sp createSurface() const; + virtual status_t ditch(); const LayerBitmap& getBuffer(int i) const { return mBuffers[i]; } LayerBitmap& getBuffer(int i) { return mBuffers[i]; } @@ -108,7 +109,9 @@ private: class SurfaceLayer : public LayerBaseClient::Surface { public: - SurfaceLayer(SurfaceID id, const sp& owner); + SurfaceLayer(const sp& flinger, + SurfaceID id, const sp& owner); + ~SurfaceLayer(); private: virtual sp getBuffer(); diff --git a/libs/surfaceflinger/LayerBase.cpp b/libs/surfaceflinger/LayerBase.cpp index 6da0bf7..8609225 100644 --- a/libs/surfaceflinger/LayerBase.cpp +++ b/libs/surfaceflinger/LayerBase.cpp @@ -681,25 +681,32 @@ sp LayerBaseClient::getSurface() sp LayerBaseClient::createSurface() const { - return new Surface(clientIndex(), mIdentity, + return new Surface(mFlinger, clientIndex(), mIdentity, const_cast(this)); } // --------------------------------------------------------------------------- -LayerBaseClient::Surface::Surface(SurfaceID id, int identity, +LayerBaseClient::Surface::Surface( + const sp& flinger, + SurfaceID id, int identity, const sp& owner) - : mToken(id), mIdentity(identity), mOwner(owner) -{ + : mFlinger(flinger), mToken(id), mIdentity(identity), mOwner(owner) +{ } -LayerBaseClient::Surface::~Surface() { - // TODO: We now have a point here were we can clean-up the - // client's mess. - // This is also where surface id should be recycled. - //LOGD("Surface %d, heaps={%p, %p} destroyed", - // mId, mHeap[0].get(), mHeap[1].get()); +LayerBaseClient::Surface::~Surface() +{ + /* + * This is a good place to clean-up all client resources + */ + + // destroy client resources + sp layer = getOwner(); + if (layer != 0) { + mFlinger->destroySurface(layer); + } } sp LayerBaseClient::Surface::getOwner() const { diff --git a/libs/surfaceflinger/LayerBase.h b/libs/surfaceflinger/LayerBase.h index 2a4e133..ccff36d 100644 --- a/libs/surfaceflinger/LayerBase.h +++ b/libs/surfaceflinger/LayerBase.h @@ -203,14 +203,19 @@ public: /** * isSecure - true if this surface is secure, that is if it prevents - * screenshots or vns servers. + * screenshots or VNC servers. */ virtual bool isSecure() const { return false; } - enum { // flags for doTransaction() - eVisibleRegion = 0x00000002, - eRestartTransaction = 0x00000008 - }; + /** signal this layer that it's not needed any longer */ + virtual status_t ditch() { return NO_ERROR; } + + + + enum { // flags for doTransaction() + eVisibleRegion = 0x00000002, + eRestartTransaction = 0x00000008 + }; inline const State& drawingState() const { return mDrawingState; } @@ -244,7 +249,7 @@ protected: GLint textureName, const GGLSurface& t, GLuint& textureWidth, GLuint& textureHeight) const; - SurfaceFlinger* mFlinger; + sp mFlinger; uint32_t mFlags; // cached during validateVisibility() @@ -316,7 +321,9 @@ public: ISurfaceFlingerClient::surface_data_t* params) const; protected: - Surface(SurfaceID id, int identity, const sp& owner); + Surface(const sp& flinger, + SurfaceID id, int identity, + const sp& owner); virtual ~Surface(); virtual status_t onTransact(uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags); @@ -330,8 +337,9 @@ public: virtual sp createOverlay(uint32_t w, uint32_t h, int32_t format); - private: + protected: friend class LayerBaseClient; + sp mFlinger; int32_t mToken; int32_t mIdentity; wp mOwner; diff --git a/libs/surfaceflinger/LayerBuffer.cpp b/libs/surfaceflinger/LayerBuffer.cpp index f22ff59..97d6f97 100644 --- a/libs/surfaceflinger/LayerBuffer.cpp +++ b/libs/surfaceflinger/LayerBuffer.cpp @@ -50,10 +50,21 @@ LayerBuffer::~LayerBuffer() { } +void LayerBuffer::onFirstRef() +{ + mSurface = new SurfaceBuffer(mFlinger, clientIndex(), + const_cast(this)); +} + sp LayerBuffer::createSurface() const { - return new SurfaceBuffer(clientIndex(), - const_cast(this)); + return mSurface; +} + +status_t LayerBuffer::ditch() +{ + mSurface.clear(); + return NO_ERROR; } bool LayerBuffer::needsBlending() const { @@ -167,9 +178,9 @@ sp LayerBuffer::clearSource() { // LayerBuffer::SurfaceBuffer // ============================================================================ -LayerBuffer::SurfaceBuffer::SurfaceBuffer(SurfaceID id, - const sp& owner) - : LayerBaseClient::Surface(id, owner->getIdentity(), owner) +LayerBuffer::SurfaceBuffer::SurfaceBuffer(const sp& flinger, + SurfaceID id, const sp& owner) + : LayerBaseClient::Surface(flinger, id, owner->getIdentity(), owner) { } diff --git a/libs/surfaceflinger/LayerBuffer.h b/libs/surfaceflinger/LayerBuffer.h index 2d44121..5284409 100644 --- a/libs/surfaceflinger/LayerBuffer.h +++ b/libs/surfaceflinger/LayerBuffer.h @@ -50,7 +50,6 @@ class LayerBuffer : public LayerBaseClient LayerBuffer& mLayer; }; - public: static const uint32_t typeInfo; static const char* const typeID; @@ -61,9 +60,11 @@ public: Client* client, int32_t i); virtual ~LayerBuffer(); + virtual void onFirstRef(); virtual bool needsBlending() const; virtual sp createSurface() const; + virtual status_t ditch(); virtual void onDraw(const Region& clip) const; virtual uint32_t doTransaction(uint32_t flags); virtual void unlockPageFlip(const Transform& planeTransform, Region& outDirtyRegion); @@ -177,7 +178,8 @@ private: class SurfaceBuffer : public LayerBaseClient::Surface { public: - SurfaceBuffer(SurfaceID id, const sp& owner); + SurfaceBuffer(const sp& flinger, + SurfaceID id, const sp& owner); virtual ~SurfaceBuffer(); virtual status_t registerBuffers(const ISurface::BufferHeap& buffers); @@ -191,12 +193,10 @@ private: return static_cast(Surface::getOwner().get()); } }; - - friend class SurfaceFlinger; - sp getClientSurface() const; - + mutable Mutex mLock; sp mSource; + sp mSurface; bool mInvalidate; bool mNeedsBlending; }; diff --git a/libs/surfaceflinger/SurfaceFlinger.cpp b/libs/surfaceflinger/SurfaceFlinger.cpp index a63a282..6b2a103 100644 --- a/libs/surfaceflinger/SurfaceFlinger.cpp +++ b/libs/surfaceflinger/SurfaceFlinger.cpp @@ -1033,7 +1033,8 @@ status_t SurfaceFlinger::removeLayer_l(const sp& layerBase) ssize_t index = mCurrentState.layersSortedByZ.remove(layerBase); if (index >= 0) { mLayersRemoved = true; - sp layer = LayerBase::dynamicCast< LayerBaseClient* >(layerBase.get()); + sp layer = + LayerBase::dynamicCast< LayerBaseClient* >(layerBase.get()); if (layer != 0) { mLayerMap.removeItem(layer->serverIndex()); } @@ -1041,11 +1042,23 @@ status_t SurfaceFlinger::removeLayer_l(const sp& layerBase) } // it's possible that we don't find a layer, because it might // have been destroyed already -- this is not technically an error - // from the user because there is a race between destroySurface, - // destroyclient and destroySurface-from-a-transaction. + // from the user because there is a race between BClient::destroySurface(), + // ~BClient() and destroySurface-from-a-transaction. return (index == NAME_NOT_FOUND) ? status_t(NO_ERROR) : index; } +status_t SurfaceFlinger::purgatorizeLayer_l(const sp& layerBase) +{ + // First add the layer to the purgatory list, which makes sure it won't + // go away, then remove it from the main list (through a transaction). + ssize_t err = removeLayer_l(layerBase); + if (err >= 0) { + mLayerPurgatory.add(layerBase); + } + return (err == NAME_NOT_FOUND) ? status_t(NO_ERROR) : err; +} + + void SurfaceFlinger::free_resources_l() { // Destroy layers that were removed @@ -1252,14 +1265,53 @@ sp SurfaceFlinger::createPushBuffersSurfaceLocked( return layer; } -status_t SurfaceFlinger::destroySurface(SurfaceID index) +status_t SurfaceFlinger::removeSurface(SurfaceID index) +{ + /* + * called by the window manager, when a surface should be marked for + * destruction. + */ + + // TODO: here we should make the surface disappear from the screen + // and mark it for removal. however, we can't free anything until all + // client are done. All operations on this surface should return errors. + + status_t err = NAME_NOT_FOUND; + sp layer; + + { // scope for the lock + Mutex::Autolock _l(mStateLock); + layer = getLayerUser_l(index); + err = purgatorizeLayer_l(layer); + if (err == NO_ERROR) { + setTransactionFlags(eTransactionNeeded); + } + } + + if (layer != 0) { + // do this outside of mStateLock + layer->ditch(); + } + return err; +} + +status_t SurfaceFlinger::destroySurface(const sp& layer) { + /* + * called by ~ISurface() when all references are gone + */ + + /* FIXME: + * - this can calls ~Layer(), which is wrong because we're not in the + * GL thread, and ~Layer() currently calls OpenGL. + * - ideally we want to release as much GL state as possible after + * purgatorizeLayer_l() has been called and the surface is not in any + * active list. + * - ideally we'd call ~Layer() without mStateLock held + */ + Mutex::Autolock _l(mStateLock); - const sp& layer = getLayerUser_l(index); - status_t err = removeLayer_l(layer); - if (err < 0) - return err; - setTransactionFlags(eTransactionNeeded); + mLayerPurgatory.remove(layer); return NO_ERROR; } @@ -1626,7 +1678,7 @@ sp BClient::createSurface( status_t BClient::destroySurface(SurfaceID sid) { sid |= (mId << 16); // add the client-part to id - return mFlinger->destroySurface(sid); + return mFlinger->removeSurface(sid); } status_t BClient::setState(int32_t count, const layer_state_t* states) diff --git a/libs/surfaceflinger/SurfaceFlinger.h b/libs/surfaceflinger/SurfaceFlinger.h index cb84542..b488ab5 100644 --- a/libs/surfaceflinger/SurfaceFlinger.h +++ b/libs/surfaceflinger/SurfaceFlinger.h @@ -203,7 +203,8 @@ private: Client* client, DisplayID display, int32_t id, uint32_t w, uint32_t h, uint32_t flags); - status_t destroySurface(SurfaceID surface_id); + status_t removeSurface(SurfaceID surface_id); + status_t destroySurface(const sp& layer); status_t setClientState(ClientID cid, int32_t count, const layer_state_t* states); @@ -287,6 +288,7 @@ private: sp getLayerUser_l(SurfaceID index) const; status_t addLayer_l(const sp& layer); status_t removeLayer_l(const sp& layer); + status_t purgatorizeLayer_l(const sp& layer); void free_resources_l(); uint32_t getTransactionFlags(uint32_t flags); @@ -315,7 +317,9 @@ private: volatile int32_t mTransactionFlags; volatile int32_t mTransactionCount; Condition mTransactionCV; + SortedVector< sp > mLayerPurgatory; + // protected by mStateLock (but we could use another lock) Tokenizer mTokens; DefaultKeyedVector mClientsMap; -- cgit v1.1