[Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
Miklós Máté
mtmkls at gmail.com
Wed Feb 17 23:13:07 UTC 2016
On 02/14/2016 02:46 AM, Ian Romanick wrote:
> On 02/11/2016 12:58 PM, Miklós Máté wrote:
>> 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.
> Yeah... people have a tendency to only consider their own use cases. I
> suspect that when Chrome unbinds a drawable, that drawable is
> effectively dead.
>
>> 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).
> At least the last time I checked, approximately 0% of real, shipping
> applications call glXCreateWindow or glXDestroyWindow. It's not because
> the know what they're doing. More often than not, it's because they
> don't know what they're doing. They write the minimum amount of window
> system glue to make their app work and move on to the "interesting"
> bits. You even point out in another message that Wine does exactly this.
Yeah, the more I learn about GLX, the less my sanity is.
>
> With this change, all of those applications will leak the window
> drawable until the process dies. I can't really get behind creating
> resource leaks in Wine and Chrome.
>
> Presumably apps do call XDestroyWindow after unbinding a window from
> GLX. I think the proper fix is to decrement the ref count on the DRI
> drawables in glXMakeContextCurrent and either when glXDestroy* is called
> or when the underlying X resource is destroyed (e.g., by
> XDestroyWindow). I don't think it's possible to catch XDestroyWindow on
> the client. We used to do things like that using some Xlib mechanisms,
> but XCB doesn't let us do those clever things any more. Maybe Jamey has
> a suggestion.
>
> The other alternative would be to destroy the DRI drawable on the server
> when the associated window is destroyed. Since all the buffers for the
> DRI drawables are client-allocated these days, that may not even be
> possible, now that I think about it.
>
> I believe we still need the ref counting because drivers won't be able
> to handle having the DRI drawable disappear because an app calls
> glXDestroyWindow while the window is bound. The spec says that context
> updates occur but drawable contents are undefined. I suspect that
> having the DRI drawable disappear will result in a crash somewhere.
Mesa never handled correctly the case of glXDestroyWindow while the drawable
was bound. My patch doesn't change that. Maybe instead of removing the
refcounting
altogether I should add refcounting to create and destroy (although
nobody calls them)?
To sum up the situation, it seems there are 3 possible solutions:
- destroy drawables when unbound (the current situation)
- destroy drawables only when explicitly deleted, and live with a
resource leak
- plug the leak by introducing horrible hacks to catch XDestroyWindow
calls
I don't know how severe the leak is, but the second option doesn't seem
unacceptable to me. How many drawables are leaked this way?
MM
>
>> 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