[PATCH] dri2: Reference count DRI2 buffers

Christopher James Halse Rogers christopher.halse.rogers at canonical.com
Fri Aug 20 02:05:40 PDT 2010


On Fri, 2010-08-20 at 07:55 +0200, Oldřich Jedlička wrote:
> On Friday 20 August 2010 02:04:35 Christopher James Halse Rogers wrote:
> > On Thu, 2010-08-19 at 21:23 +0200, Oldřich Jedlička wrote:
> > > 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.
> > 
> > Yeah, there is a chance that a stray DRI2_SwapBuffersComplete event
> > could be written to an uninterested client.  It certainly won't try to
> > write to an invalid client, though, so it shouldn't crash X.  And it is
> > reasonably unlikely that a client will go away and a new client will
> > both fill the same slot *and* get the same memory address - there's a
> > lot of memory allocation going on in the surrounding code.
> 
> Question is if the client can handle this :-)
> 
> > > 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.
> > 
> > I thought about that, too.  It seemed a bit excessive for the driver to
> > maintain a list of clients as they come and go just for the purpose of
> > not sending an event when a client quits.
> 
> There is no need to have a list of clients. Each client would have the list of 
> events kept in the client's devPrivate area (registered with 
> dixRegisterPrivateKey, found by dixLookupPrivate) - ony the event list head is 
> necessary:
> 
> 1. The event would have "prev" and "next" pointers for the purposes of the 
> list and a new "valid" boolean field.
> 
> 2. Register Client State callback by the call to AddCallback and call 
> dixRegisterPrivateKey in the driver initialization routine. Add RemoveCallback 
> in the driver shut-down routine.
> 
> 3. Whenever new client connects, the list head would be set to 0 (done in the 
> Client State callback). This step is probably unnecessary if the area is set 
> to 0 via calloc (I'm just not sure).
> 
> 4. Whenever the new event is created, dixLookupPrivate would get the client's 
> list head and the new event would be added to the list head.
> 
> 5. Whenever the client dies (recognized in the Client State callback), the 
> list would be walked-through and events invalidated (valid=false).
> 
> 6. For valid events (valid==true) on event callback the event would be removed 
> from the list (just modify the event's prev's "next" and event's next's "prev" 
> pointers, eventually modifying the client's list head).
> 
> 7. For invalid events (valid==false) the list would stay unmodified (because of 
> the list head modification on freed client's memory), only the event would be 
> freed.
> 
> This looks to me like a few lines of code for each point, nothing big.

Ah, right.  That's the reverse of what I was thinking; it's more
reasonable.

It still seems a bit heavyweight to me for this corner case.

> 
> > The pointer comparison is quick, cheap, and ensures we won't crash X.
> 
> Yes, that should work most of the time :-) But this might add some hard-to-
> reproduce problem with client getting unwanted message.
> 
> > > 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.
> > 
> > I think that there should actually be solution 3: the DRI2 extension
> > handles this for drivers as a part of the swapbuffers/waitmsc common
> > code.
> 
> Yes, definitely.
> 
> But it looks like the driver is currently scheduling the event (and holding 
> wrong data - ClientPtr) and handling it, only notifying DRI2 on xserver about 
> the result. So that would mean creating some common part that handles event 
> creation/handling/invalidation. The driver would call xserver when the event 
> arrives and xserver would call driver back if it still applies. Or something 
> simillar...

Well, the need to reference count buffers could easily be be removed by
having DRI2ScheduleSwap take a reference to the buffers and
DRI2SwapComplete decrement that, in the same way that it handles
pending_swaps and such already.

Similarly, if we need to handle the Client gone fun it could be handled
there.

> 
> > Intel has reference counted buffers, this patch adds
> > reference-counted buffers to radeon, and when Nouveau gains SwapBuffers
> > support *they* will need to reference-count their buffers.
> 
> Did you find if Intel driver handles disconnected clients somehow, or they just 
> don't care?

The Intel driver doesn't care - it merrily writes away.  And generates
“invalid drawable” warnings from DRI2 (as does this patch), but that's
somewhat beside the point ;).

> 
> Oldřich.
>  
> > Given the nature of SwapBuffers / WaitMSC (set a trigger, do some X
> > work, run a callback later) it seems that all driver implementations
> > will have to solve this problem, so it makes sense to do it in the DRI2
> > extension itself.
> > 
> > > 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;
> > > >  
> > > >  }


-------------- 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-driver-ati/attachments/20100820/ef6392e1/attachment.pgp>


More information about the xorg-driver-ati mailing list