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 > <[email protected]> --- > 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; > } _______________________________________________ xorg-driver-ati mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-driver-ati
