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

[Chris Wilson]

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.

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)
+            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