On Fri, 27 Mar 2009 13:20:25 -0400 Kristian Høgsberg <[email protected]> wrote:
> On Thu, Mar 26, 2009 at 6:33 PM, Jesse Barnes > <[email protected]> wrote: > > Ok, I think this is where the leak was: > > __glXUnrefDrawable->DrawableGone->__glXUnrefDrawable. This sequence > > meant the glxPriv would stay around (since it was used), but > > DrawableGone had generally already disposed of the pDrawable before > > the call to Unref, so we never got into DRI2DestroyBuffers, thus > > the leak. > > > > The loop seems broken to me. It *looks* like DrawableGone should be > > the real free routine, only called when a drawable really is free, > > so I've fixed up the code to conform to that. This means removing > > the GLX priv frees from DRI and DRI2 routines and putting them here > > and using the GLX unref routine everwhere (since it will only free > > the resource when the refcount reaches 0). > > > > Any thoughts? I'd appreciate some more testers too... I'm not > > quite sure about the generic DoDestroyDrawable change, but if the > > refcount is always 1 there anyway it shouldn't make a difference > > and seems more correct. > > The __GLXdrawable is a reference counted object, and the glx code > references it in a couple of places: when there's an X resource > pointing to it (a GLXWindow, GLXPixmap or GLXPbuffer) and when it's > the current drawable or readable for a context. The __GLXdrawable is > allocated by the backend in use (dri, dri2 or swrast) and must be > freed by the same backend (don't mix alloc and free across abstraction > barriers). We unref the __GLXdrawable when we either set a different > drawable/readable and when the X resource goes away. > > The leak is (as you pointed out before) because we NULL the pDraw > pointer before calling the backend destructor, which then can't unref > the DRI2Drawable, which we then leak. You had the right fix in the > originial post, we just need to not touch glxPriv after > __glXUnrefDrawable if it got destroyed. I suggest this change to > DrawableGone in irc: > > refcount = glxPriv->refcount; > __glXUnrefDrawable(glxPriv); > if (refcount > 1) { > glxPriv->pDraw = NULL; > glxPriv->drawId = 0; > } Yep, that seems to work too... Magnus or Vasily can you guys confirm? Thanks, -- Jesse Barnes, Intel Open Source Technology Center diff --git a/glx/glxext.c b/glx/glxext.c index c882372..fec45b7 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -119,6 +119,7 @@ static int ContextGone(__GLXcontext* cx, XID id) static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) { ScreenPtr pScreen = glxPriv->pDraw->pScreen; + int refcount; switch (glxPriv->type) { case GLX_DRAWABLE_PIXMAP: @@ -127,9 +128,12 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) break; } - glxPriv->pDraw = NULL; - glxPriv->drawId = 0; + refcount = glxPriv->refCount; __glXUnrefDrawable(glxPriv); + if (refcount > 1) { + glxPriv->pDraw = NULL; + glxPriv->drawId = 0; + } return True; } _______________________________________________ xorg mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/xorg
