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 <[email protected]> > > > > > > > > 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ä <[email protected]> > > 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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
