Although there may be more than one resource handles pointing to the Drawable, we only want to destroy it once and only reference the resource which may have just been deleted on the first instance.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Kristian Høgsberg krh@bitplanet.net Cc: Michel Dänzer daenzer@vmware.com --- glx/glxcmds.c | 6 ++++++ glx/glxdrawable.h | 3 +++ glx/glxext.c | 15 ++++++++++----- 3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/glx/glxcmds.c b/glx/glxcmds.c index de9c3f0..fa153d5 100644 --- a/glx/glxcmds.c +++ b/glx/glxcmds.c @@ -529,6 +529,7 @@ __glXGetDrawable(__GLXcontext *glxc, GLXDrawable drawId, ClientPtr client, *error = BadAlloc; return NULL; } + pGlxDraw->refcnt++;
return pGlxDraw; } @@ -1099,8 +1100,10 @@ __glXDrawableInit(__GLXdrawable *drawable, drawable->pDraw = pDraw; drawable->type = type; drawable->drawId = drawId; + drawable->otherId = 0; drawable->config = config; drawable->eventMask = 0; + drawable->refcnt = 0;
return GL_TRUE; } @@ -1130,6 +1133,7 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, pGlxDraw->destroy (pGlxDraw); return BadAlloc; } + pGlxDraw->refcnt++;
/* Add the glx drawable under the XID of the underlying X drawable * too. That way we'll get a callback in DrawableGone and can @@ -1139,6 +1143,8 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, pGlxDraw->destroy (pGlxDraw); return BadAlloc; } + pGlxDraw->refcnt++; + pGlxDraw->otherId = pDraw->id;
return Success; } diff --git a/glx/glxdrawable.h b/glx/glxdrawable.h index 2a365c5..80c3234 100644 --- a/glx/glxdrawable.h +++ b/glx/glxdrawable.h @@ -51,8 +51,11 @@ struct __GLXdrawable { void (*waitX)(__GLXdrawable *); void (*waitGL)(__GLXdrawable *);
+ int refcnt; /* number of resources handles referencing this */ + DrawablePtr pDraw; XID drawId; + XID otherId; /* for glx1.3 we need to track the original Drawable as well */
/* ** Either GLX_DRAWABLE_PIXMAP, GLX_DRAWABLE_WINDOW or diff --git a/glx/glxext.c b/glx/glxext.c index 4bd5d6b..c473aa9 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -128,13 +128,18 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) * constructors, we added it as a glx drawable resource under both * its glx drawable ID and it X drawable ID. Remove the other * resource now so we don't a callback for freed memory. */ - if (glxPriv->drawId != glxPriv->pDraw->id) { - if (xid == glxPriv->drawId) - FreeResourceByType(glxPriv->pDraw->id, __glXDrawableRes, TRUE); - else - FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE); + if (glxPriv->otherId) { + XID other = glxPriv->otherId; + glxPriv->otherId = 0; + if (xid == other) + FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE); + else + FreeResourceByType(other, __glXDrawableRes, TRUE); }
+ if (--glxPriv->refcnt) + return TRUE; + for (c = glxAllContexts; c; c = next) { next = c->next; if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
On Fre, 2010-12-10 at 12:43 +0000, Chris Wilson wrote:
Although there may be more than one resource handles pointing to the Drawable, we only want to destroy it once and only reference the resource which may have just been deleted on the first instance.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Kristian Høgsberg krh@bitplanet.net Cc: Michel Dänzer daenzer@vmware.com
glx/glxcmds.c | 6 ++++++ glx/glxdrawable.h | 3 +++ glx/glxext.c | 15 ++++++++++----- 3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/glx/glxcmds.c b/glx/glxcmds.c index de9c3f0..fa153d5 100644 --- a/glx/glxcmds.c +++ b/glx/glxcmds.c @@ -1139,6 +1143,8 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, pGlxDraw->destroy (pGlxDraw); return BadAlloc; }
- pGlxDraw->refcnt++;
- pGlxDraw->otherId = pDraw->id;
I think this should use drawableId instead of pDraw->id, and refcnt should only be incremented if it's different from glxDrawableId. Combined with the patch I attached to https://bugs.freedesktop.org/show_bug.cgi?id=28181 this block could look something like
if (drawableId != glxDrawableId) { /* Add the glx drawable under the XID of the underlying X drawable * too. That way we'll get a callback in DrawableGone and can * clean up properly when the drawable is destroyed. */ if (!AddResource(drawableId, __glXDrawableRes, pGlxDraw)) { pGlxDraw->destroy (pGlxDraw); return BadAlloc; }
pGlxDraw->refcnt++; pGlxDraw->otherId = pDraw->id; }
Then this patch will hopefully address that bug and maybe others with references to DrawableGone. Fingers crossed. :)
diff --git a/glx/glxext.c b/glx/glxext.c index 4bd5d6b..c473aa9 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -128,13 +128,18 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) * constructors, we added it as a glx drawable resource under both * its glx drawable ID and it X drawable ID. Remove the other * resource now so we don't a callback for freed memory. */
- if (glxPriv->drawId != glxPriv->pDraw->id) {
- if (xid == glxPriv->drawId)
FreeResourceByType(glxPriv->pDraw->id, __glXDrawableRes, TRUE);- else
FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
if (glxPriv->otherId) {
XID other = glxPriv->otherId;glxPriv->otherId = 0;if (xid == other)FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);elseFreeResourceByType(other, __glXDrawableRes, TRUE);}
if (--glxPriv->refcnt)
return TRUE;
The return at the end of the function uses True instead of TRUE.
Although there may be more than one resource handles pointing to the Drawable, we only want to destroy it once and only reference the resource which may have just been deleted on the first instance.
v2: Apply fixes and combine with another bug fix from Michel Dänzer, https://bugs.freedesktop.org/show_bug.cgi?id=28181
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Kristian Høgsberg krh@bitplanet.net Cc: Michel Dänzer daenzer@vmware.com --- glx/glxcmds.c | 23 +++++++++++++++-------- glx/glxdrawable.h | 3 +++ glx/glxext.c | 15 ++++++++++----- 3 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/glx/glxcmds.c b/glx/glxcmds.c index de9c3f0..b3ea784 100644 --- a/glx/glxcmds.c +++ b/glx/glxcmds.c @@ -529,6 +529,7 @@ __glXGetDrawable(__GLXcontext *glxc, GLXDrawable drawId, ClientPtr client, *error = BadAlloc; return NULL; } + pGlxDraw->refcnt++;
return pGlxDraw; } @@ -1099,8 +1100,10 @@ __glXDrawableInit(__GLXdrawable *drawable, drawable->pDraw = pDraw; drawable->type = type; drawable->drawId = drawId; + drawable->otherId = 0; drawable->config = config; drawable->eventMask = 0; + drawable->refcnt = 0;
return GL_TRUE; } @@ -1130,14 +1133,18 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, pGlxDraw->destroy (pGlxDraw); return BadAlloc; } + pGlxDraw->refcnt++;
- /* Add the glx drawable under the XID of the underlying X drawable - * too. That way we'll get a callback in DrawableGone and can - * clean up properly when the drawable is destroyed. */ - if (drawableId != glxDrawableId && - !AddResource(pDraw->id, __glXDrawableRes, pGlxDraw)) { - pGlxDraw->destroy (pGlxDraw); - return BadAlloc; + if (drawableId != glxDrawableId) { + /* Add the glx drawable under the XID of the underlying X drawable + * too. That way we'll get a callback in DrawableGone and can + * clean up properly when the drawable is destroyed. */ + if (!AddResource(drawableId, __glXDrawableRes, pGlxDraw)) { + pGlxDraw->destroy (pGlxDraw); + return BadAlloc; + } + pGlxDraw->refcnt++; + pGlxDraw->otherId = drawableId; }
return Success; diff --git a/glx/glxdrawable.h b/glx/glxdrawable.h index 2a365c5..80c3234 100644 --- a/glx/glxdrawable.h +++ b/glx/glxdrawable.h @@ -51,8 +51,11 @@ struct __GLXdrawable { void (*waitX)(__GLXdrawable *); void (*waitGL)(__GLXdrawable *);
+ int refcnt; /* number of resources handles referencing this */ + DrawablePtr pDraw; XID drawId; + XID otherId; /* for glx1.3 we need to track the original Drawable as well */
/* ** Either GLX_DRAWABLE_PIXMAP, GLX_DRAWABLE_WINDOW or diff --git a/glx/glxext.c b/glx/glxext.c index 4bd5d6b..77db8b0 100644 --- a/glx/glxext.c +++ b/glx/glxext.c @@ -128,13 +128,18 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid) * constructors, we added it as a glx drawable resource under both * its glx drawable ID and it X drawable ID. Remove the other * resource now so we don't a callback for freed memory. */ - if (glxPriv->drawId != glxPriv->pDraw->id) { - if (xid == glxPriv->drawId) - FreeResourceByType(glxPriv->pDraw->id, __glXDrawableRes, TRUE); - else - FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE); + if (glxPriv->otherId) { + XID other = glxPriv->otherId; + glxPriv->otherId = 0; + if (xid == other) + FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE); + else + FreeResourceByType(other, __glXDrawableRes, TRUE); }
+ if (--glxPriv->refcnt) + return True; + for (c = glxAllContexts; c; c = next) { next = c->next; if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
On Fre, 2010-12-10 at 13:38 +0000, Chris Wilson wrote:
Although there may be more than one resource handles pointing to the Drawable, we only want to destroy it once and only reference the resource which may have just been deleted on the first instance.
v2: Apply fixes and combine with another bug fix from Michel Dänzer, https://bugs.freedesktop.org/show_bug.cgi?id=28181
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk Cc: Kristian Høgsberg krh@bitplanet.net Cc: Michel Dänzer daenzer@vmware.com
Reviewed-by: Michel Dänzer michel@daenzer.net
I'd wait for Kristian's review as well though.
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Michel Dänzer