[PATCH 1/2] DRI2: Reference count buffers across SwapBuffers

Pauli Nieminen ext-pauli.nieminen at nokia.com
Wed Dec 8 07:47:19 PST 2010


On 08/12/10 07:56 +0100, ext Christopher James Halse Rogers wrote:
> The SwapBuffers request requires that we trigger the swap at some point
> in the future.  Sane drivers implement this by passing this request to
> something that will trigger a callback with the buffer pointers
> at the appropriate time.
> 
> The client can cause those buffers to be freed in X before the trigger
> occurs, most easily by quitting.  This leads to a server crash in
> the driver when trying to do the swap.
> 
> See http://bugs.freedesktop.org/show_bug.cgi?id=29065 for Radeon and
> https://bugs.freedesktop.org/show_bug.cgi?id=28080 for Intel.
> 

Could this reference counting be applied to DRI2Drawable instead of per
buffer?

But in any case this looks like good idea. I didn't yet have to have detailed
look what is inside the patch.

> Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> ---
>  hw/xfree86/dri2/dri2.c |   98 ++++++++++++++++++++++++++++++++++++++---------
>  hw/xfree86/dri2/dri2.h |    1 +
>  2 files changed, 80 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index e4693d9..6da2e17 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -107,12 +107,41 @@ typedef struct _DRI2Screen {
>      ConfigNotifyProcPtr		 ConfigNotify;
>  } DRI2ScreenRec;
>  
> +typedef struct _DRI2SwapCompleteDataRec {
> +    DRI2BufferPtr	src;
> +    DRI2BufferPtr	dest;
> +    void *		data;
> +} DRI2SwapCompleteDataRec, *DRI2SwapCompleteDataPtr;
> +
>  static DRI2ScreenPtr
>  DRI2GetScreen(ScreenPtr pScreen)
>  {
>      return dixLookupPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey);
>  }
>  
> +static void
> +buffer_ref(DRI2BufferPtr buffer)
> +{
> +    buffer->refcnt++;
> +}
> +
> +static void
> +buffer_unref(DrawablePtr pDraw, DRI2BufferPtr buffer)
> +{
> +    DRI2ScreenPtr ds;
> +    if (buffer->refcnt == 0) {
> +	xf86DrvMsg(pDraw->pScreen->myNum, X_ERROR,
> +	    "[DRI2] Attempt to unreference already freed buffer ignored\n");
> +	return;
> +    }
> +
> +    buffer->refcnt--;
> +    if (buffer->refcnt == 0) {
> +	ds = DRI2GetScreen(pDraw->pScreen);
> +	(*ds->DestroyBuffer)(pDraw, buffer);
> +    }
> +}
> +
>  static DRI2DrawablePtr
>  DRI2GetDrawable(DrawablePtr pDraw)
>  {
> @@ -261,7 +290,6 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, XID id,
>  static int DRI2DrawableGone(pointer p, XID id)
>  {
>      DRI2DrawablePtr pPriv = p;
> -    DRI2ScreenPtr   ds = pPriv->dri2_screen;
>      DRI2DrawableRefPtr ref, next;
>      WindowPtr pWin;
>      PixmapPtr pPixmap;
> @@ -300,7 +328,7 @@ static int DRI2DrawableGone(pointer p, XID id)
>  
>      if (pPriv->buffers != NULL) {
>  	for (i = 0; i < pPriv->bufferCount; i++)
> -	    (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]);
> +	    buffer_unref(pDraw, pPriv->buffers[i]);
>  
>  	free(pPriv->buffers);
>      }
> @@ -341,6 +369,7 @@ allocate_or_reuse_buffer(DrawablePtr pDraw, DRI2ScreenPtr ds,
>  	|| !dimensions_match
>  	|| (pPriv->buffers[old_buf]->format != format)) {
>  	*buffer = (*ds->CreateBuffer)(pDraw, attachment, format);
> +	buffer_ref(*buffer);
>  	pPriv->serialNumber = DRI2DrawableSerial(pDraw);
>  	return TRUE;
>  
> @@ -355,13 +384,12 @@ static void
>  update_dri2_drawable_buffers(DRI2DrawablePtr pPriv, DrawablePtr pDraw,
>  			     DRI2BufferPtr *buffers, int *out_count, int *width, int *height)
>  {
> -    DRI2ScreenPtr   ds = DRI2GetScreen(pDraw->pScreen);
>      int i;
>  
>      if (pPriv->buffers != NULL) {
>  	for (i = 0; i < pPriv->bufferCount; i++) {
>  	    if (pPriv->buffers[i] != NULL) {
> -		(*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]);
> +		buffer_unref(pDraw, pPriv->buffers[i]);
>  	    }
>  	}
>  
> @@ -498,7 +526,7 @@ err_out:
>  
>      for (i = 0; i < count; i++) {
>  	    if (buffers[i] != NULL)
> -		    (*ds->DestroyBuffer)(pDraw, buffers[i]);
> +		    buffer_unref(pDraw, buffers[i]);
>      }
>  
>      free(buffers);
> @@ -708,21 +736,31 @@ DRI2WakeClient(ClientPtr client, DrawablePtr pDraw, int frame,
>      }
>  }
>  
> +static void
> +free_swap_complete_data (DrawablePtr pDraw, DRI2SwapCompleteDataPtr pSwapData)
> +{
> +    buffer_unref(pDraw, pSwapData->src);
> +    buffer_unref(pDraw, pSwapData->dest);
> +    free(pSwapData);
> +}
> +
>  void
>  DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, int frame,
>  		   unsigned int tv_sec, unsigned int tv_usec, int type,
>  		   DRI2SwapEventPtr swap_complete, void *swap_data)
>  {
> -    ScreenPtr	    pScreen = pDraw->pScreen;
> -    DRI2DrawablePtr pPriv;
> -    CARD64          ust = 0;
> -    BoxRec          box;
> -    RegionRec       region;
> +    ScreenPtr	            pScreen = pDraw->pScreen;
> +    DRI2DrawablePtr         pPriv;
> +    CARD64                  ust = 0;
> +    BoxRec                  box;
> +    RegionRec               region;
> +    DRI2SwapCompleteDataPtr pSwapData = swap_data;
>  
>      pPriv = DRI2GetDrawable(pDraw);
>      if (pPriv == NULL) {
>          xf86DrvMsg(pScreen->myNum, X_ERROR,
>  		   "[DRI2] %s: bad drawable\n", __func__);
> +	free_swap_complete_data(pDraw, pSwapData);
>  	return;
>      }
>  
> @@ -739,12 +777,15 @@ DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, int frame,
>  
>      ust = ((CARD64)tv_sec * 1000000) + tv_usec;
>      if (swap_complete)
> -	swap_complete(client, swap_data, type, ust, frame, pPriv->swap_count);
> +	swap_complete(client, pSwapData->data, type, ust, frame,
> +		      pPriv->swap_count);
>  
>      pPriv->last_swap_msc = frame;
>      pPriv->last_swap_ust = ust;
>  
>      DRI2WakeClient(client, pDraw, frame, tv_sec, tv_usec);
> +    
> +    free_swap_complete_data(pDraw, pSwapData);
>  }
>  
>  Bool
> @@ -771,12 +812,13 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc,
>  		CARD64 divisor, CARD64 remainder, CARD64 *swap_target,
>  		DRI2SwapEventPtr func, void *data)
>  {
> -    ScreenPtr       pScreen = pDraw->pScreen;
> -    DRI2ScreenPtr   ds = DRI2GetScreen(pDraw->pScreen);
> -    DRI2DrawablePtr pPriv;
> -    DRI2BufferPtr   pDestBuffer = NULL, pSrcBuffer = NULL;
> -    int             ret, i;
> -    CARD64          ust, current_msc;
> +    ScreenPtr               pScreen = pDraw->pScreen;
> +    DRI2ScreenPtr           ds = DRI2GetScreen(pDraw->pScreen);
> +    DRI2DrawablePtr         pPriv;
> +    DRI2BufferPtr           pDestBuffer = NULL, pSrcBuffer = NULL;
> +    DRI2SwapCompleteDataPtr pSwapData;
> +    int                     ret, i;
> +    CARD64                  ust, current_msc;
>  
>      pPriv = DRI2GetDrawable(pDraw);
>      if (pPriv == NULL) {
> @@ -796,6 +838,23 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc,
>  		   "[DRI2] %s: drawable has no back or front?\n", __func__);
>  	return BadDrawable;
>      }
> +    
> +    pSwapData = malloc(sizeof *pSwapData);
> +    if (pSwapData == NULL)
> +	return BadAlloc;
> +
> +    /* 
> +     * Take a reference to the buffers we're exchanging.
> +     * This ensures that these buffers will remain valid until the swap
> +     * is completed.
> +     *
> +     * DRI2SwapComplete is required to unref these buffers.
> +     */
> +    buffer_ref(pSrcBuffer);
> +    buffer_ref(pDestBuffer);
> +    pSwapData->src = pSrcBuffer;
> +    pSwapData->dest = pDestBuffer;
> +    pSwapData->data = data;
>  
>      /* Old DDX or no swap interval, just blit */
>      if (!ds->ScheduleSwap || !pPriv->swap_interval) {
> @@ -812,7 +871,7 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc,
>  
>  	(*ds->CopyRegion)(pDraw, &region, pDestBuffer, pSrcBuffer);
>  	DRI2SwapComplete(client, pDraw, target_msc, 0, 0, DRI2_BLIT_COMPLETE,
> -			 func, data);
> +			 func, pSwapData);
>  	return Success;
>      }
>  
> @@ -851,11 +910,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, CARD64 target_msc,
>  
>      pPriv->swapsPending++;
>      ret = (*ds->ScheduleSwap)(client, pDraw, pDestBuffer, pSrcBuffer,
> -			      swap_target, divisor, remainder, func, data);
> +			      swap_target, divisor, remainder, func, pSwapData);
>      if (!ret) {
>  	pPriv->swapsPending--; /* didn't schedule */
>          xf86DrvMsg(pScreen->myNum, X_ERROR,
>  		   "[DRI2] %s: driver failed to schedule swap\n", __func__);
> +	free_swap_complete_data(pDraw, pSwapData);
>  	return BadDrawable;
>      }
>  
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index fe0bf6c..12343fd 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -42,6 +42,7 @@ typedef struct {
>      unsigned int pitch;
>      unsigned int cpp;
>      unsigned int flags;
> +    unsigned int refcnt;
>      unsigned int format;
>      void *driverPrivate;
>  } DRI2BufferRec, *DRI2BufferPtr;
> -- 
> 1.7.2.3
> 
> _______________________________________________
> 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


More information about the xorg-devel mailing list