[PATCH xserver] dri2: Invalidate DRI2 buffers for all windows with the same pixmap on swap.

[Ville Syrjälä]

On Fri, Mar 25, 2011 at 03:32:06PM +0100, ext Michel Dänzer wrote:
> On Fre, 2011-03-25 at 14:47 +0200, Ville Syrjälä wrote: 
> > On Fri, Mar 25, 2011 at 12:35:37PM +0100, ext Michel Dänzer wrote:
> > > From: Michel Dänzer <daenzer at vmware.com>
> > > 
> > > Without this, when a compositing manager unredirects a fullscreen window which
> > > uses DRI2 and page flipping, the DRI2 buffer information for the compositing
> > > manager's output window (typically the Composite Overlay Window or root window)
> > > may become stale, resulting in all kinds of hilarity.
> > 
> > Make sense to me.
> 
> Thanks, does that mean I have your Reviewed-by: ?

Reviewed-by: Ville Syrjälä <ville.syrjala at nokia.com>

I think you could split the two changes into separate patches though.

I was also thinking that there should be a way to avoid walking the
whole window tree in some cases. Eg. you would first walk towards the
root until you encounter a different window pixmap, and then start
walking the tree from there, and stop walking the children whenever
you encounter a different window pixmap.

> > BTW I'm toying around with offscreen flipping, and for that I'm using
> > the following patch to also invalidate the window's pixmap [...]
> 
> Makes sense to me as well.

Oh and there's another change that's necessary;
DRI2InvalidateBuffersEvent() must send the event with the client's
pixmap ID (ref->id) instead of pDraw->id. But that change is already
part of the ref count patch set.

> > Subject: [PATCH] dri2: Invalidate window pixmaps
> > 
> > While a redirected window is flipped, it's pixmap may still be used as
> > and EGL image and should also get invalidated. When sending invalidate
> > events for a window, also send the events for it's pixmap.
> 
> it's -> its
> 
> in both cases.

Right.

> > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> > index 03011ff..5faf042 100644
> > --- a/hw/xfree86/dri2/dri2.c
> > +++ b/hw/xfree86/dri2/dri2.c
> > @@ -1248,6 +1261,7 @@ DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w, int h, int bw,
> >  	return Success;
> >  
> >      DRI2InvalidateDrawable(DRI2GetDrawable(pDraw));
> > +    DRI2InvalidateWindowPixmap(pWin);
> 
> Could just do something like
> 
>     DRI2InvalidateDrawable(&pScreen->GetWindowPixmap(pWin)->drawable);
> 
> either in DRI2InvalidateWindowPixmap() or in its callers directly.

The ref count patch set changes DRI2InvalidateDrawable() to take a
DRI2DrawablePtr instead of DrawablePtr.

> > I do wonder if we could always attach the ref list to the window pixmap
> > instead of the window itself. That would avoid the need to walk the
> > whole window tree when a flip occurs. But I didn't look into this any
> > further, so I'm not sure how we would handle cases where the window
> > pixmap changes.
> 
> I think the problems start earlier than that. E.g. unredirected windows
> all share the screen pixmap, but the DRI2 buffers can't be shared
> between windows in general.

I was thinking that the buffers would still belong to the DRI2 drawable,
but the ref list would be attached to the pixmap. DRI2InvalidateDrawable
would then just get the ref list via the window pixmap.

> > > Also re-generate the real front buffer information every time the client asks
> > > for it, or we might keep around stale cached information.
> > 
> > For offscreen flipping I solved this by always updating the buffer
> > information in the ReuseBufferNotify() hook.
> 
> That hasn't landed in master yet, has it?

Hmm. I thought it had, but apparently not.

> > BTW I don't really like the current dri2 design. The old interface with
> > one CreateBuffers call instead of multiple CreateBuffer calls seemed
> > better. It made it easier to do some things in the driver code. For
> > example the depth_pixmap handling in the ati driver looks broken with
> > the new dri2 interface.
> 
> Quite possibly. I think the DRI2 driver interface generally puts too
> much logic in the drivers. So e.g. the drivers do flipping in a hackish
> way internally, which is why this patch needs to always re-generate the
> front buffer information.

Yeah. Most things should be in the shared code, however my feeling is
that a structure where the shared code would be more of a utility
library would allow more flexibility in the driver. Oh well, no time to
really think about such design issue at the moment.

-- 
Ville Syrjälä