[PATCH] dri2: Reference-count DRI2 buffers.

[Michel Dänzer]

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