[PATCH] dri2: Reference-count DRI2 buffers.

[Michel Dänzer]

On Die, 2010-08-17 at 12:19 +1000, Christopher James Halse Rogers
wrote: 
> When a client calls ScheduleSwap we set up a kernel callback when the
> relevent vblank event occurs.  However, it's possible for the client
> to go away between calling ScheduleSwap and the vblank event,
> resulting in the buffers being destroyed before they're passed to
> radeon_dri2_frame_event_handler.
> 
> Add reference-counting to the buffers and take a reference in
> radeon_dri2_schedule_swap to ensure the buffers won't be destroyed
> before the vblank event is dealt with.

Basically sounds good, but: If the client went away, what does
event->client point to in radeon_dri2_frame_event_handler()?


> Fixes: http://bugs.freedesktop.org/show_bug.cgi?id=29065
> Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> ---
>  src/radeon_dri2.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> index 6356711..00b5712 100644
> --- a/src/radeon_dri2.c
> +++ b/src/radeon_dri2.c
> @@ -55,6 +55,7 @@ typedef DRI2Buffer2Ptr BufferPtr;
>  struct dri2_buffer_priv {
>      PixmapPtr   pixmap;
>      unsigned int attachment;
> +    unsigned int refcnt;
>  };
>  
> 
> @@ -236,6 +237,7 @@ radeon_dri2_create_buffer(DrawablePtr drawable,
>      buffers->flags = 0; /* not tiled */
>      privates->pixmap = pixmap;
>      privates->attachment = attachment;
> +    privates->refcnt = 1;
>  
>      return buffers;
>  }
> @@ -267,13 +269,26 @@ radeon_dri2_destroy_buffer(DrawablePtr drawable, BufferPtr buffers)
>      if(buffers)
>      {
>          ScreenPtr pScreen = drawable->pScreen;
> -        struct dri2_buffer_priv *private;
> +        struct dri2_buffer_priv *private = buffers->driverPrivate;
>  
> -        private = buffers->driverPrivate;
> -        (*pScreen->DestroyPixmap)(private->pixmap);
> +        /* Trying to free an already freed buffer is unlikely to end well */
> +        if (private->refcnt == 0) {
> +            ScrnInfoPtr scrn = xf86Screens[pScreen->myNum];
>  
> -        free(buffers->driverPrivate);
> -        free(buffers);
> +            xf86DrvMsg(scrn->scrnIndex, X_WARNING, 
> +                       "Attempted to destroy previously destroyed buffer.\
> + This is a programming error\n");
> +            return;
> +	}
> +
> +        private->refcnt--;
> +        if (private->refcnt == 0)
> +        {
> +            (*pScreen->DestroyPixmap)(private->pixmap);
> +
> +            free(buffers->driverPrivate);
> +            free(buffers);
> +        }

Also, pixmaps already have a reference count, please just use that
instead of adding another layer.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer