[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