[PATCH 4/9] dri2: Add reference counting to DRI2

[Christopher James Halse Rogers]

On Thu, 2011-02-03 at 19:48 +0200, Pauli wrote:
> From: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
> 
> Asynchronous request like SwapBuffers can still reference Drawable after
> the Drawable has been freed. DRI2Drawable cleanup should be delayed
> until all asynchronous operations have completed.
> 
> Reference counted DRI2Drawable helps to keep DRI2Drawable around until
> all request on it has completed.
> 
> Signed-off-by: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
> ---
>  hw/xfree86/dri2/dri2.c |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index f564822..21ee982 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -83,6 +83,7 @@ typedef struct _DRI2Drawable {
>      CARD64		 last_swap_ust; /* ust at completion of most recent swap */
>      int			 swap_limit; /* for N-buffering */
>      unsigned long	 serialNumber;
> +    unsigned		 refcnt;
>  } DRI2DrawableRec;
>  
>  typedef struct _DRI2Screen {
> @@ -190,12 +191,14 @@ DRI2AllocateDrawable(DrawablePtr pDraw)
>      pPriv->last_swap_ust = 0;
>      list_init(&pPriv->reference_list);
>      pPriv->serialNumber = DRI2DrawableSerial(pDraw);
> +    pPriv->refcnt = 0;
>  
>      if (pDraw->type == DRAWABLE_WINDOW) {
>  	pWin = (WindowPtr) pDraw;
>  	dixSetPrivate(&pWin->devPrivates, dri2WindowPrivateKey, pPriv);
>      } else {
>  	pPixmap = (PixmapPtr) pDraw;
> +	pPixmap->refcnt++;
>  	dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, pPriv);
>      }
>  
> @@ -251,6 +254,8 @@ DRI2AddDrawableRef(DRI2DrawablePtr pPriv, XID id, XID dri2_id,
>  	if (!AddResource(id, dri2DrawableRes, pPriv))
>  	    return BadAlloc;
>  
> +    pPriv->refcnt++;
> +
>      ref->id = id;
>      ref->dri2_id = dri2_id; 
>      ref->invalidate = invalidate;
> @@ -309,6 +314,7 @@ static int DRI2DrawableGone(pointer p, XID id)
>  
>      list_for_each_entry_safe(ref, next, &pPriv->reference_list, link) {
>  	if (ref->dri2_id == id) {
> +	    pPriv->refcnt--;
>  	    list_del(&ref->link);
>  	    /* If this was the last ref under this X drawable XID,
>  	     * unregister the X drawable resource. */
> @@ -319,13 +325,15 @@ static int DRI2DrawableGone(pointer p, XID id)
>  	}
>  
>  	if (ref->id == id) {
> +	    pPriv->refcnt--;
>  	    list_del(&ref->link);
>  	    FreeResourceByType(ref->dri2_id, dri2DrawableRes, TRUE);
>  	    free(ref);
>  	}
>      }
>  
> -    if (!list_is_empty(&pPriv->reference_list))
> +
> +    if (pPriv->refcnt > 0)
>  	return Success;
>  
>      pDraw = pPriv->drawable;
> @@ -343,6 +351,7 @@ static int DRI2DrawableGone(pointer p, XID id)
>      } else {
>  	pPixmap = (PixmapPtr) pDraw;
>  	dixSetPrivate(&pPixmap->devPrivates, dri2PixmapPrivateKey, NULL);
> +	pDraw->pScreen->DestroyPixmap(pPixmap);
>      }
>  
>      free(pPriv);
> @@ -694,7 +703,12 @@ void
>  DRI2WaitMSCComplete(ClientPtr client, DRI2DrawablePtr pPriv, int frame,
>  		    unsigned int tv_sec, unsigned int tv_usec)
>  {
> +    pPriv->refcnt--;
>  
> +    if (pPriv->refcnt == 0) {

In DRI2DrawableGone you compare to > 0.  Although I'm convinced that
WaitMSCComplete won't be called with pPriv->refcnt <= 0, it wouldn't
hurt to be similarly paranoid here, and compare <= 0 rather than == 0.

> +	DRI2DrawableGone(pPriv, 0);
> +	return;
> +    }
>  
>      ProcDRI2WaitMSCReply(client, ((CARD64)tv_sec * 1000000) + tv_usec,
>  			 frame, pPriv->swap_count);
> @@ -752,7 +766,12 @@ DRI2SwapComplete(ClientPtr client, DRI2DrawablePtr pPriv, int frame,
>  
>      pPriv->swapsPending--;
>      pPriv->swap_count++;
> +    pPriv->refcnt--;
>  
> +    if (pPriv->refcnt == 0) {
> +	DRI2DrawableGone(pPriv, 0);
> +	return;
> +    }
>  
>      if (pDraw) {
>  	BoxRec          box;
> @@ -836,6 +855,7 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
>  	RegionInit(&region, &box, 0);
>  
>  	pPriv->swapsPending++;
> +	pPriv->refcnt++;
>  
>  	(*ds->CopyRegion)(pDraw, &region, pDestBuffer, pSrcBuffer);
>  	DRI2SwapComplete(client, pPriv, target_msc, 0, 0, DRI2_BLIT_COMPLETE,
> @@ -877,10 +897,12 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
>      }
>  
>      pPriv->swapsPending++;
> +    pPriv->refcnt++;
>      ret = (*ds->ScheduleSwap)(client, pDraw, pDestBuffer, pSrcBuffer,
>  			      swap_target, divisor, remainder, func, data);
>      if (!ret) {
>  	pPriv->swapsPending--; /* didn't schedule */
> +	pPriv->refcnt--;
>          xf86DrvMsg(pScreen->myNum, X_ERROR,
>  		   "[DRI2] %s: driver failed to schedule swap\n", __func__);
>  	return BadDrawable;
> @@ -951,6 +973,8 @@ DRI2WaitMSC(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
>      if (pDraw == NULL)
>  	return BadDrawable;
>  
> +    pPriv->refcnt++;
> +
>      /* Old DDX just completes immediately */
>      if (!ds->ScheduleWaitMSC) {
>  	DRI2WaitMSCComplete(client, pPriv, target_msc, 0, 0);
> @@ -959,8 +983,10 @@ DRI2WaitMSC(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
>      }
>  
>      ret = (*ds->ScheduleWaitMSC)(client, pDraw, target_msc, divisor, remainder);
> -    if (!ret)
> +    if (!ret) {
> +	pPriv->refcnt--;
>  	return BadDrawable;
> +    }
>  
>      return Success;
>  }

Reviewed-By: Christopher James Halse Rogers
<christopher.halse.rogers at canonical.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110204/9a641594/attachment.pgp>