[PATCH] dri2: Reference count DRI2 buffers

[Oldřich Jedlička]

On Thursday 19 August 2010 10:57:21 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.

I was also thinking about the solution and did some xorg-server investigation. 
Personally I don't like comparing pointer values (ClientPtr), because it could 
be the same - sequence malloc/free/malloc could return the same pointer value.

Here are two other possible solutions:

1. Add a "uniqueId" (increasing number with each client) to ClientRec. Then 
you can compare something really unique. On the other hand this needs change 
in xorg-server.

2. Use AddCallback(&ClientStateCallback, our_client_state_changed_method, 0) 
during driver initialization to detect that any client went away and 
invalidate its events (add field "valid" in event). That would be even better 
than solution 1 - no change of xorg-server is needed. Each client could have 
private data too - double-linked list of pending events - registered with 
dixRegisterPrivateKey(). The event would be removed from the list only when it 
is valid (otherwise the prev/next list pointers would be invalid too). Invalid 
events would be ignored in the handler, they would be only freed.

Personally I like solution 2, because it fully uses xorg-server facilities. 
But I don't know if this isn't too much or if there exists a simpler solution.

Oldřich.

> 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.
> 
> Fixes: http://bugs.freedesktop.org/show_bug.cgi?id=29065
> 
> v2: Don't write completion events to the client if it has quit.
> Signed-off-by: Christopher James Halse Rogers
> <christopher.halse.rogers at canonical.com> ---
>  src/radeon_dri2.c |   73
> +++++++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 66
> insertions(+), 7 deletions(-)
> 
> diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> index 4cafbc6..c6636c6 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;
>  };
> 
> 
> @@ -244,6 +245,7 @@ radeon_dri2_create_buffer(DrawablePtr drawable,
>      buffers->flags = 0; /* not tiled */
>      privates->pixmap = pixmap;
>      privates->attachment = attachment;
> +    privates->refcnt = 1;
> 
>      return buffers;
>  }
> @@ -275,13 +277,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);
> +        }
>      }
>  }
>  #endif
> @@ -362,6 +377,7 @@ enum DRI2FrameEventType {
>  typedef struct _DRI2FrameEvent {
>      XID drawable_id;
>      ClientPtr client;
> +    int client_index;
>      enum DRI2FrameEventType type;
>      int frame;
> 
> @@ -372,11 +388,26 @@ typedef struct _DRI2FrameEvent {
>      DRI2BufferPtr back;
>  } DRI2FrameEventRec, *DRI2FrameEventPtr;
> 
> +static void
> +radeon_dri2_ref_buffer(BufferPtr buffer)
> +{
> +    struct dri2_buffer_priv *private = buffer->driverPrivate;
> +    private->refcnt++;
> +}
> +
> +static void
> +radeon_dri2_unref_buffer(BufferPtr buffer)
> +{
> +    struct dri2_buffer_priv *private = buffer->driverPrivate;
> +    radeon_dri2_destroy_buffer(&(private->pixmap->drawable), buffer);
> +}
> +
>  void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int
> tv_sec, unsigned int tv_usec, void *event_data) {
>      DRI2FrameEventPtr event = event_data;
>      DrawablePtr drawable;
> +    ClientPtr client;
>      ScreenPtr screen;
>      ScrnInfoPtr scrn;
>      int status;
> @@ -387,6 +418,8 @@ void radeon_dri2_frame_event_handler(unsigned int
> frame, unsigned int tv_sec, status = dixLookupDrawable(&drawable,
> event->drawable_id, serverClient, M_ANY, DixWriteAccess);
>      if (status != Success) {
> +        radeon_dri2_unref_buffer(event->front);
> +        radeon_dri2_unref_buffer(event->back);
>          free(event);
>          return;
>      }
> @@ -394,6 +427,17 @@ void radeon_dri2_frame_event_handler(unsigned int
> frame, unsigned int tv_sec, screen = drawable->pScreen;
>      scrn = xf86Screens[screen->myNum];
> 
> +    /* event->client may have quit between submitting a SwapBuffers
> request +     * and this callback being triggered.
> +     *
> +     * Check our saved client pointer against the client in the saved
> client +     * slot.  This will catch almost all cases where the client
> that requested +     * SwapBuffers has gone away, and will guarantee that
> there is at least a +     * valid client to write the BufferSwapComplete
> event to.
> +     */
> +    client = event->client == clients[event->client_index] ?
> +            event->client : NULL;
> +
>      switch (event->type) {
>      case DRI2_FLIP:
>      case DRI2_SWAP:
> @@ -405,11 +449,11 @@ void radeon_dri2_frame_event_handler(unsigned int
> frame, unsigned int tv_sec, radeon_dri2_copy_region(drawable, &region,
> event->front, event->back); swap_type = DRI2_BLIT_COMPLETE;
> 
> -        DRI2SwapComplete(event->client, drawable, frame, tv_sec, tv_usec,
> +        DRI2SwapComplete(client, drawable, frame, tv_sec, tv_usec,
>                  swap_type, event->event_complete, event->event_data);
>          break;
>      case DRI2_WAITMSC:
> -        DRI2WaitMSCComplete(event->client, drawable, frame, tv_sec,
> tv_usec); +        DRI2WaitMSCComplete(client, drawable, frame, tv_sec,
> tv_usec); break;
>      default:
>          /* Unknown type */
> @@ -418,6 +462,8 @@ void radeon_dri2_frame_event_handler(unsigned int
> frame, unsigned int tv_sec, break;
>      }
> 
> +    radeon_dri2_unref_buffer(event->front);
> +    radeon_dri2_unref_buffer(event->back);
>      free(event);
>  }
> 
> @@ -512,6 +558,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr
> client, DrawablePtr draw,
> 
>      wait_info->drawable_id = draw->id;
>      wait_info->client = client;
> +    wait_info->client_index = client->index;
>      wait_info->type = DRI2_WAITMSC;
> 
>      /* Get current count */
> @@ -648,11 +695,19 @@ static int radeon_dri2_schedule_swap(ClientPtr
> client, DrawablePtr draw,
> 
>      swap_info->drawable_id = draw->id;
>      swap_info->client = client;
> +    swap_info->client_index = client->index;
>      swap_info->event_complete = func;
>      swap_info->event_data = data;
>      swap_info->front = front;
>      swap_info->back = back;
> 
> +    /* radeon_dri2_frame_event_handler will get called some unknown time
> in the +     * future with these buffers.  Take a reference to ensure that
> they won't +     * get destroyed before then.
> +     */
> +    radeon_dri2_ref_buffer(front);
> +    radeon_dri2_ref_buffer(back);
> +
>      /* Get current count */
>      vbl.request.type = DRM_VBLANK_RELATIVE;
>      if (crtc > 0)
> @@ -776,6 +831,10 @@ blit_fallback:
>      DRI2SwapComplete(client, draw, 0, 0, 0, DRI2_BLIT_COMPLETE, func,
> data); if (swap_info)
>          free(swap_info);
> +
> +    radeon_dri2_unref_buffer(front);
> +    radeon_dri2_unref_buffer(back);
> +
>      *target_msc = 0; /* offscreen, so zero out target vblank count */
>      return TRUE;
>  }