Re: [PATCH 1/2] Replace usage of DamageRegionAppend with DamageDamageRegion to fix reportAfter.
On Sam, 2010-10-30 at 12:36 -0700, Keith Packard wrote: On Sat, 30 Oct 2010 18:07:19 +0200, Michel Dänzer mic...@daenzer.net wrote: Right, but as long as there's at least one rendering operation in between, at that point EXA will synchronize the pixmap copies according to the accumulated pending damage. That's the assumption broken by your change. Eric's patch just catches a few non-rendering paths where post-op damage would not generate Damage events, as DamageRegionProcessPending would never have been invoked. For all rendering paths, it changes nothing [...] I know, as I said, the problem is when there's *no* rendering operation between DamageRegionAppend and DamageRegionProcessPending. (IIRC the problem I mentioned with starting compiz from a naked xterm was due to ProcDamageCreate called by compiz) -- Earthling Michel Dänzer |http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] Replace usage of DamageRegionAppend with DamageDamageRegion to fix reportAfter.
On Fre, 2010-10-29 at 12:21 -0700, Eric Anholt wrote: On Fri, 29 Oct 2010 10:46:00 +0200, Michel Dänzer mic...@daenzer.net wrote: On Don, 2010-10-28 at 20:46 -0700, Eric Anholt wrote: In all these cases, any rendering implied by this damage has already occurred, and we want to get the damage out to the client. Some of the DamageRegionAppend calls were explicitly telling damage to flush the reportAfter damage out, but not all. I haven't been able to confirm it on some quick tests, but I suspect this will break EXA in some cases, as it assumes that between a DamageRegionAppend/DamageRegionProcessPending call pair the corresponding region will be modified by a rendering operation and will thus be valid in the current copy of the pixmap contents and invalid in the other copy. Maybe what's needed here is some other mechanism for specifying a region that is not related to actual rendering but only to be reported to clients via the DAMAGE extension for informational purposes. FWIW, the test that prompted me to split up damage processing into two steps was starting compiz in an xterm on an otherwise 'naked' X server. The xterm window borders (the parts between the decoration and the actual terminal contents) would previously be corrupted. I suspect this change will reintroduce that problem with the EXA 'classic' scheme at least. [...] Without this change, you'll still get the pending damage reported at some later date when other damage occurs to the pixmap that triggers a processing of pending damage, completely disconnected from when the rendering occurred, if any was involved. Right, but as long as there's at least one rendering operation in between, at that point EXA will synchronize the pixmap copies according to the accumulated pending damage. That's the assumption broken by your change. Though I agree some of the cases you're changing may not have been 100% correct before, and introducing another mechanism for handling damage not directly related to rendering should fix that (and as a bonus avoid superfluous synchronization between EXA's pixmap copies). So I still think that would be the proper solution. -- Earthling Michel Dänzer |http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] Replace usage of DamageRegionAppend with DamageDamageRegion to fix reportAfter.
On Sat, 30 Oct 2010 18:07:19 +0200, Michel Dänzer mic...@daenzer.net wrote: Right, but as long as there's at least one rendering operation in between, at that point EXA will synchronize the pixmap copies according to the accumulated pending damage. That's the assumption broken by your change. Eric's patch just catches a few non-rendering paths where post-op damage would not generate Damage events, as DamageRegionProcessPending would never have been invoked. For all rendering paths, it changes nothing (the change in EXA simply calls DamageDamageRegion instead of the equivalent in-line sequence of DamageRegionAppend followed by DamageRegionProcessPending). -- keith.pack...@intel.com pgprXdQCuUzc3.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] Replace usage of DamageRegionAppend with DamageDamageRegion to fix reportAfter.
On Don, 2010-10-28 at 20:46 -0700, Eric Anholt wrote: In all these cases, any rendering implied by this damage has already occurred, and we want to get the damage out to the client. Some of the DamageRegionAppend calls were explicitly telling damage to flush the reportAfter damage out, but not all. I haven't been able to confirm it on some quick tests, but I suspect this will break EXA in some cases, as it assumes that between a DamageRegionAppend/DamageRegionProcessPending call pair the corresponding region will be modified by a rendering operation and will thus be valid in the current copy of the pixmap contents and invalid in the other copy. Maybe what's needed here is some other mechanism for specifying a region that is not related to actual rendering but only to be reported to clients via the DAMAGE extension for informational purposes. FWIW, the test that prompted me to split up damage processing into two steps was starting compiz in an xterm on an otherwise 'naked' X server. The xterm window borders (the parts between the decoration and the actual terminal contents) would previously be corrupted. I suspect this change will reintroduce that problem with the EXA 'classic' scheme at least. -- Earthling Michel Dänzer |http://www.vmware.com Libre software enthusiast | Debian, X and DRI developer ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] Replace usage of DamageRegionAppend with DamageDamageRegion to fix reportAfter.
On Fri, 29 Oct 2010 10:46:00 +0200, Michel Dänzer mic...@daenzer.net wrote: On Don, 2010-10-28 at 20:46 -0700, Eric Anholt wrote: In all these cases, any rendering implied by this damage has already occurred, and we want to get the damage out to the client. Some of the DamageRegionAppend calls were explicitly telling damage to flush the reportAfter damage out, but not all. I haven't been able to confirm it on some quick tests, but I suspect this will break EXA in some cases, as it assumes that between a DamageRegionAppend/DamageRegionProcessPending call pair the corresponding region will be modified by a rendering operation and will thus be valid in the current copy of the pixmap contents and invalid in the other copy. Maybe what's needed here is some other mechanism for specifying a region that is not related to actual rendering but only to be reported to clients via the DAMAGE extension for informational purposes. FWIW, the test that prompted me to split up damage processing into two steps was starting compiz in an xterm on an otherwise 'naked' X server. The xterm window borders (the parts between the decoration and the actual terminal contents) would previously be corrupted. I suspect this change will reintroduce that problem with the EXA 'classic' scheme at least. Breakdown of the actual changed locations here: compalloc.c: notification to trigger recomposite on transition to automatic compositing. compwindow.c: 1) client notification of damage after optimized-out noop internal rendering (introduced with original import of composite, I'm not clear on why it's needed) 2) client notification of damage to trigger a redraw. damageext.c: 1) client notification of full damage in new damage struct on a window to trigger a redraw 2) adding damage to a drawable *for direct rendering that has already occurred* exa.c: Noop, replacing two function calls with the right one. glxdri: Noop, replacing two function calls with the right one. xf86Rotate.c: full damage on new shadow to trigger full shadow redraw later. Without this change, you'll still get the pending damage reported at some later date when other damage occurs to the pixmap that triggers a processing of pending damage, completely disconnected from when the rendering occurred, if any was involved. This just brings the reporting in to when it's supposed to happen, because either the rendering has already happened or it's a communication mechanism between the server internals and compositing (either automatic or manual) to trigger a redraw so that EXA has some actual rendering (which will, itself, produce damage for actual rendering). I can't see how this would make EXA any more broken. pgpCFVX8sdJKW.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/2] Replace usage of DamageRegionAppend with DamageDamageRegion to fix reportAfter.
In all these cases, any rendering implied by this damage has already
occurred, and we want to get the damage out to the client. Some of
the DamageRegionAppend calls were explicitly telling damage to flush
the reportAfter damage out, but not all.
Bug #30260. Fixes the compiz wallpaper plugin with client damage
changed to reportAfter.
Signed-off-by: Eric Anholt e...@anholt.net
---
composite/compalloc.c |2 +-
composite/compwindow.c|4 ++--
damageext/damageext.c |4 ++--
exa/exa.c |3 +--
glx/glxdri.c |4 +---
hw/xfree86/modes/xf86Rotate.c |2 +-
6 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/composite/compalloc.c b/composite/compalloc.c
index 47d5c0a..c86eb9b 100644
--- a/composite/compalloc.c
+++ b/composite/compalloc.c
@@ -238,7 +238,7 @@ compFreeClientWindow (WindowPtr pWin, XID id)
DamageRegister (pWin-drawable, cw-damage);
cw-damageRegistered = TRUE;
pWin-redirectDraw = RedirectDrawAutomatic;
- DamageRegionAppend(pWin-drawable, pWin-borderSize);
+ DamageDamageRegion(pWin-drawable, pWin-borderSize);
}
if (wasMapped !pWin-mapped)
{
diff --git a/composite/compwindow.c b/composite/compwindow.c
index 8849dc3..d17ff77 100644
--- a/composite/compwindow.c
+++ b/composite/compwindow.c
@@ -519,7 +519,7 @@ compCopyWindow (WindowPtr pWin, DDXPointRec ptOldOrg,
RegionPtr prgnSrc)
RegionTranslate(prgnSrc,
pWin-drawable.x - ptOldOrg.x,
pWin-drawable.y - ptOldOrg.y);
- DamageRegionAppend(pWin-drawable, prgnSrc);
+ DamageDamageRegion(pWin-drawable, prgnSrc);
}
cs-CopyWindow = pScreen-CopyWindow;
pScreen-CopyWindow = compCopyWindow;
@@ -598,7 +598,7 @@ compSetRedirectBorderClip (WindowPtr pWin, RegionPtr
pRegion)
/*
* Report that as damaged so it will be redrawn
*/
-DamageRegionAppend(pWin-drawable, damage);
+DamageDamageRegion(pWin-drawable, damage);
RegionUninit(damage);
/*
* Save the new border clip region
diff --git a/damageext/damageext.c b/damageext/damageext.c
index f5265dd..4aa0ff3 100644
--- a/damageext/damageext.c
+++ b/damageext/damageext.c
@@ -222,7 +222,7 @@ ProcDamageCreate (ClientPtr client)
if (pDrawable-type == DRAWABLE_WINDOW)
{
pRegion = ((WindowPtr) pDrawable)-borderClip;
- DamageRegionAppend(pDrawable, pRegion);
+ DamageDamageRegion(pDrawable, pRegion);
}
return Success;
@@ -292,7 +292,7 @@ ProcDamageAdd (ClientPtr client)
* screen coordinates like damage expects.
*/
RegionTranslate(pRegion, pDrawable-x, pDrawable-y);
-DamageRegionAppend(pDrawable, pRegion);
+DamageDamageRegion(pDrawable, pRegion);
RegionTranslate(pRegion, -pDrawable-x, -pDrawable-y);
return Success;
diff --git a/exa/exa.c b/exa/exa.c
index fc15c24..8adf847 100644
--- a/exa/exa.c
+++ b/exa/exa.c
@@ -159,8 +159,7 @@ exaPixmapDirty (PixmapPtr pPix, int x1, int y1, int x2, int
y2)
return;
RegionInit(region, box, 1);
-DamageRegionAppend(pPix-drawable, region);
-DamageRegionProcessPending(pPix-drawable);
+DamageDamageRegion(pPix-drawable, region);
RegionUninit(region);
}
diff --git a/glx/glxdri.c b/glx/glxdri.c
index 41482c9..6458ef9 100644
--- a/glx/glxdri.c
+++ b/glx/glxdri.c
@@ -834,9 +834,7 @@ static void __glXReportDamage(__DRIdrawable *driDraw,
RegionInit(region, (BoxPtr) rects, num_rects);
RegionTranslate(region, pDraw-x, pDraw-y);
-DamageRegionAppend(pDraw, region);
-/* This is wrong, this needs a seperate function. */
-DamageRegionProcessPending(pDraw);
+DamageDamageRegion(pDraw, region);
RegionUninit(region);
__glXleaveServer(GL_FALSE);
diff --git a/hw/xfree86/modes/xf86Rotate.c b/hw/xfree86/modes/xf86Rotate.c
index fdc38c5..57c3499 100644
--- a/hw/xfree86/modes/xf86Rotate.c
+++ b/hw/xfree86/modes/xf86Rotate.c
@@ -168,7 +168,7 @@ xf86CrtcDamageShadow (xf86CrtcPtr crtc)
if (damage_box.x2 pScreen-width) damage_box.x2 = pScreen-width;
if (damage_box.y2 pScreen-height) damage_box.y2 = pScreen-height;
RegionInit(damage_region, damage_box, 1);
-DamageRegionAppend ((*pScreen-GetScreenPixmap)(pScreen)-drawable,
+DamageDamageRegion ((*pScreen-GetScreenPixmap)(pScreen)-drawable,
damage_region);
RegionUninit(damage_region);
crtc-shadowClear = TRUE;
--
1.7.2.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/2] Replace usage of DamageRegionAppend with DamageDamageRegion to fix reportAfter.
On Thu, 28 Oct 2010 20:46:22 -0700, Eric Anholt e...@anholt.net wrote: In all these cases, any rendering implied by this damage has already occurred, and we want to get the damage out to the client. Some of the DamageRegionAppend calls were explicitly telling damage to flush the reportAfter damage out, but not all. Bug #30260. Fixes the compiz wallpaper plugin with client damage changed to reportAfter. Signed-off-by: Eric Anholt e...@anholt.net Reviewed-by: Keith Packard kei...@keithp.com (testers on other graphics chips are encouraged to give this a try so we don't have to revert the revert of the revert) -- keith.pack...@intel.com pgptop09zq9Xp.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
