On Friday 20 August 2010 11:05:40 Christopher James Halse Rogers wrote: > 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.
Yeah, looks so. I think it is now the ATI driver developpers turn to say what they want - if your patch is good or needs enhancing. > > > 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. That's right. Maybe somebody from xorg-devel list would be interrested. > > > > 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 ;). This is clearly a collective DRI2 interface problem, so no surprise here :-) Oldřich. > > 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 > > > > > <[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
