[PATCH xserver 3/4] dri2: Only create one unnamed reference per Drawable per Client

[Ville Syrjälä]

On Wed, Feb 03, 2016 at 09:54:45AM +0000, Chris Wilson wrote:
> This workarounds issues in mesa and Mali that likes to create a new
> DRI2Drawble per context and per frame, respectively. The idea is that we
> only create one unnamed reference per Client on each Drawable that is
> never destroyed - and we make the assumption that the only caller is
> the DRI2 dispatcher and thus we don't need to check that the invalidate
> handler is the same. Every DRI2Drawable reference sends an invalidate
> event, and so not only do we have a slow memory leak, we also suffer a
> performance loss as the Clients create more and more references to the
> same Drawable - unless we limit them to a single reference each.
> 
> The issue was first reported 5^H 6 years ago by Pauli Nieminen and I have
> incorporated his patches here. His improvement was to add a reference
> count to the per-Client unnamed reference and by doing so could support
> DRI2DestroyDrawable again. However, it remains the case that as the
> lifecycles between the GLXDrawable (DRI2Drawable) and the parent
> Drawable are decoupled, mesa cannot call DRI2DestroyDrawable on
> Drawables it did not create (or else risk generating BadDrawable runtime
> errors), see mesa commit 4ebf07a426771b62123e5fcb5a8be0de24037af1
> Author: Kristian Høgsberg <krh at bitplanet.net>
> Date:   Mon Sep 13 08:39:42 2010 -0400
> 
>     glx: Don't destroy DRI2 drawables for legacy glx drawables
> 
>     For GLX 1.3 drawables, we can destroy the DRI2 drawable when the GLX
>     drawable is destroyed.  However, for legacy drawables, there os no
>     good way of knowing when the application is done with it, so we just
>     let the DRI2 drawable linger on the server.  The server will destroy
>     the DRI2 drawable when it destroys the X drawable or the client exits
>     anyway.
> 
>     https://bugs.freedesktop.org/show_bug.cgi?id=30109
> 
> and as such it rules out both using named references by the Clients and
> the reference ever being reaped.

Hmm. IIRC Pauli's solution avoided the BadDrawable by replacing
validDrawable() with something dri2 specific that looks things up in
the dri2 resource instead. And thus you could do the dri2DestroyDrawable
after the X drawable is gone without risking errors. So the dri2
drawable was more or less separate from the X drawable, except you
address both using the same X drawable id since the dri2 drawable
doesn't have its own id.

I suppose the main benefit of that is that the client stops getting
invalidate events for drawables it's no longer interested in. The
downside is that the dri2 drawable could then linger around after the
X drawable is gone if the client didn't explicitly destroy it.

What I can't recall is how it would have dealt with the client reusing
the drawable id after destroying the X drawable if the dri2 drawable
was still around. Maybe it didn't. Shame the dri2 drawable doesn't have
its own id.

> 
> Note Present incorporates its own version of this type of reference leak.
> 
> v2: Incorporate refcounting of the unnamed reference by Pauli Nieminen.
> 
> Cc: Daniel Drake <drake at endlessm.com>
> Link: https://freedesktop.org/patch/19695/
> Link: http://lists.x.org/archives/xorg-devel/2010-November/014783.html
> Link: http://lists.x.org/archives/xorg-devel/2010-November/014782.html
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  hw/xfree86/dri2/dri2.c    | 57 +++++++++++++++++++++++++++++++++++++++++++----
>  hw/xfree86/dri2/dri2ext.c |  4 ++--
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 4dc40f8..2f05c64 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -275,11 +275,24 @@ DRI2SwapLimit(DrawablePtr pDraw, int swap_limit)
>  
>  typedef struct DRI2DrawableRefRec {
>      XID pid;
> +    unsigned int refcnt;
>      DRI2InvalidateProcPtr invalidate;
>      void *priv;
>      struct xorg_list link;
>  } DRI2DrawableRefRec, *DRI2DrawableRefPtr;
>  
> +static DRI2DrawableRefPtr
> +DRI2FindClientDrawableRef(ClientPtr client, DRI2DrawablePtr pPriv)
> +{
> +    DRI2DrawableRefPtr ref;
> +
> +    xorg_list_for_each_entry(ref, &pPriv->reference_list, link)
> +        if (ref->refcnt && CLIENT_ID(ref->pid) == client->index)

So the refcnt check here prevents this from matching with the dri2
drawables created by glx, as those will have refcnt==0.

Looks sane enough to me. Though it's been years since I last looked at
this stuff, so not sure I even remember all the relevant details.
But anyways,
Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

> +            return ref;
> +
> +    return NULL;
> +}
> +
>  int
>  DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid,
>                     DRI2InvalidateProcPtr invalidate, void *priv)
> @@ -295,16 +308,34 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid,
>  
>      pPriv->prime_id = dri2ClientPrivate(client)->prime_id;
>  
> +    if (pid == 0) {
> +        ref = DRI2FindClientDrawableRef(client, pPriv);
> +        if (ref) {
> +            ref->refcnt++;
> +            assert(ref->invalidate == invalidate);
> +            assert(ref->priv == priv);
> +            return Success;
> +        }
> +    }
> +
>      ref = malloc(sizeof *ref);
>      if (ref == NULL)
>          return BadAlloc;
>  
> -    if (!AddResource(pid, dri2ReferenceRes, ref)) {
> +    if (pid == 0) {
> +        ref->refcnt = 1;
> +        ref->pid = FakeClientID(client->index);
> +    }
> +    else {
> +        ref->refcnt = 0;
> +        ref->pid = pid;
> +    }
> +
> +    if (!AddResource(ref->pid, dri2ReferenceRes, ref)) {
>          free(ref);
>          return BadAlloc;
>      }
>  
> -    ref->pid = pid;
>      ref->invalidate = invalidate;
>      ref->priv = priv;
>      xorg_list_add(&ref->link, &pPriv->reference_list);
> @@ -313,9 +344,27 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID pid,
>  }
>  
>  int
> -DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID id)
> +DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID pid)
>  {
> -    FreeResourceByType(id, dri2ReferenceRes, FALSE);
> +    if (pid == 0) {
> +        DRI2DrawablePtr pPriv;
> +        DRI2DrawableRefPtr ref;
> +
> +        pPriv = DRI2GetDrawable(pDraw);
> +        if (pPriv == NULL)
> +            return BadDrawable;
> +
> +        ref = DRI2FindClientDrawableRef(client, pPriv);
> +        if (ref == NULL)
> +            return BadMatch;
> +
> +        if (--ref->refcnt == 0)
> +            pid = ref->pid;
> +    }
> +
> +    if (pid != 0)
> +        FreeResourceByType(pid, dri2ReferenceRes, FALSE);
> +
>      return Success;
>  }
>  
> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index c42dcbc..4a73a39 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -185,7 +185,7 @@ ProcDRI2CreateDrawable(ClientPtr client)
>                         &pDrawable, &status))
>          return status;
>  
> -    status = DRI2CreateDrawable(client, pDrawable, FakeClientID(client->index),
> +    status = DRI2CreateDrawable(client, pDrawable, 0,
>                                  DRI2InvalidateBuffersEvent, client);
>      if (status != Success)
>          return status;
> @@ -205,7 +205,7 @@ ProcDRI2DestroyDrawable(ClientPtr client)
>                         &pDrawable, &status))
>          return status;
>  
> -    return Success;
> +    return DRI2DestroyDrawable(client, pDrawable, 0);
>  }
>  
>  static int
> -- 
> 2.7.0

-- 
Ville Syrjälä
Intel OTC