[PATCH] damage: Only track extents where possible

[Chris Wilson]

On Thu, Feb 26, 2015 at 02:58:45PM -0500, Adam Jackson wrote:
> 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.

x11perf -aa10text certainly prefers extents-only tracking here. I don't
imagine any other test would regress, but I can check for completeness.

> > +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.

It was a bit cheeky, and it definitely relies on the current code in regionstr.h.
Looking at it again, we can't discard the rectangles from pDamageRegion
as we may be called before the op and so required to keep pPendingDamage
intact.
 
> > @@ -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.

In tree users:

DamageReportNone:
  ephyr - region is used for host migration
  exa - region is used for video memory migrations
  modesetting - region is used to limit shadow updates
  randr - region is used during composite
  shadow(mi) - region is exposed to clients

DamageReportNonEmpty:
  composite - region is used
  exa - region is used
  glamor -  region ignored
  xwayland - only extents used

DamageReportRawRegion:
  shadow(xf86) - rectangles are exposed to clients

The DamageReportNone and DamageReportRawRegion the rectangles are part
of the respective API (though we could just report the extents
rectangle), but for e.g. drm/udl (modesetting, shadow) at least we do
want the raw rectangles and the migration would be very expensive. randr
doesn't know if it will be quick or slow, so must err on the side of
tracking the rectangles. ephyr should be able to determine if it has
quick updates via DRI2/SHM or pushing pixels over the wire.

As for DamageReportNonEmpty, exa has to pessimise that pixel data is slow
to readback and so should track rectangles. xwayland/glamor already
ignore the rectangles. composite however doesn't know whether it will be
faster to ignore rectangles (because the copy is hwaccel) or to track
rectangles... Which is where we can make a decision based on xvfb +
xcompgr -a (and also check with the major ddx).

So because of exa, I think we want a DamageSetTrackExtentsOnly()
function call so that consumers can opt out of the full region tracking.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre