[PATCH 11/14] damageext: Xineramify (v6)

[Adam Jackson]

On Mon, 2013-12-02 at 15:38 -0800, Keith Packard wrote:
> Adam Jackson <ajax at redhat.com> writes:
> 
> I've been reading code this afternoon, and I think your approach is
> subtly broken.
> 
> PanoramiXDamageQueue traps the damage additions to screen 0, creates a
> Dispatch callback and then waits until the request completes before
> merging damage. Other screens call PanoramiXDamageAccumulate.
> 
> This assumes that damage will be delivered to all screens, which I don't
> think is always the case as rendering which is clipped out will generate
> no damage, and hence no callback.

No.  Accumulate -> DamageReportDamage on screen 0 -> Queue.  Even if
clipping means there's no damage to screen 0's logical aperture, screen
0's DamageExtPtr tracks the whole protocol screen, and its callback
always gets invoked.

> I think you just want to have all of the screens hit the same callback
> and add damage to the screen 0 instance; that way, it won't matter which
> subset of the screens receive damage.

That is what happens, yes.

Probably this could be made more obvious by actually using the same
callback on all screens, and folding the non-screen-0 logic in directly.
I vaguely recall trying to write it that way and hitting some bug, which
was likely my own fault rather than some fundamental incompatibility,
but since I had it working with the Accumulate/Queue split I decided not
to worry about it.

> Independently, I'm not thrilled with a DIX-level DispatchCallback, but I
> have to admit that I don't have a better solution -- adding it down
> inside panoramix would mean touching every single rendering function to
> process the queued damage, and there's no other place between the
> panoramix wrapper and Dispatch.

Yeah, I didn't like it either.  I had hoped to be able to just do the
event emission from screen 0's callback, but not all the rendering paths
do FOR_NSCREENS_BACKWARD.  And in a previous iteration of this patch I'd
used FlushCallback, but WriteEventsToClient can itself flush so that can
be infinite recursion.

The other thing I don't like is how AddCallback mallocs, which you'd
like not need to do on every rendering request.  But between damage
itself mallocing the regions and the miTranslate thing we're already
pretty heavyweight here.

We could probably make this prettier by splitting request processing
into validate/do/event/reply phases at the top level.  It'd be a huge
pile of churn, so it's really not 1.15 material, and I'd be worried
about changing event emission order which is effectively client ABI at
this point.  DispatchCallback seemed like the least ugly and most
expedient thing.

- ajax