[PATCHv2 1/2] DRI2: Track DRI2 drawables as resources, not privates
Kristian Høgsberg
krh at bitplanet.net
Wed Apr 14 07:25:49 PDT 2010
Keith, have you had time to look at this respun fix for 26394?
2010/4/9 Kristian Høgsberg <krh at bitplanet.net>:
> 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 <krh at bitplanet.net>
> ---
>
> 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
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
More information about the xorg-devel
mailing list