[PATCH] DRI2: Free DRI2 drawable references in DRI2DestroyDrawable

2010-10-25 Thread Pauli Nieminen 
If client calls DRI2CreateDrawable multiple times for same drawable
DRI2 creates multiple references. Multiple references cause DRI2 send
multiple invalidate events for same client.

Problem is easy to spot from xtrace. For example following filtered
snippet from problematic client:

000:<:0b85: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000:<:0b89: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:<:0b8e: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000:<:0b8f: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:<:0bc4: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000:<:0bc5: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0

Problem is triggered because client side EGL implementation is using
DRI2 directly.

DRI2 code in server doesn't handle DRI2DestroyDrawable that leaks
references. If client recreates rendering target EGL destroys and
creates DRI2 drawable. In that case DRI2DestroyDrawable leaks DRI2
drawable references.

To fix this memory leak/performance problem server has to free the
reference when client requests for it.

If client calls DRI2CreateDrawable twice for same drawable same
reference is reused and reference count is incremented.

Signed-off-by: Pauli Nieminen 
CC: Kristian Høgsberg 

---
 hw/xfree86/dri2/dri2.c|  100 +++-
 hw/xfree86/dri2/dri2.h|4 +-
 hw/xfree86/dri2/dri2ext.c |2 +-
 3 files changed, 83 insertions(+), 23 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 34f735f..103db1a 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -193,6 +193,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
 typedef struct DRI2DrawableRefRec {
 XID  id;
 XID  dri2_id;
+int  refcnt;
 DRI2InvalidateProcPtr  invalidate;
 void*priv;
 struct list  link;
@@ -229,6 +230,7 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
dri2_id,
 
 ref->id = id;
 ref->dri2_id = dri2_id; 
+ref->refcnt = 1;
 ref->invalidate = invalidate;
 ref->priv = priv;
 list_add(&ref->link, &pPriv->reference_list);
@@ -236,11 +238,24 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
dri2_id,
 return Success;
 }
 
+static DRI2DrawableRefPtr
+DRI2FindClientDrawableRef(ClientPtr client, DRI2DrawablePtr pPriv)
+{
+   DRI2DrawableRefPtr ref;
+
+   list_for_each_entry(ref, &pPriv->reference_list, link) {
+   if (CLIENT_ID(ref->dri2_id) == client->index)
+   return ref;
+   }
+   return NULL;
+}
+
 int
 DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID id,
   DRI2InvalidateProcPtr invalidate,

Re: [PATCH] DRI2: Free DRI2 drawable references in DRI2DestroyDrawable

2010-10-25 Thread Jesse Barnes 
On Mon, 25 Oct 2010 16:55:35 +0300
Pauli Nieminen  wrote:

> If client calls DRI2CreateDrawable multiple times for same drawable
> DRI2 creates multiple references. Multiple references cause DRI2 send
> multiple invalidate events for same client.

I've been burned by the drawable handling code too many times...  I
think Kristian touched it last, hopefully he can review.

-- 
Jesse Barnes, Intel Open Source Technology Center
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] DRI2: Free DRI2 drawable references in DRI2DestroyDrawable

2010-10-29 Thread Rami Ylimäki 

 From resource management point of view this patch seems correct to me.

Reviewed-by: Rami Ylimäki 

On 10/25/2010 04:55 PM, Pauli Nieminen wrote:

If client calls DRI2CreateDrawable multiple times for same drawable
DRI2 creates multiple references. Multiple references cause DRI2 send
multiple invalidate events for same client.

Problem is easy to spot from xtrace. For example following filtered
snippet from problematic client:

000:<:0b85: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000:<:0b89: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:<:0b8e: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000:<:0b8f: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
000:<:0bc4: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
attachments={attachment=BackLeft(0x0001)};
000:<:0bc5: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
remainder_lo=0

Problem is triggered because client side EGL implementation is using
DRI2 directly.

DRI2 code in server doesn't handle DRI2DestroyDrawable that leaks
references. If client recreates rendering target EGL destroys and
creates DRI2 drawable. In that case DRI2DestroyDrawable leaks DRI2
drawable references.

To fix this memory leak/performance problem server has to free the
reference when client requests for it.

If client calls DRI2CreateDrawable twice for same drawable same
reference is reused and reference count is incremented.

Signed-off-by: Pauli Nieminen
CC: Kristian Høgsberg

---
  hw/xfree86/dri2/dri2.c|  100 +++-
  hw/xfree86/dri2/dri2.h|4 +-
  hw/xfree86/dri2/dri2ext.c |2 +-
  3 files changed, 83 insertions(+), 23 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 34f735f..103db1a 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -193,6 +193,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
  typedef struct DRI2DrawableRefRec {
  XID id;
  XID dri2_id;
+int  refcnt;
  DRI2InvalidateProcPtr invalidate;
  void   *priv;
  struct list link;
@@ -229,6 +230,7 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
dri2_id,

  ref->id = id;
  ref->dri2_id = dri2_id;
+ref->refcnt = 1;
  ref->invalidate = invalidate;
  ref->priv = priv;
  list_add(&ref->link,&pPriv->reference_list);
@@ -236,11 +238,24 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
dri2_id,
  return Success;
  }

+static DRI2DrawableRefPtr
+DRI2FindClientDrawableRef(ClientPtr client, DRI2DrawablePtr pPriv)
+{
+   DRI2DrawableRefPtr ref;
+
+   list_for_each_entry(ref,&pPriv->reference_list, link) {
+   if (CLIENT_ID(ref->dri2_id) == client->index)
+   return ref;
+

Re: [PATCH] DRI2: Free DRI2 drawable references in DRI2DestroyDrawable

2010-10-29 Thread Kristian Høgsberg 
On Mon, Oct 25, 2010 at 9:55 AM, Pauli Nieminen
 wrote:
> If client calls DRI2CreateDrawable multiple times for same drawable
> DRI2 creates multiple references. Multiple references cause DRI2 send
> multiple invalidate events for same client.

Yup, there is a leak, but I think we should implement
DRI2DestroyDrawable by looking up the first DRI2DrawableRef where with
a dri2_id matching the client id and then call FreeResource(id, FALSE)
on that.

Kristian

> Problem is easy to spot from xtrace. For example following filtered
> snippet from problematic client:
>
> 000:<:0b85: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
> attachments={attachment=BackLeft(0x0001)};
> 000:<:0b89: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
> target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
> remainder_lo=0
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b89: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:<:0b8e: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
> attachments={attachment=BackLeft(0x0001)};
> 000:<:0b8f: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
> target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
> remainder_lo=0
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:>:0b8f: Event DRI2-InvalidateBuffers(67) drawable=0x00800011
> 000:<:0bc4: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
> attachments={attachment=BackLeft(0x0001)};
> 000:<:0bc5: 32: DRI2-Request(132,8): SwapBuffers drawable=0x00800011
> target_msc_hi=0 target_msc_lo=0 divisor_hi=0 divisor_lo=0 remainder_hi=0
> remainder_lo=0
>
> Problem is triggered because client side EGL implementation is using
> DRI2 directly.
>
> DRI2 code in server doesn't handle DRI2DestroyDrawable that leaks
> references. If client recreates rendering target EGL destroys and
> creates DRI2 drawable. In that case DRI2DestroyDrawable leaks DRI2
> drawable references.
>
> To fix this memory leak/performance problem server has to free the
> reference when client requests for it.
>
> If client calls DRI2CreateDrawable twice for same drawable same
> reference is reused and reference count is incremented.
>
> Signed-off-by: Pauli Nieminen 
> CC: Kristian Høgsberg 
>
> ---
>  hw/xfree86/dri2/dri2.c    |  100 +++-
>  hw/xfree86/dri2/dri2.h    |    4 +-
>  hw/xfree86/dri2/dri2ext.c |    2 +-
>  3 files changed, 83 insertions(+), 23 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 34f735f..103db1a 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -193,6 +193,7 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
>  typedef struct DRI2DrawableRefRec {
>     XID                  id;
>     XID                  dri2_id;
> +    int                  refcnt;
>     DRI2InvalidateProcPtr      invalidate;
>     void        *priv;
>     struct list          link;
> @@ -229,6 +230,7 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
> dri2_id,
>
>     ref->id = id;
>     ref->dri2_id = dri2_id;
> +    ref->refcnt = 1;
>     ref->invalidate = invalidate;
>     ref->priv = priv;
>     list_add(&ref->link, &pPriv->reference_list);
> @@ -236,11 +238,24 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID 
> dri2_id,
>     return Success;
>  }
>
> +static