2010/8/21 Oldřich Jedlička <[email protected]>: > 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 Dave. _______________________________________________ xorg-driver-ati mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-driver-ati
