[PATCH] DRI2: Free DRI2 drawable references in DRI2DestroyDrawable
Rami Ylimäki
rami.ylimaki at vincit.fi
Fri Oct 29 07:50:11 PDT 2010
From resource management point of view this patch seems correct to me.
Reviewed-by: Rami Ylimäki <rami.ylimaki at vincit.fi>
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(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.
>
> If client calls DRI2CreateDrawable twice for same drawable same
> reference is reused and reference count is incremented.
>
> Signed-off-by: Pauli Nieminen<ext-pauli.nieminen at nokia.com>
> CC: Kristian Høgsberg<krh at bitplanet.net>
>
> ---
> 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, void *priv)
> {
> DRI2DrawablePtr pPriv;
> + DRI2DrawableRefPtr ref;
> XID dri2_id;
> int rc;
>
> @@ -249,6 +264,11 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID id,
> pPriv = DRI2AllocateDrawable(pDraw);
> if (pPriv == NULL)
> return BadAlloc;
> + ref = DRI2FindClientDrawableRef(client, pPriv);
> + if (ref) {
> + ref->refcnt++;
> + return Success;
> + }
>
> dri2_id = FakeClientID(client->index);
> rc = DRI2AddDrawableRef(pPriv, id, dri2_id, invalidate, priv);
> @@ -258,34 +278,15 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID id,
> return Success;
> }
>
> -static int DRI2DrawableGone(pointer p, XID id)
> +static int
> +DRI2CleanupDrawable(DRI2DrawablePtr pPriv)
> {
> - DRI2DrawablePtr pPriv = p;
> DRI2ScreenPtr ds = pPriv->dri2_screen;
> - DRI2DrawableRefPtr ref, next;
> WindowPtr pWin;
> PixmapPtr pPixmap;
> DrawablePtr pDraw;
> int i;
>
> - list_for_each_entry_safe(ref, next,&pPriv->reference_list, link) {
> - if (ref->dri2_id == id) {
> - list_del(&ref->link);
> - /* If this was the last ref under this X drawable XID,
> - * unregister the X drawable resource. */
> - if (!DRI2LookupDrawableRef(pPriv, ref->id))
> - FreeResourceByType(ref->id, dri2DrawableRes, TRUE);
> - free(ref);
> - break;
> - }
> -
> - if (ref->id == id) {
> - list_del(&ref->link);
> - FreeResourceByType(ref->dri2_id, dri2DrawableRes, TRUE);
> - free(ref);
> - }
> - }
> -
> if (!list_is_empty(&pPriv->reference_list))
> return Success;
>
> @@ -310,6 +311,63 @@ static int DRI2DrawableGone(pointer p, XID id)
> return Success;
> }
>
> +int
> +DRI2DestroyDrawable(ClientPtr client, DrawablePtr pDraw, XID id)
> +{
> + DRI2DrawablePtr pPriv = DRI2GetDrawable(pDraw);
> + DRI2DrawableRefPtr ref, next;
> + Bool found = FALSE;
> +
> + if (!pPriv)
> + return BadDrawable;
> +
> + list_for_each_entry_safe(ref, next,&pPriv->reference_list, link) {
> + if (CLIENT_ID(ref->dri2_id) == client->index) {
> + found = TRUE;
> + if (--ref->refcnt> 0)
> + break;
> + list_del(&ref->link);
> + FreeResourceByType(ref->dri2_id, dri2DrawableRes, TRUE);
> + /* If this was the last ref under this X drawable XID,
> + * unregister the X drawable resource. */
> + if (!DRI2LookupDrawableRef(pPriv, ref->id))
> + FreeResourceByType(ref->id, dri2DrawableRes, TRUE);
> + free(ref);
> + break;
> + }
> + }
> +
> + if (!found)
> + return BadDrawable;
> +
> + return DRI2CleanupDrawable(pPriv);
> +}
> +
> +static int DRI2DrawableGone(pointer p, XID id)
> +{
> + DRI2DrawablePtr pPriv = p;
> + DRI2DrawableRefPtr ref, next;
> +
> + list_for_each_entry_safe(ref, next,&pPriv->reference_list, link) {
> + if (ref->dri2_id == id) {
> + list_del(&ref->link);
> + /* If this was the last ref under this X drawable XID,
> + * unregister the X drawable resource. */
> + if (!DRI2LookupDrawableRef(pPriv, ref->id))
> + FreeResourceByType(ref->id, dri2DrawableRes, TRUE);
> + free(ref);
> + break;
> + }
> +
> + if (ref->id == id) {
> + list_del(&ref->link);
> + FreeResourceByType(ref->dri2_id, dri2DrawableRes, TRUE);
> + free(ref);
> + }
> + }
> + return DRI2CleanupDrawable(pPriv);
> +}
> +
> static int
> find_attachment(DRI2DrawablePtr pPriv, unsigned attachment)
> {
> 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);
> }
>
>
>
>
> _______________________________________________
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20101029/2fc7810b/attachment.htm>
More information about the xorg-devel
mailing list