Re: [Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
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 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 Signed-off-by: Stéphane Marchesin Reviewed-by: Adam Jackson 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 p
Re: [Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
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 >> 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 >> Signed-off-by: Stéphane Marchesin >> Reviewed-by: Adam Jackson >> >> 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. 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. > 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
Re: [Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
On 02/12/2016 06:56 AM, Michel Dänzer wrote: On 12.02.2016 05:58, 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 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 Signed-off-by: Stéphane Marchesin Reviewed-by: Adam Jackson 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. Could that be fixed by starting with refcount=1 instead of 0? In theory, yes. In practice, no. I tried to add refcount=1 to the glXCreate* functions, but for example, you don't need to call glXCreateWindow: win=XCreateWindow(); glXMakeCurrent(dpy, win, glc); is fine. This is what the example on OpenGL wiki does, and more importantly, Wine does this, too. When should refcount=1 be set in such cases? I couldn't figure it out. 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. Unless I'm missing something, one issue I see with your patch is that a DRI drawable could be destroyed while it's still current for a context. This is an interesting question. The GLX 1.4 specification says on page 28 that "If draw is destroyed after glXMakeContextCurrent is called, then subsequent rendering commands will be processed and the context state will be updated, but the frame buffer state becomes undefined." This suggests that the lifetime of the draw buffer ends with destroy, not unbind. If this is true, we don't need refcount on makecurrent. However, on page 23 it says "The storage for the GLX pixmap will be freed when it is not current to any client." This is interesting, because one pixmap can be current to more than one client, and thus it needs to be refcounted across multiple processes. I think "storage" in this case means the server-side resources, and not the dri drawable, but I could be wrong. MM ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
On 12.02.2016 05:58, 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 >> 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 >> Signed-off-by: Stéphane Marchesin >> Reviewed-by: Adam Jackson >> >> 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. Could that be fixed by starting with refcount=1 instead of 0? > 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. Unless I'm missing something, one issue I see with your patch is that a DRI drawable could be destroyed while it's still current for a context. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
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 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 Signed-off-by: Stéphane Marchesin Reviewed-by: Adam Jackson 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" --- 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 *) driFetchDraw
Re: [Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
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
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
Signed-off-by: Stéphane Marchesin
Reviewed-by: Adam Jackson
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. :(
>> 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"
>
>> ---
>> 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_
Re: [Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
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.
> 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"
> ---
> 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
[Mesa-dev] [PATCH 6/7] glx: remove incorrect refcounting of DRIdrawable
dri drawables must never be released when unbound from a context
as long as their corresponding glx objects (window, pixmap, pbuffer)
still exist
this fixes fd.o bug #93955 and disappearing characters in KotOR when
soft shadows are enabled
---
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 dri
