[PATCH v2 1/2] DRI2: Free DRI2 drawable references in DRI2DestroyDrawable

[Pauli Nieminen]

Ping.

On 01/11/10 12:22 +0100, Nieminen Pauli (EXT-Collabora/Helsinki) 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 see in xtrace. Like following filtered snippet from
> problematic client:
> 000:<:0b85: 16: DRI2-Request(132,5): GetBuffers drawable=0x00800011
> attachments={attachment=BackLeft(0x00000001)};
> 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(0x00000001)};
> 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(0x00000001)};
> 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.
> 
> V2:
> * Simplified DRI2DestroyDrawable based on Kristian's review
> * Split refcnt to different patch
> 
> Signed-off-by: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
> CC: Rami Ylimäki <rami.ylimaki at vincit.fi>
> CC: Kristian Høgsberg <krh at bitplanet.net>
> 
> ---
>  hw/xfree86/dri2/dri2.c    |   31 +++++++++++++++++++++++++++++++
>  hw/xfree86/dri2/dri2.h    |    4 +++-
>  hw/xfree86/dri2/dri2ext.c |    2 +-
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index 34f735f..75af3ea 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -236,6 +236,18 @@ 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, void *priv)
> @@ -258,6 +270,25 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID id,
>      return Success;
>  }
>  
> +int
> +DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID id)
> +{
> +    DRI2DrawablePtr pPriv = DRI2GetDrawable(pDraw);
> +    DRI2DrawableRefPtr ref;
> +
> +    if (!pPriv)
> +	return BadDrawable;
> +
> +    ref = DRI2FindClientDrawableRef(client, pPriv);
> +
> +    if (!ref)
> +	return BadDrawable;
> +
> +    FreeResourceByType(ref->dri2_id, dri2DrawableRes, FALSE);
> +
> +    return Success;
> +}
> +
>  static int DRI2DrawableGone(pointer p, XID id)
>  {
>      DRI2DrawablePtr pPriv = p;
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index fe0bf6c..dd63c30 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -214,7 +214,9 @@ extern _X_EXPORT int DRI2CreateDrawable(ClientPtr client,
>  					DRI2InvalidateProcPtr invalidate,
>  					void *priv);
>  
> -extern _X_EXPORT void DRI2DestroyDrawable(DrawablePtr pDraw);
> +extern _X_EXPORT int DRI2DestroyDrawable(ClientPtr client,
> +                                         DrawablePtr pDraw,
> +                                         XID id);
>  
>  extern _X_EXPORT DRI2BufferPtr *DRI2GetBuffers(DrawablePtr pDraw,
>  			     int *width,
> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index 4e48e65..c7e6be1 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -200,7 +200,7 @@ ProcDRI2DestroyDrawable(ClientPtr client)
>  		       &pDrawable, &status))
>  	return status;
>  
> -    return Success;
> +    return DRI2DestroyDrawable(client, pDrawable, stuff->drawable);
>  }
>  
>  
> -- 
> 1.7.0.4