[cairo] Pushing vmx patches in pixman

[Luca Barbato]

Soeren Sandmann wrote:
> I don't have a PowerPC, so I can't test this myself. The minimum
> amount of testing that needs to be done is:
> 
>         (a) An X server linked to pixman works and displays a GNOME
>         desktop correctly, specifically without artefacts in the icon
>         rendering.
> 
>         (b) Rendercheck run against the X server generally
>         passes. There are a number of options to this program. The
>         most important formats to test are 16 and 24 bit RGB formats,
>         and 8 bit alpha formats. The most important operations are
>         BLEND, OVER, ADD, IN, CLEAR.
> 
>         (c) The cairo test suite passes when run like this: 
> 
>                 env CAIRO_TEST_TARGET=image make test
> 
> It would also be good to see cairo-perf results to verify that this is
> actually a speed-up.

I'll run those tests soonish, the last time I submitted this code it 
passed cairo's make test, I'll check again since it was long ago.

> 
> Below are some comments on the code, but I don't know the VMX
> instruction set, so there is a limit to how much I can comment.

Thank you for the review nonetheless.

> 
> One general thing I noticed is that the code uses 'inline' without any
> qualifiers. In my experience this causes gcc to not always inline the
> functions. 
> 

added always_inline as you suggested.

> Can we just do this instead:
> 
>         #ifdef USE_VMX
>         if (!info && pixman_have_vmx())
>                 info = get_fast_path (...);
>         #endif
> 
> for all the architecture specific clauses? (I know this was not
> introduced by you, but I think we should get this fixed before it
> spreads further).

Updated as you suggested.

> This strikes me a overly complicated. Is there any reason we can't
> just have a global variable that gets set in the signal handler, so we
> don't have to think about longjumps?  Also, I really don't think
> raising a signal on every compositing operation is a good idea.
> 

Rewrote as suggested.

> I don't think you use this VMX constant anywhere, and the CPUFeatures
> enum is more intended for x86 CPU's anyway. In any case it certainly
> shouldn't alias with SSE ...

Right, removed.

> This macro is not used at all, and it wouldn't work if it were because
> of the space between LOAD_VECTOR and (source).

Removed as well.

> 
> Finally, you have cutted and pasted all the fbMulAdd macros. I can see
> why you did that, but it would be better if we can make the combine.pl
> generate them in their own headerfiles (pixman-arithmetic{32,64}.h,
> maybe) so we don't have to have this code duplicated.

I'd split it from combine.inc as a separate header with the arith macros 
and just include below the mask definitions, is that ok for you?

lu

-- 

Luca Barbato
Gentoo Council Member
Gentoo/linux Gentoo/PPC
http://dev.gentoo.org/~lu_zero