<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"> <html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#ffffff"> From resource management point of view this patch seems correct to me. Reviewed-by: Rami Ylimäki <a class="moz-txt-link-rfc2396E" href="mailto:rami.ylimaki@vincit.fi"><rami.ylimaki@vincit.fi></a> On 10/25/2010 04:55 PM, Pauli Nieminen wrote: <blockquote cite="mid:1288014935-31607-1-git-send-email-ext-pauli.nieminen@nokia.com" type="cite"> <pre wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:ext-pauli.nieminen@nokia.com"><ext-pauli.nieminen@nokia.com></a> CC: Kristian HÃ¸gsberg <a class="moz-txt-link-rfc2396E" href="mailto:krh@bitplanet.net"><krh@bitplanet.net></a> --- 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); } </pre> <pre wrap=""> <fieldset class="mimeAttachmentHeader"></fieldset> _______________________________________________ <a class="moz-txt-link-abbreviated" href="mailto:xorg-devel@lists.x.org:">xorg-devel@lists.x.org:</a> X.Org development Archives: <a class="moz-txt-link-freetext" href="http://lists.x.org/archives/xorg-devel">http://lists.x.org/archives/xorg-devel</a> Info: <a class="moz-txt-link-freetext" href="http://lists.x.org/mailman/listinfo/xorg-devel">http://lists.x.org/mailman/listinfo/xorg-devel</a></pre> </blockquote> </body> </html>