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 <marc...@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 <pi...@chromium.org>
Signed-off-by: Stéphane Marchesin <marc...@chromium.org>
Reviewed-by: Adam Jackson <a...@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-sta...@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
