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

[Michel Dänzer]

On Mon, 2010-11-15 at 11:24 -0800, Jeremy Huddleston wrote: 
> 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.
> 
> I read through your concerns in the replies to the patch submissions. 

Thanks.

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

It's not speculation.


> 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)."

So the problem obviously can't be with the rendering paths.
Incidentally, I pointed out repeatedly that it isn't.


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

In addition to my previous explanations in the review thread of the
original patch, see commit 29586101dc11d498b212510f8dedbfeca7f8c859
('EXA: Only record damage generated by rendering operations') where I
fixed the problem with xterm described earlier in that thread (btw, I
forgot to mention the problem didn't only manifest itself in that
particular corner case but also sometimes when opening a context menu in
xterm or other apps using the same toolkit).

Later, Maarten's commit 1861250cd7e84b05e8298b74e3c7e97da72ddfba
('{damage,exa}: sanitise damage') undid that, which only didn't
reintroduce the problem above because ProcDamageCreate only called
DamageRegionPending (now DamageRegionAppend) but not
damageRegionSubmitted (now DamageRegionProcessPending).

I have previously admitted in the other thread that relying on
non-rendering paths not calling DamageRegionProcessPending isn't a
perfect solution for telling them apart from rendering paths (and have
suggested a possible better solution), but this change regresses it from
mostly working to being unable to work.

Maybe my wording was too humble before, let me try to be clear now: This
will result in some users experiencing visual corruption that wasn't
there before. The only question in my mind is how many users will be hit
and how hard. I suspect I merely couldn't reproduce the problem anymore
because I'm using KMS (which implies the EXA 'mixed' pixmap memory
management scheme, which keeps the GPU copy of the pixmap contents fully
valid most of the time), I'm concerned this could become a case of
leaving non-KMS (which implies the EXA 'classic' pixmap memory
management scheme, which may only keep each pixel of a pixmap valid in
either copy at any time) users in the dust.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer