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 <eric at 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