On Don, 2010-08-19 at 10:20 +1000, Christopher James Halse Rogers wrote: > On Wed, 2010-08-18 at 20:52 +0200, Oldřich Jedlička wrote: > > On Wednesday 18 August 2010 10:49:20 Michel Dänzer wrote: > > > On Mit, 2010-08-18 at 18:24 +1000, Christopher James Halse Rogers > > > > > > wrote: > > > > On Tue, 2010-08-17 at 11:41 +0100, Michel Dänzer wrote: > > > > > On Die, 2010-08-17 at 12:19 +1000, 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. > > > > > > > > > > > > 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. > > > > > > > > > > Basically sounds good, but: If the client went away, what does > > > > > event->client point to in radeon_dri2_frame_event_handler()? > > > > > > > > Well, it points somewhere in the clients array, which will hopefully > > > > have clientGone set. At worst, it looks like it'll write a > > > > DRI2_BufferSwapComplete event to the unlucky client which claimed the > > > > empty spot. > > > > > > Maybe that's acceptable, but has it been investigated if the client > > > destruction could be deferred until the event arrival instead / as well? > > > > Isn't the ClientPtr allocated in dix/dispatch.c in method > > NextAvailableClient? > > It uses dixAllocateObjectWithPrivates that calls malloc, so it could happen > > that your ClientPtr might be freed (method CloseDownClient) when the > > callback > > arrives. > > Urgh, yes. I think that we can get notified of clients that go away, so > we could probably hack up a list of clients to not send to. That's > really ugly though. I'll see if I can find a cleaner solution.
Alban Browaeys' option 2 patch from the bug report doesn't seem too bad. I think we still need the reference counting though, otherwise e.g. I don't think there's anything to prevent the client from explicitly destroying the DRI2 buffers while there's a pending event referencing them. > > > > > Also, pixmaps already have a reference count, please just use that > > > > > instead of adding another layer. > > > > > > > > Can we use that? I originally thought of using that layer, but won't > > > > that interfere with the pixmap reference counting? It would work for > > > > attachment != DRI2BufferFrontLeft since the pixmap is entirely private > > > > in that case, but with attachment == DRI2BufferFrontLeft the buffer > > > > pixmap is visible to the outside, and so might have a lifespan different > > > > to the DRI2 buffer. > > > > > > That's what the reference count is for. :) As long as > > > increasing/decreasing the reference count is balanced in every layer > > > using the pixmap it should Just Work™. > > It looks to me like we sometimes need to destroy DRI2 buffers which are > associated with a still-valid pixmap, so we can't use the pixmap's > reference count. Yeah sorry, I had overlooked the fact radeon_dri2_frame_event_handler() needs the DRI2Buffer(2)Ptr, not only the pixmaps themselves. -- Earthling Michel Dänzer | http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer _______________________________________________ xorg-driver-ati mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-driver-ati
