[PATCH] dri2: Reference count DRI2 buffers
Oldřich Jedlička
oldium.pro at seznam.cz
Thu Aug 19 22:55:45 PDT 2010
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.
> 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...
> 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?
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, ®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;
> > >
> > > }
More information about the xorg-driver-ati
mailing list