Keith, have you had time to look at this respun fix for 26394?
2010/4/9 Kristian Høgsberg <[email protected]>: > The main motivation here is to have the resource system clean up the > DRI2 drawable automatically so glx doesn't have to. Right now, the > glx drawable resource must be destroyed before the X drawable, so that > calling DRI2DestroyDrawable doesn't crash. By making the DRI2 > drawable a resource, GLX doesn't have to worry about that and the > resource destruction order becomes irrelevant. > > https://bugs.freedesktop.org/show_bug.cgi?id=26394 > > Signed-off-by: Kristian Høgsberg <[email protected]> > --- > > Here's a different approach to fixing 26394. It's more invasive and there's > a little piece of uglyness in the DRI2DrawableGone implementation. We > pass the root window to DestroyBuffer instead of the actual drawable, since > the drawable may have been freed at this point. The driver only uses the > drawable to lookup the screen anyway, but it is a change in the DRI2 API > semantics. > > glx/glxdri2.c | 5 -- > hw/xfree86/dri2/dri2.c | 139 ++++++++++++-------------------------------- > hw/xfree86/dri2/dri2ext.c | 22 ------- > 3 files changed, 38 insertions(+), 128 deletions(-) > > diff --git a/glx/glxdri2.c b/glx/glxdri2.c > index e791bf6..8c883b0 100644 > --- a/glx/glxdri2.c > +++ b/glx/glxdri2.c > @@ -105,11 +105,6 @@ __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) > - DRI2DestroyDrawable(drawable->pDraw); > - > __glXDrawableRelease(drawable); > > xfree(private); > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c > index 2bdb733..6c4dabc 100644 > --- a/hw/xfree86/dri2/dri2.c > +++ b/hw/xfree86/dri2/dri2.c > @@ -48,15 +48,14 @@ > CARD8 dri2_major; /* version of DRI2 supported by DDX */ > CARD8 dri2_minor; > > -static int dri2ScreenPrivateKeyIndex; > +static int dri2ScreenPrivateKeyIndex; > static DevPrivateKey dri2ScreenPrivateKey = &dri2ScreenPrivateKeyIndex; > -static int dri2WindowPrivateKeyIndex; > -static DevPrivateKey dri2WindowPrivateKey = &dri2WindowPrivateKeyIndex; > -static int dri2PixmapPrivateKeyIndex; > -static DevPrivateKey dri2PixmapPrivateKey = &dri2PixmapPrivateKeyIndex; > +static RESTYPE dri2DrawableRes; > + > +typedef struct _DRI2Screen *DRI2ScreenPtr; > > typedef struct _DRI2Drawable { > - unsigned int refCount; > + DRI2ScreenPtr dri2_screen; > int width; > int height; > DRI2BufferPtr *buffers; > @@ -73,9 +72,8 @@ typedef struct _DRI2Drawable { > int swap_limit; /* for N-buffering */ > } DRI2DrawableRec, *DRI2DrawablePtr; > > -typedef struct _DRI2Screen *DRI2ScreenPtr; > - > typedef struct _DRI2Screen { > + ScreenPtr screen; > unsigned int numDrivers; > const char **driverNames; > const char *deviceName; > @@ -101,45 +99,35 @@ DRI2GetScreen(ScreenPtr pScreen) > static DRI2DrawablePtr > DRI2GetDrawable(DrawablePtr pDraw) > { > - WindowPtr pWin; > - PixmapPtr pPixmap; > + DRI2DrawablePtr pPriv; > + int rc; > > - if (!pDraw) > + rc = dixLookupResourceByType((pointer *) &pPriv, pDraw->id, > + dri2DrawableRes, NULL, DixReadAccess); > + if (rc != Success) > return NULL; > > - if (pDraw->type == DRAWABLE_WINDOW) > - { > - pWin = (WindowPtr) pDraw; > - return dixLookupPrivate(&pWin->devPrivates, dri2WindowPrivateKey); > - } > - else > - { > - pPixmap = (PixmapPtr) pDraw; > - return dixLookupPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey); > - } > + return pPriv; > } > > int > DRI2CreateDrawable(DrawablePtr pDraw) > { > DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen); > - WindowPtr pWin; > - PixmapPtr pPixmap; > DRI2DrawablePtr pPriv; > CARD64 ust; > + int rc; > > - pPriv = DRI2GetDrawable(pDraw); > - if (pPriv != NULL) > - { > - pPriv->refCount++; > - return Success; > - } > + rc = dixLookupResourceByType((pointer *) &pPriv, pDraw->id, > + dri2DrawableRes, NULL, DixReadAccess); > + if (rc == Success || rc != BadValue) > + return rc; > > pPriv = xalloc(sizeof *pPriv); > if (pPriv == NULL) > return BadAlloc; > > - pPriv->refCount = 1; > + pPriv->dri2_screen = ds; > pPriv->width = pDraw->width; > pPriv->height = pDraw->height; > pPriv->buffers = NULL; > @@ -158,43 +146,30 @@ DRI2CreateDrawable(DrawablePtr pDraw) > pPriv->last_swap_msc = 0; > pPriv->last_swap_ust = 0; > > - if (pDraw->type == DRAWABLE_WINDOW) > - { > - pWin = (WindowPtr) pDraw; > - dixSetPrivate(&pWin->devPrivates, dri2WindowPrivateKey, pPriv); > - } > - else > - { > - pPixmap = (PixmapPtr) pDraw; > - dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, pPriv); > - } > + if (!AddResource(pDraw->id, dri2DrawableRes, pPriv)) > + return BadAlloc; > > return Success; > } > > -static void > -DRI2FreeDrawable(DrawablePtr pDraw) > +static int DRI2DrawableGone(pointer p, XID id) > { > - DRI2DrawablePtr pPriv; > - WindowPtr pWin; > - PixmapPtr pPixmap; > + DRI2DrawablePtr pPriv = p; > + DRI2ScreenPtr ds = pPriv->dri2_screen; > + DrawablePtr root; > + int i; > > - pPriv = DRI2GetDrawable(pDraw); > - if (pPriv == NULL) > - return; > + root = &WindowTable[ds->screen->myNum]->drawable; > + if (pPriv->buffers != NULL) { > + for (i = 0; i < pPriv->bufferCount; i++) > + (*ds->DestroyBuffer)(root, pPriv->buffers[i]); > + > + xfree(pPriv->buffers); > + } > > xfree(pPriv); > > - if (pDraw->type == DRAWABLE_WINDOW) > - { > - pWin = (WindowPtr) pDraw; > - dixSetPrivate(&pWin->devPrivates, dri2WindowPrivateKey, NULL); > - } > - else > - { > - pPixmap = (PixmapPtr) pDraw; > - dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, NULL); > - } > + return Success; > } > > static int > @@ -505,10 +480,6 @@ DRI2WaitMSCComplete(ClientPtr client, DrawablePtr pDraw, > int frame, > > pPriv->blockedClient = NULL; > pPriv->blockedOnMsc = FALSE; > - > - /* If there's still a swap pending, let DRI2SwapComplete free it */ > - if (pPriv->refCount == 0 && pPriv->swapsPending == 0) > - DRI2FreeDrawable(pDraw); > } > > static void > @@ -576,13 +547,6 @@ DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, > int frame, > pPriv->last_swap_ust = ust; > > DRI2WakeClient(client, pDraw, frame, tv_sec, tv_usec); > - > - /* > - * It's normal for the app to have exited with a swap outstanding, but > - * don't free the drawable until they're all complete. > - */ > - if (pPriv->swapsPending == 0 && pPriv->refCount == 0) > - DRI2FreeDrawable(pDraw); > } > > Bool > @@ -750,7 +714,7 @@ DRI2WaitMSC(ClientPtr client, DrawablePtr pDraw, CARD64 > target_msc, > Bool ret; > > pPriv = DRI2GetDrawable(pDraw); > - if (pPriv == NULL || pPriv->refCount == 0) > + if (pPriv == NULL) > return BadDrawable; > > /* Old DDX just completes immediately */ > @@ -774,7 +738,7 @@ DRI2WaitSBC(ClientPtr client, DrawablePtr pDraw, CARD64 > target_sbc, > DRI2DrawablePtr pPriv; > > pPriv = DRI2GetDrawable(pDraw); > - if (pPriv == NULL || pPriv->refCount == 0) > + if (pPriv == NULL) > return BadDrawable; > > /* target_sbc == 0 means to block until all pending swaps are > @@ -800,36 +764,6 @@ DRI2WaitSBC(ClientPtr client, DrawablePtr pDraw, CARD64 > target_sbc, > return Success; > } > > -void > -DRI2DestroyDrawable(DrawablePtr pDraw) > -{ > - DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen); > - DRI2DrawablePtr pPriv; > - > - pPriv = DRI2GetDrawable(pDraw); > - if (pPriv == NULL) > - return; > - > - pPriv->refCount--; > - if (pPriv->refCount > 0) > - return; > - > - if (pPriv->buffers != NULL) { > - int i; > - > - for (i = 0; i < pPriv->bufferCount; i++) > - (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]); > - > - xfree(pPriv->buffers); > - } > - > - /* If the window is destroyed while we have a swap or wait pending, don't > - * actually free the priv yet. We'll need it in the DRI2SwapComplete() > - * callback and we'll free it there once we're done. */ > - if (!pPriv->swapsPending && !pPriv->blockedClient) > - DRI2FreeDrawable(pDraw); > -} > - > Bool > DRI2HasSwapControl(ScreenPtr pScreen) > { > @@ -890,6 +824,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info) > if (!ds) > return FALSE; > > + ds->screen = pScreen; > ds->fd = info->fd; > ds->deviceName = info->deviceName; > dri2_major = 1; > @@ -961,6 +896,8 @@ DRI2Setup(pointer module, pointer opts, int *errmaj, int > *errmin) > { > static Bool setupDone = FALSE; > > + dri2DrawableRes = CreateNewResourceType(DRI2DrawableGone, > "DRI2Drawable"); > + > if (!setupDone) > { > setupDone = TRUE; > diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c > index 094d54d..17df130 100644 > --- a/hw/xfree86/dri2/dri2ext.c > +++ b/hw/xfree86/dri2/dri2ext.c > @@ -51,7 +51,6 @@ > #include "xf86Module.h" > > static ExtensionEntry *dri2Extension; > -static RESTYPE dri2DrawableRes; > > static Bool > validDrawable(ClientPtr client, XID drawable, Mask access_mode, > @@ -172,11 +171,6 @@ ProcDRI2CreateDrawable(ClientPtr client) > if (status != Success) > return status; > > - if (!AddResource(stuff->drawable, dri2DrawableRes, pDrawable)) { > - DRI2DestroyDrawable(pDrawable); > - return BadAlloc; > - } > - > return client->noClientException; > } > > @@ -192,8 +186,6 @@ ProcDRI2DestroyDrawable(ClientPtr client) > &pDrawable, &status)) > return status; > > - FreeResourceByType(stuff->drawable, dri2DrawableRes, FALSE); > - > return client->noClientException; > } > > @@ -627,25 +619,11 @@ SProcDRI2Dispatch (ClientPtr client) > } > } > > -static int DRI2DrawableGone(pointer p, XID id) > -{ > - DrawablePtr pDrawable = p; > - > - DRI2DestroyDrawable(pDrawable); > - > - return Success; > -} > - > int DRI2EventBase; > > static void > DRI2ExtensionInit(void) > { > - dri2DrawableRes = CreateNewResourceType(DRI2DrawableGone, > "DRI2Drawable"); > - > - if (!dri2DrawableRes) > - return; > - > dri2Extension = AddExtension(DRI2_NAME, > DRI2NumberEvents, > DRI2NumberErrors, > -- > 1.7.0.1 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
