DamageRegionAppend versus DamageDamageRegion (was Re: xserver: Branch 'server-1.9-branch' - 3 commits)

[Jeremy Huddleston]

On Nov 15, 2010, at 01:33, Michel Dänzer wrote:

> On Sam, 2010-11-13 at 15:21 -0800, Jeremy Huddleston wrote: 
>> 
>> commit dfda3c696dd72ecc5cc4fa69d8bb4521ba554cf3
>> Author: Eric Anholt <e...@anholt.net>
>> Date:   Thu Oct 28 20:46:22 2010 -0700
>> 
>>    Replace usage of DamageRegionAppend with DamageDamageRegion to fix 
>> reportAfter.
> 
> I can understand that Eric and Keith don't care about breaking EXA, but
> it's sad to see that apparently that's considered okay even for a
> 'stable' branch.
> 0

I read through your concerns in the replies to the patch submissions.  I 
understand that you have concerns that there might be an interaction issue with 
EXA, but that is speculation that doesn't seem backed up.  This patch series 
(which I cherry-picked in as it was in master to maintain logging despite the 
apply-revert-reapply of the related 8d7b7a0d71... patch) addresses an existing 
bug (#30260), and I suspect that most distributions would pull these changes 
into their builds.  

Maybe this is a difference in how we view the stable branch.  I view it as a 
branch with a locked API and a minor set of changes that most distributions and 
end users would prefer.  I believe that end users would prefer that #30260 be 
addressed rather than hold it back on (unsubstantiated) suspicion that there 
may be some difference in EXA.  As Keith said in his response to you, "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)."

     RegionInit(&region, &box, 1);
-    DamageRegionAppend(&pPix->drawable, &region);
-    DamageRegionProcessPending(&pPix->drawable);
+    DamageDamageRegion(&pPix->drawable, &region);
     RegionUninit(&region);

given that (my comments added as //...):

void
DamageRegionAppend (DrawablePtr pDrawable, RegionPtr pRegion)
{
    damageRegionAppend (pDrawable, pRegion, FALSE, -1);
}

void
DamageRegionProcessPending (DrawablePtr pDrawable)
{
    damageRegionProcessPending (pDrawable);
}

void
DamageDamageRegion (DrawablePtr pDrawable,
                    RegionPtr   pRegion)
{
    damageRegionAppend (pDrawable, pRegion, FALSE, -1); // Same as 
DamageRegionAppend(pDrawable, pRegion)

    /* Go back and report this damage for DamagePtrs with reportAfter set, since
     * this call isn't part of an in-progress drawing op in the call chain and
     * the DDX probably just wants to know about it right away.
     */
    damageRegionProcessPending (pDrawable); // Same as 
DamageRegionProcessPending(pDrawable)
}

I fail to see how this could have the detrimental impact you suspect.  For EXA 
and GLX, the change is readability.  For others, the functional change is that 
now damageRegionProcessPending is called immediately rather than queuing them 
up.

Your response to Keith was, "I know, as I said, the problem is when there's 
*no* rendering operation between DamageRegionAppend and 
DamageRegionProcessPending." which seems tangental and not relevant to this 
change set.  If that actually is a problem, it was a problem before, and it 
remains a problem after.

If you can prove that there is some functional impact, I will certainly revert 
the changes in 1.9.3 RC2, but that is on you to prove.

--Jeremy

_______________________________________________
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