[PATCH] dri2: Reference-count DRI2 buffers.

[Oldřich Jedlička]

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.

The ClientRec doesn't have any reference counting. My opinion is that holding 
the ClientPtr isn't good (but I'm not xorg-server developper, so I might miss 
something).

Oldřich.

> > > > @@ -267,13 +269,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);
> > > > +        }
> > > 
> > > 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™.