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

[Michel Dänzer]

On Fre, 2011-03-25 at 17:27 +0200, Ville Syrjälä wrote: 
> 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 can certainly do that, though neither alone will fix the problem.


> 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.

I thought about that as well, but wanted to keep it simple for now. It
can always be optimized if this shows up on profiles.


> > > 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.

Gotcha.


> > > 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.

Hmm, that could work.


> > > 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.

Yeah, that approach seems to be working well in Gallium.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer