[Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
Miklós Máté
mtmkls at gmail.com
Thu Feb 11 20:58:46 UTC 2016
On 02/09/2016 05:02 PM, Ian Romanick wrote:
> On 02/08/2016 05:11 PM, Ian Romanick wrote:
>> On 02/05/2016 01:11 PM, Miklós Máté wrote:
>>> dri drawables must never be released when unbound from a context
>>> as long as their corresponding glx objects (window, pixmap, pbuffer)
>>> still exist
>> I'd really like to have Kristian weigh in, since DRI2 was his design,
>> and this is all his code being affected. That said, I'm a little unsure
>> about this change.
>>
>> The DRI drawables and associated resources will still get freed when the
>> application eventually calls glXDestroyPBuffer or glXDestroyWindow.
>>
>> But I'm not 100% sure what will happen with GLX 1.2 applications...
>> there are no glXDestroy* functions in GLX 1.2. In that case the
>> application will have a soft leak because calling XDestroyWindow won't
>> do anything on the client (or server?) for the DRI resources. I suspect
>> that's what this code was trying to prevent.
> And this commit affirms that:
>
> commit bf69ce37f0dcbb479078ee676d5100ac63e20750
> Author: Stéphane Marchesin <marcheu at chromium.org>
> Date: Wed Jun 15 15:09:12 2011 -0700
>
> glx: implement drawable refcounting.
>
> The current dri context unbind logic will leak drawables until the process
> dies (they will then get released by the GEM code). There are two ways to fix
> this: either always call driReleaseDrawables every time we unbind a context
> (but that costs us round trips to the X server at getbuffers() time) or
> implement proper drawable refcounting. This patch implements the latter.
>
> Signed-off-by: Antoine Labour <piman at chromium.org>
> Signed-off-by: Stéphane Marchesin <marcheu at chromium.org>
> Reviewed-by: Adam Jackson <ajax at redhat.com>
>
> Since we don't have any way to know when the corresponding GLX object
> ceases to exist, we don't know when it's safe to destroy the DRI
> object. I don't know how we can not leak and not prematurely destroy
> the object. :(
That commit message exaggerates a little when it says "proper
refcounting", because it only refcounts on bind (in driFetchDrawable),
but not on create/destroy. With current Mesa the following happens:
create glXPbuffer: dri drawable is created, refcount=0
bind it: refcount++
unbind it: refcount--, drawable is destroyed
bind again: create new drawable, refcount=1
unbind again: refcount--, drawable is destroyed
etc.
They also say that "current unbind logic will leak drawables until the
process dies", which is simply not true. The glXDestroy* calls properly
dispose of the corresponding dri objects.
With GLX 1.3 the dri drawables are correctly created and destroyed along
with their corresponding glx objects even without the above commit. IMHO
if someone deliberately uses GLX 1.2 or earlier, they know what they are
doing (1.3 was released in 1998). I'd say that it's never safe to
destroy a dri object until it's explicitly destroyed by the user, so I'm
still convinced that my patch is correct.
MM
>>> this fixes fd.o bug #93955 and disappearing characters in KotOR when
>>> soft shadows are enabled
>> I think this is also a candidate for stable. Reference this in the
>> commit message like:
>>
>> This fixes disappearing characters in KotOR when soft shadows are enabled.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93955
>> Cc: "11.1" <mesa-stable at lists.freedesktop.org>
>>
>>> ---
>>> src/glx/dri2_glx.c | 4 ----
>>> src/glx/dri3_glx.c | 4 ----
>>> src/glx/dri_common.c | 38 --------------------------------------
>>> src/glx/dri_glx.c | 4 ----
>>> src/glx/drisw_glx.c | 4 ----
>>> src/glx/glxclient.h | 1 -
>>> 6 files changed, 55 deletions(-)
>>>
>>> diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
>>> index 7710349..ebc878f 100644
>>> --- a/src/glx/dri2_glx.c
>>> +++ b/src/glx/dri2_glx.c
>>> @@ -122,8 +122,6 @@ dri2_destroy_context(struct glx_context *context)
>>> struct dri2_context *pcp = (struct dri2_context *) context;
>>> struct dri2_screen *psc = (struct dri2_screen *) context->psc;
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> free((char *) context->extensions);
>>>
>>> (*psc->core->destroyContext) (pcp->driContext);
>>> @@ -145,8 +143,6 @@ dri2_bind_context(struct glx_context *context, struct glx_context *old,
>>> pdraw = (struct dri2_drawable *) driFetchDrawable(context, draw);
>>> pread = (struct dri2_drawable *) driFetchDrawable(context, read);
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> if (pdraw)
>>> dri_draw = pdraw->driDrawable;
>>> else if (draw != None)
>>> diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
>>> index 6054ffc..38f5799 100644
>>> --- a/src/glx/dri3_glx.c
>>> +++ b/src/glx/dri3_glx.c
>>> @@ -186,8 +186,6 @@ dri3_destroy_context(struct glx_context *context)
>>> struct dri3_context *pcp = (struct dri3_context *) context;
>>> struct dri3_screen *psc = (struct dri3_screen *) context->psc;
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> free((char *) context->extensions);
>>>
>>> (*psc->core->destroyContext) (pcp->driContext);
>>> @@ -206,8 +204,6 @@ dri3_bind_context(struct glx_context *context, struct glx_context *old,
>>> pdraw = (struct dri3_drawable *) driFetchDrawable(context, draw);
>>> pread = (struct dri3_drawable *) driFetchDrawable(context, read);
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> if (pdraw == NULL || pread == NULL)
>>> return GLXBadDrawable;
>>>
>>> diff --git a/src/glx/dri_common.c b/src/glx/dri_common.c
>>> index 6728d38..2d334ab 100644
>>> --- a/src/glx/dri_common.c
>>> +++ b/src/glx/dri_common.c
>>> @@ -404,7 +404,6 @@ driFetchDrawable(struct glx_context *gc, GLXDrawable glxDrawable)
>>> return NULL;
>>>
>>> if (__glxHashLookup(priv->drawHash, glxDrawable, (void *) &pdraw) == 0) {
>>> - pdraw->refcount ++;
>>> return pdraw;
>>> }
>>>
>>> @@ -420,47 +419,10 @@ driFetchDrawable(struct glx_context *gc, GLXDrawable glxDrawable)
>>> (*pdraw->destroyDrawable) (pdraw);
>>> return NULL;
>>> }
>>> - pdraw->refcount = 1;
>>>
>>> return pdraw;
>>> }
>>>
>>> -_X_HIDDEN void
>>> -driReleaseDrawables(struct glx_context *gc)
>>> -{
>>> - const struct glx_display *priv = gc->psc->display;
>>> - __GLXDRIdrawable *pdraw;
>>> -
>>> - if (priv == NULL)
>>> - return;
>>> -
>>> - if (__glxHashLookup(priv->drawHash,
>>> - gc->currentDrawable, (void *) &pdraw) == 0) {
>>> - if (pdraw->drawable == pdraw->xDrawable) {
>>> - pdraw->refcount --;
>>> - if (pdraw->refcount == 0) {
>>> - (*pdraw->destroyDrawable)(pdraw);
>>> - __glxHashDelete(priv->drawHash, gc->currentDrawable);
>>> - }
>>> - }
>>> - }
>>> -
>>> - if (__glxHashLookup(priv->drawHash,
>>> - gc->currentReadable, (void *) &pdraw) == 0) {
>>> - if (pdraw->drawable == pdraw->xDrawable) {
>>> - pdraw->refcount --;
>>> - if (pdraw->refcount == 0) {
>>> - (*pdraw->destroyDrawable)(pdraw);
>>> - __glxHashDelete(priv->drawHash, gc->currentReadable);
>>> - }
>>> - }
>>> - }
>>> -
>>> - gc->currentDrawable = None;
>>> - gc->currentReadable = None;
>>> -
>>> -}
>>> -
>>> _X_HIDDEN bool
>>> dri2_convert_glx_attribs(unsigned num_attribs, const uint32_t *attribs,
>>> unsigned *major_ver, unsigned *minor_ver,
>>> diff --git a/src/glx/dri_glx.c b/src/glx/dri_glx.c
>>> index d087751..69d7502 100644
>>> --- a/src/glx/dri_glx.c
>>> +++ b/src/glx/dri_glx.c
>>> @@ -526,8 +526,6 @@ dri_destroy_context(struct glx_context * context)
>>> struct dri_context *pcp = (struct dri_context *) context;
>>> struct dri_screen *psc = (struct dri_screen *) context->psc;
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> free((char *) context->extensions);
>>>
>>> (*psc->core->destroyContext) (pcp->driContext);
>>> @@ -547,8 +545,6 @@ dri_bind_context(struct glx_context *context, struct glx_context *old,
>>> pdraw = (struct dri_drawable *) driFetchDrawable(context, draw);
>>> pread = (struct dri_drawable *) driFetchDrawable(context, read);
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> if (pdraw == NULL || pread == NULL)
>>> return GLXBadDrawable;
>>>
>>> diff --git a/src/glx/drisw_glx.c b/src/glx/drisw_glx.c
>>> index 241ac7f..2350292 100644
>>> --- a/src/glx/drisw_glx.c
>>> +++ b/src/glx/drisw_glx.c
>>> @@ -234,8 +234,6 @@ drisw_destroy_context(struct glx_context *context)
>>> struct drisw_context *pcp = (struct drisw_context *) context;
>>> struct drisw_screen *psc = (struct drisw_screen *) context->psc;
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> free((char *) context->extensions);
>>>
>>> (*psc->core->destroyContext) (pcp->driContext);
>>> @@ -254,8 +252,6 @@ drisw_bind_context(struct glx_context *context, struct glx_context *old,
>>> pdraw = (struct drisw_drawable *) driFetchDrawable(context, draw);
>>> pread = (struct drisw_drawable *) driFetchDrawable(context, read);
>>>
>>> - driReleaseDrawables(&pcp->base);
>>> -
>>> if (pdraw == NULL || pread == NULL)
>>> return GLXBadDrawable;
>>>
>>> diff --git a/src/glx/glxclient.h b/src/glx/glxclient.h
>>> index 2776b44..9105be3 100644
>>> --- a/src/glx/glxclient.h
>>> +++ b/src/glx/glxclient.h
>>> @@ -134,7 +134,6 @@ struct __GLXDRIdrawableRec
>>> GLenum textureTarget;
>>> GLenum textureFormat; /* EXT_texture_from_pixmap support */
>>> unsigned long eventMask;
>>> - int refcount;
>>> };
>>>
>>> /*
>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
More information about the mesa-dev
mailing list