[PATCH] hw/xfree86: Fix block handler wrapping in xf86Rotate

[Keith Packard]

Eric Anholt <eric at anholt.net> writes:

> I'm uncomfortable with you mashing these 3 changes together,
> particularly the reinsertion change in what was supposed to be just a
> bugfix commit.  It didn't take much grepping for me to find more code
> that wasn't rewrapping the blockhandler according to these rules:

Yeah, stuff which doesn't dynamically wrap, and which wraps at server
init time I didn't worry about as it can't cause problems of the nature
I found. But, it would be best to have everything following the rules
From a documentation perspective. I audited the code which wraps after
server init time (which are also the only ones which unwrap) and made
sure all of those were correct; those are the only ones which could
potentially cause problems.

Given that the software cursor code *does* unwrap, having more code
which operates in the same fashion is just going to expose the same bugs
in other ways though; I guess that's "worse" in some metric?

But, yes, splitting the patch into two pieces would be more sensible,
and also auditing the static wrapping code to make sure it follows the
same style guidelines for consistency would be a good idea.

There are only a few users of the BlockHandler in the core server:

    composite/compalloc.c

        Dynamic and correct.

    exa/exa.c

        Static and correct

    hw/kdrive/src/kdrive.c

        Not a wrapper, this is the bottom function
        
    hw/xfree86/common/xf86VGAarbiter.c

        Static and incorrect

    hw/xfree86/dri/dri.c

        This is abusing the screen BlockHandler typedef

    hw/xfree86/modes/xf86Rotate.c

        Dynamic and currently incorrect.

    hw/xwin/winvideo.c
        
        Not actually being used (the code is commented out)

    mi/misprite.c

        Dynamic and incorrect. (although it does work currently).
        
    render/animcur.c

        Dynamic and correct.
        
misprite should clearly be fixed; it happens to work currently because
nothing that it calls changes BlockHandlers, but it calls a *lot* of
stuff...

Given that we have two layers which require correct wrapping behavior,
I'd suggest fixing the core server to do it right everywhere and then
letting the rotate code also expect correct behavior. That should be a
sequence of probably five patches:

        1) Add the comment
        2) Fix xf86Rotate to be correct
        3) Fix misprite to be correct
        4) fix xf86VGAarbiter to be correct
        5) change xf86Rotate to leave things unwrapped

Every video driver also has a block handler, and I'm pretty sure they're
all wrapping the miscrinit.c setting of NoopDDA as there doesn't appear
to be another one anywhere. In any case, I think these are all static,
and so it would be nice to make them correct, but it would probably be
nicer to just not have them wrapping at all.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140416/919259fb/attachment.sig>