[PATCH] dri2: Reference count DRI2 buffers

[Oldřich Jedlička]

Hi Dave,

On Saturday 21 August 2010 10:50:21 Dave Airlie wrote:
> 2010/8/21 Oldřich Jedlička <oldium.pro at seznam.cz>:
> > 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.
> 
> It would probably be better to move this discussion to xorg-devel, or
> at least involve Kristian,.
> 
> I'll try and review this thread next week if I can, I suck at dealing
> with swapbuffers stuff

Alban Browaeys has implemented a patch that uses client's private area to keep 
the list of pending events and invalidating them when the client has gone. See 
bug 29065 (https://bugs.freedesktop.org/show_bug.cgi?id=29065) for the patch 
(for the xf86-video-ati package).

Oldřich.

> 
> Dave.