[PATCH] damage: Only track extents where possible

[Adam Jackson]

On Wed, 2015-02-25 at 18:01 +0000, Chris Wilson wrote:
> For external Damage, we need only track sufficient information to
> satisfy the DamageReportLevel. That is if the Client only wishes to hear
> that the Damage is now non-empty or if the extents change, we only need
> to track the extents of the Damage and can discard the actual
> rectangles. This speeds up the union operation, speeding up damage
> processing for Client as well - with a noticeable increase in
> performance of gnome-shell (which uses DamageReportBoundingBox) for
> example.

I like the idea, not quite sold on the realization.

> Internal users of Damage have access to the DamageRegion irrespective of
> the DamageReportLevel and so we need to keep the full region intact for
> them.

That's not quite what isInternal means, it's currently used to hide
software sprite rendering from the protocol event stream.  xwl and
composite also create their damages with isInternal FALSE.  For xwl that
could just be fixed since it's not initializing midc, but composite
really does want that suppression to work.

So I guess the question is whether automatic windows should take the
bounding box snap too, and I guess whichever method makes x11perf look
better when 'xcompmgr -a' is good enough for me.

> +static void DamageCombineExtents(DamagePtr pDamage, RegionPtr pDamageRegion)
> +{
> +    if (!pDamage->isInternal) {
> +        RegionUninit(pDamageRegion);
> +        RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion);
> +        RegionUninit(&pDamage->damage);
> +    } else
> +        RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion);
> +}

First time I've seen this idiom of relying on RegionUninit to empty the
rect list but leave the bounding box.  I don't necessarily have an issue
with it but a comment would be nice.

> @@ -1929,7 +1939,7 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr pDamageRegion)
>          break;
>      case DamageReportBoundingBox:
>          tmpBox = *RegionExtents(&pDamage->damage);
> -        RegionUnion(&pDamage->damage, &pDamage->damage, pDamageRegion);
> +        DamageCombineExtents(pDamage, pDamageRegion);
>          if (!BOX_SAME(&tmpBox, RegionExtents(&pDamage->damage))) {
>              (*pDamage->damageReport) (pDamage, &pDamage->damage,
>                                        pDamage->closure);

Presumably this half doesn't even need to inspect isInternal, since the
bounding box is all that's requested in any case.  There are no in-tree
BoundingBox consumers so nothing can be relying on getting anything more
detailed in the report hook, and the protocol event snaps to the box
anyway.

- ajax