[PATCH] dri2: Reference count DRI2 buffers
Christopher James Halse Rogers
christopher.halse.rogers at canonical.com
Thu Aug 19 17:04:35 PDT 2010
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.
>
> 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.
The pointer comparison is quick, cheap, and ensures we won't crash X.
>
> 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. 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.
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, ®ion,
> > 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/212b8beb/attachment.pgp>
More information about the xorg-driver-ati
mailing list