Hi Michel, I spent a good deal of time avoiding using resources in dri2.c since we should be able to just use pointers directly from aiglx clients. There should be a simpler way to fix this that doesn't use resources or clients in dri2.c. Keeping drawable resource tracking out of dri2.c was a deliberate choice and I don't want to see that reverted unless there is no other way. The problem is that the DRI2Drawable is a client side resource for direct rendering clients (they get to it through dri2ext.c) but for aiglx clients, it's a server internal object that is not exposed as a client resource.
cheers, Kristian 2009/8/2 Michel Dänzer <[email protected]>: > From: Michel Dänzer <[email protected]> > > * Add new variants of DRI2CreateDrawable, DRI2DestroyDrawable and > DRI2CopyRegion which take a ClientPtr and an XID instead of a DrawablePtr. > Use these in dri2ext.c always and in glxdri2.c for windows (not all pixmaps > have an associated XID, and we can increase their reference count to prevent > them from disappearing beneath us) to avoid derefencing a DrawablePtr for a > window which has been destroyed. Fixes crashes when quitting MacSlow's > rgba-glx demo. > * Move DRI2 drawable resource tracking from dri2ext.c to dri2.c, and add > screen > DestroyPixmap/Window hooks to prevent DRI2 drawable leaks under any > circumstances. > --- > glx/glxcmds.c | 26 ++++----- > glx/glxdrawable.h | 1 + > glx/glxdri2.c | 39 ++++++++---- > hw/xfree86/dri2/dri2.c | 145 > +++++++++++++++++++++++++++++++++++++++++++-- > hw/xfree86/dri2/dri2.h | 8 +++ > hw/xfree86/dri2/dri2ext.c | 32 ++-------- > 6 files changed, 191 insertions(+), 60 deletions(-) > > diff --git a/glx/glxcmds.c b/glx/glxcmds.c > index f5632d1..5112303 100644 > --- a/glx/glxcmds.c > +++ b/glx/glxcmds.c > @@ -1085,7 +1085,8 @@ __glXDrawableInit(__GLXdrawable *drawable, > __GLXscreen *screen, DrawablePtr pDraw, int type, > XID drawId, __GLXconfig *config) > { > - drawable->pDraw = pDraw; > + drawable->pScreen = pDraw->pScreen; > + drawable->pDraw = pDraw->type == DRAWABLE_PIXMAP ? pDraw : NULL; > drawable->type = type; > drawable->drawId = drawId; > drawable->config = config; > @@ -1097,19 +1098,14 @@ __glXDrawableInit(__GLXdrawable *drawable, > void > __glXDrawableRelease(__GLXdrawable *drawable) > { > - ScreenPtr pScreen = drawable->pDraw->pScreen; > - > - switch (drawable->type) { > - case GLX_DRAWABLE_PIXMAP: > - case GLX_DRAWABLE_PBUFFER: > - (*pScreen->DestroyPixmap)((PixmapPtr) drawable->pDraw); > - break; > - } > + /* We only store the DrawablePtr for pixmaps */ > + if (drawable->pDraw) > + (*drawable->pScreen->DestroyPixmap)((PixmapPtr) drawable->pDraw); > } > > static int > DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen, __GLXconfig > *config, > - DrawablePtr pDraw, XID glxDrawableId, int type) > + DrawablePtr pDraw, XID drawableId, XID glxDrawableId, int > type) > { > __GLXdrawable *pGlxDraw; > > @@ -1119,7 +1115,7 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen > *pGlxScreen, __GLXconfig *conf > return BadMatch; > > pGlxDraw = pGlxScreen->createDrawable(pGlxScreen, pDraw, type, > - glxDrawableId, config); > + drawableId, config); > if (pGlxDraw == NULL) > return BadAlloc; > > @@ -1148,7 +1144,7 @@ DoCreateGLXPixmap(ClientPtr client, __GLXscreen > *pGlxScreen, __GLXconfig *config > return BadPixmap; > } > > - err = DoCreateGLXDrawable(client, pGlxScreen, config, pDraw, > + err = DoCreateGLXDrawable(client, pGlxScreen, config, pDraw, drawableId, > glxDrawableId, GLX_DRAWABLE_PIXMAP); > > if (err == Success) > @@ -1305,7 +1301,7 @@ DoCreatePbuffer(ClientPtr client, int screenNum, XID > fbconfigId, > __glXleaveServer(GL_FALSE); > > return DoCreateGLXDrawable(client, pGlxScreen, config, &pPixmap->drawable, > - glxDrawableId, GLX_DRAWABLE_PBUFFER); > + glxDrawableId, glxDrawableId, > GLX_DRAWABLE_PBUFFER); > } > > int __glXDisp_CreatePbuffer(__GLXclientState *cl, GLbyte *pc) > @@ -1425,8 +1421,8 @@ int __glXDisp_CreateWindow(__GLXclientState *cl, GLbyte > *pc) > if (!validGlxFBConfigForWindow(client, config, pDraw, &err)) > return err; > > - return DoCreateGLXDrawable(client, pGlxScreen, config, > - pDraw, req->glxwindow, GLX_DRAWABLE_WINDOW); > + return DoCreateGLXDrawable(client, pGlxScreen, config, pDraw, > + req->window, req->glxwindow, > GLX_DRAWABLE_WINDOW); > } > > int __glXDisp_DestroyWindow(__GLXclientState *cl, GLbyte *pc) > diff --git a/glx/glxdrawable.h b/glx/glxdrawable.h > index 3f165ed..922eefd 100644 > --- a/glx/glxdrawable.h > +++ b/glx/glxdrawable.h > @@ -51,6 +51,7 @@ struct __GLXdrawable { > void (*waitX)(__GLXdrawable *); > void (*waitGL)(__GLXdrawable *); > > + ScreenPtr pScreen; > DrawablePtr pDraw; > XID drawId; > > diff --git a/glx/glxdri2.c b/glx/glxdri2.c > index ed7fb4c..e8fde0d 100644 > --- a/glx/glxdri2.c > +++ b/glx/glxdri2.c > @@ -104,10 +104,10 @@ __glXDRIdrawableDestroy(__GLXdrawable *drawable) > > (*core->destroyDrawable)(private->driDrawable); > > - /* If the X window was destroyed, the dri DestroyWindow hook will > - * aready have taken care of this, so only call if pDraw isn't NULL. */ > - if (drawable->pDraw != NULL) > + if (drawable->pDraw) > DRI2DestroyDrawable(drawable->pDraw); > + else > + DRI2DestroyDrawableWithId(__pGlxClient, drawable->drawId); > > __glXDrawableRelease(drawable); > > @@ -126,10 +126,14 @@ __glXDRIdrawableCopySubBuffer(__GLXdrawable *drawable, > box.y1 = private->height - y - h; > box.x2 = x + w; > box.y2 = private->height - y; > - REGION_INIT(drawable->pDraw->pScreen, ®ion, &box, 0); > + REGION_INIT(drawable->pScreen, ®ion, &box, 0); > > - DRI2CopyRegion(drawable->pDraw, ®ion, > - DRI2BufferFrontLeft, DRI2BufferBackLeft); > + if (drawable->pDraw) > + DRI2CopyRegion(drawable->pDraw, ®ion, > + DRI2BufferFrontLeft, DRI2BufferBackLeft); > + else > + DRI2CopyRegionWithId(__pGlxClient, drawable->drawId, ®ion, > + DRI2BufferFrontLeft, DRI2BufferBackLeft); > } > > static GLboolean > @@ -154,10 +158,14 @@ __glXDRIdrawableWaitX(__GLXdrawable *drawable) > box.y1 = 0; > box.x2 = private->width; > box.y2 = private->height; > - REGION_INIT(drawable->pDraw->pScreen, ®ion, &box, 0); > + REGION_INIT(drawable->pScreen, ®ion, &box, 0); > > - DRI2CopyRegion(drawable->pDraw, ®ion, > - DRI2BufferFakeFrontLeft, DRI2BufferFrontLeft); > + if (drawable->pDraw) > + DRI2CopyRegion(drawable->pDraw, ®ion, > + DRI2BufferFakeFrontLeft, DRI2BufferFrontLeft); > + else > + DRI2CopyRegionWithId(__pGlxClient, drawable->drawId, ®ion, > + DRI2BufferFakeFrontLeft, DRI2BufferFrontLeft); > } > > static void > @@ -171,10 +179,14 @@ __glXDRIdrawableWaitGL(__GLXdrawable *drawable) > box.y1 = 0; > box.x2 = private->width; > box.y2 = private->height; > - REGION_INIT(drawable->pDraw->pScreen, ®ion, &box, 0); > + REGION_INIT(drawable->pScreen, ®ion, &box, 0); > > - DRI2CopyRegion(drawable->pDraw, ®ion, > - DRI2BufferFrontLeft, DRI2BufferFakeFrontLeft); > + if (drawable->pDraw) > + DRI2CopyRegion(drawable->pDraw, ®ion, > + DRI2BufferFrontLeft, DRI2BufferFakeFrontLeft); > + else > + DRI2CopyRegionWithId(__pGlxClient, drawable->drawId, ®ion, > + DRI2BufferFrontLeft, DRI2BufferFakeFrontLeft); > } > > static int > @@ -387,7 +399,8 @@ __glXDRIscreenCreateDrawable(__GLXscreen *screen, > private->base.waitGL = __glXDRIdrawableWaitGL; > private->base.waitX = __glXDRIdrawableWaitX; > > - if (DRI2CreateDrawable(pDraw)) { > + if (private->base.pDraw ? DRI2CreateDrawable(private->base.pDraw) : > + DRI2CreateDrawableWithId(__pGlxClient, private->base.drawId)) { > xfree(private); > return NULL; > } > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c > index 8795cd1..d054b8a 100644 > --- a/hw/xfree86/dri2/dri2.c > +++ b/hw/xfree86/dri2/dri2.c > @@ -42,6 +42,18 @@ > > #include "xf86.h" > > +extern RESTYPE dri2DrawableRes; > + > +static Bool > +validDrawable(ClientPtr client, XID drawable, DrawablePtr *pDrawable, int > *status) > +{ > + *status = dixLookupDrawable(pDrawable, drawable, client, 0, > DixReadAccess); > + if (*status != Success) > + return FALSE; > + > + return TRUE; > +} > + > static int dri2ScreenPrivateKeyIndex; > static DevPrivateKey dri2ScreenPrivateKey = &dri2ScreenPrivateKeyIndex; > static int dri2WindowPrivateKeyIndex; > @@ -68,6 +80,8 @@ typedef struct _DRI2Screen { > DRI2DestroyBufferProcPtr DestroyBuffer; > DRI2CopyRegionProcPtr CopyRegion; > > + DestroyPixmapProcPtr DestroyPixmap; > + DestroyWindowProcPtr DestroyWindow; > HandleExposuresProcPtr HandleExposures; > } DRI2ScreenRec, *DRI2ScreenPtr; > > @@ -133,6 +147,27 @@ DRI2CreateDrawable(DrawablePtr pDraw) > return Success; > } > > +int > +DRI2CreateDrawableWithId(ClientPtr client, XID xDrawable) > +{ > + DrawablePtr pDraw; > + int status; > + > + if (!validDrawable(client, xDrawable, &pDraw, &status)) > + return status; > + > + status = DRI2CreateDrawable(pDraw); > + if (status != Success) > + return status; > + > + if (!AddResource(xDrawable, dri2DrawableRes, client)) { > + DRI2DestroyDrawable(pDraw); > + return BadAlloc; > + } > + > + return Success; > +} > + > static int > find_attachment(DRI2DrawablePtr pPriv, unsigned attachment) > { > @@ -337,8 +372,21 @@ DRI2CopyRegion(DrawablePtr pDraw, RegionPtr pRegion, > return Success; > } > > -void > -DRI2DestroyDrawable(DrawablePtr pDraw) > +int > +DRI2CopyRegionWithId(ClientPtr client, XID xDrawable, RegionPtr pRegion, > + unsigned int dest, unsigned int src) > +{ > + DrawablePtr pDraw; > + int status; > + > + if (!validDrawable(client, xDrawable, &pDraw, &status)) > + return status; > + > + return DRI2CopyRegion(pDraw, pRegion, dest, src); > +} > + > +static void > +DRI2DoDestroyDrawable(DrawablePtr pDraw) > { > DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen); > DRI2DrawablePtr pPriv; > @@ -349,10 +397,6 @@ DRI2DestroyDrawable(DrawablePtr pDraw) > if (pPriv == NULL) > return; > > - pPriv->refCount--; > - if (pPriv->refCount > 0) > - return; > - > if (pPriv->buffers != NULL) { > int i; > > @@ -376,6 +420,87 @@ DRI2DestroyDrawable(DrawablePtr pDraw) > } > } > > +void > +DRI2DestroyDrawable(DrawablePtr pDraw) > +{ > + DRI2DrawablePtr pPriv; > + > + pPriv = DRI2GetDrawable(pDraw); > + if (pPriv == NULL) > + return; > + > + pPriv->refCount--; > + if (pPriv->refCount > 0) > + return; > + > + DRI2DoDestroyDrawable(pDraw); > +} > + > +int > +DRI2DestroyDrawableWithId(ClientPtr client, XID xDrawable) > +{ > + DrawablePtr pDraw; > + int status; > + > + if (!validDrawable(client, xDrawable, &pDraw, &status)) > + return status; > + > + DRI2DestroyDrawable(pDraw); > + > + return Success; > +} > + > +static Bool > +DRI2DestroyPixmap (PixmapPtr pPixmap) > +{ > + ScreenPtr pScreen = pPixmap->drawable.pScreen; > + DRI2ScreenPtr ds = DRI2GetScreen(pScreen); > + Bool retval = TRUE; > + > + if (pPixmap->refcnt == 1) > + DRI2DoDestroyDrawable(&pPixmap->drawable); > + > + /* call lower wrapped functions */ > + if (ds->DestroyPixmap) { > + /* unwrap */ > + pScreen->DestroyPixmap = ds->DestroyPixmap; > + > + /* call lower layers */ > + retval = (*pScreen->DestroyPixmap)(pPixmap); > + > + /* rewrap */ > + ds->DestroyPixmap = pScreen->DestroyPixmap; > + pScreen->DestroyPixmap = DRI2DestroyPixmap; > + } > + > + return retval; > +} > + > +static Bool > +DRI2DestroyWindow(WindowPtr pWin) > +{ > + ScreenPtr pScreen = pWin->drawable.pScreen; > + DRI2ScreenPtr ds = DRI2GetScreen(pScreen); > + Bool retval = TRUE; > + > + DRI2DoDestroyDrawable(&pWin->drawable); > + > + /* call lower wrapped functions */ > + if (ds->DestroyWindow) { > + /* unwrap */ > + pScreen->DestroyWindow = ds->DestroyWindow; > + > + /* call lower layers */ > + retval = (*pScreen->DestroyWindow)(pWin); > + > + /* rewrap */ > + ds->DestroyWindow = pScreen->DestroyWindow; > + pScreen->DestroyWindow = DRI2DestroyWindow; > + } > + > + return retval; > +} > + > Bool > DRI2Connect(ScreenPtr pScreen, unsigned int driverType, int *fd, > const char **driverName, const char **deviceName) > @@ -426,6 +551,11 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info) > ds->DestroyBuffer = info->DestroyBuffer; > ds->CopyRegion = info->CopyRegion; > > + ds->DestroyPixmap = pScreen->DestroyPixmap; > + pScreen->DestroyPixmap = DRI2DestroyPixmap; > + ds->DestroyWindow = pScreen->DestroyWindow; > + pScreen->DestroyWindow = DRI2DestroyWindow; > + > dixSetPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey, ds); > > xf86DrvMsg(pScreen->myNum, X_INFO, "[DRI2] Setup complete\n"); > @@ -438,6 +568,9 @@ DRI2CloseScreen(ScreenPtr pScreen) > { > DRI2ScreenPtr ds = DRI2GetScreen(pScreen); > > + pScreen->DestroyPixmap = ds->DestroyPixmap; > + pScreen->DestroyWindow = ds->DestroyWindow; > + > xfree(ds); > dixSetPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey, NULL); > } > diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h > index 175471a..cc4bbcb 100644 > --- a/hw/xfree86/dri2/dri2.h > +++ b/hw/xfree86/dri2/dri2.h > @@ -100,8 +100,10 @@ extern _X_EXPORT Bool DRI2Connect(ScreenPtr pScreen, > extern _X_EXPORT Bool DRI2Authenticate(ScreenPtr pScreen, drm_magic_t magic); > > extern _X_EXPORT int DRI2CreateDrawable(DrawablePtr pDraw); > +extern _X_EXPORT int DRI2CreateDrawableWithId(ClientPtr client, XID > drawable); > > extern _X_EXPORT void DRI2DestroyDrawable(DrawablePtr pDraw); > +extern _X_EXPORT int DRI2DestroyDrawableWithId(ClientPtr client, XID > drawable); > > extern _X_EXPORT DRI2BufferPtr *DRI2GetBuffers(DrawablePtr pDraw, > int *width, > @@ -115,6 +117,12 @@ extern _X_EXPORT int DRI2CopyRegion(DrawablePtr pDraw, > unsigned int dest, > unsigned int src); > > +extern _X_EXPORT int DRI2CopyRegionWithId(ClientPtr client, > + XID xDrawable, > + RegionPtr pRegion, > + unsigned int dest, > + unsigned int src); > + > /** > * Determine the major and minor version of the DRI2 extension. > * > diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c > index 029dce8..fe5de7d 100644 > --- a/hw/xfree86/dri2/dri2ext.c > +++ b/hw/xfree86/dri2/dri2ext.c > @@ -50,7 +50,7 @@ > #include "xf86Module.h" > > static ExtensionEntry *dri2Extension; > -static RESTYPE dri2DrawableRes; > +RESTYPE dri2DrawableRes; > > static Bool > validDrawable(ClientPtr client, XID drawable, > @@ -156,23 +156,14 @@ static int > ProcDRI2CreateDrawable(ClientPtr client) > { > REQUEST(xDRI2CreateDrawableReq); > - DrawablePtr pDrawable; > int status; > > REQUEST_SIZE_MATCH(xDRI2CreateDrawableReq); > > - if (!validDrawable(client, stuff->drawable, &pDrawable, &status)) > - return status; > - > - status = DRI2CreateDrawable(pDrawable); > + status = DRI2CreateDrawableWithId(client, stuff->drawable); > if (status != Success) > return status; > > - if (!AddResource(stuff->drawable, dri2DrawableRes, pDrawable)) { > - DRI2DestroyDrawable(pDrawable); > - return BadAlloc; > - } > - > return client->noClientException; > } > > @@ -180,16 +171,10 @@ static int > ProcDRI2DestroyDrawable(ClientPtr client) > { > REQUEST(xDRI2DestroyDrawableReq); > - DrawablePtr pDrawable; > - int status; > > REQUEST_SIZE_MATCH(xDRI2DestroyDrawableReq); > - if (!validDrawable(client, stuff->drawable, &pDrawable, &status)) > - return status; > - > - FreeResourceByType(stuff->drawable, dri2DrawableRes, FALSE); > > - return client->noClientException; > + return DRI2DestroyDrawableWithId(client, stuff->drawable); > } > > > @@ -290,18 +275,15 @@ ProcDRI2CopyRegion(ClientPtr client) > { > REQUEST(xDRI2CopyRegionReq); > xDRI2CopyRegionReply rep; > - DrawablePtr pDrawable; > int status; > RegionPtr pRegion; > > REQUEST_SIZE_MATCH(xDRI2CopyRegionReq); > > - if (!validDrawable(client, stuff->drawable, &pDrawable, &status)) > - return status; > - > VERIFY_REGION(pRegion, stuff->region, client, DixReadAccess); > > - status = DRI2CopyRegion(pDrawable, pRegion, stuff->dest, stuff->src); > + status = DRI2CopyRegionWithId(client, stuff->drawable, pRegion, > stuff->dest, > + stuff->src); > if (status != Success) > return status; > > @@ -398,9 +380,7 @@ SProcDRI2Dispatch (ClientPtr client) > > static int DRI2DrawableGone(pointer p, XID id) > { > - DrawablePtr pDrawable = p; > - > - DRI2DestroyDrawable(pDrawable); > + DRI2DestroyDrawableWithId(p, id); > > return Success; > } > -- > 1.6.3.3 > > _______________________________________________ > xorg-devel mailing list > [email protected] > http://lists.x.org/mailman/listinfo/xorg-devel > _______________________________________________ xorg-devel mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-devel
