[Pixman] [PATCH 0/3] NEON optimizations for bilinear scaling with A8 mask for operator src, over, add

[Siarhei Siamashka]

On Tue, Apr 12, 2011 at 4:26 PM, Taekyun Kim <podain77 at gmail.com> wrote:
> Hi,
> I send a series of patches for NEON optimizations of bilinear scaling.
> It's based on latest snapshot of pixman master repository.
> First patch defines a new common header macro for bilinear a8 mask scanline
> function.
> In second patch, I implemented some bilinear scanline functions in a new
> file pixman-arm-neon-asm-bilinear.S with modified Makefile.
> Last patch just enables additional fast paths into pixman_implementation_t.
> I'm sorry about the test failure in previous patch.
> I used mask format symbol 'x' to indicate that there's no mask instead of
> 'n' which generally means 'solid mask'.
> So, in this convention,
> over_8888_x_8888
> add_8888_x_8888
> src_8888_8_8888
> over_8888_8_8888
> add_8888_8_8888
> these functions are implemented in the new file.
> It passes pixman scaling test now.
> Any kind of review would be appreciated.

Looks almost ok now. A few things should be still corrected though:
1. License/copyright notice header is needed for the newly added file.
It can be taken from the older NEON assembly file and updated to have
both Nokia and your (Samsung?) copyright for year 2011.
2. Fix indentation problems according to:
http://cgit.freedesktop.org/pixman/tree/CODING_STYLE?id=pixman-0.21.6#n22
3. More descriptive commit messages would be welcome for patches. And
commit summaries are better to start with 'ARM: '.

It's a bit regrettable that there are actually no fast paths for solid
mask and r5g6b5 color format. I think at least 'src_8888_8_0565' could
work fine and justify the existence of 565 related macros in the code.


And one more nitpick is about this part:
+ mov ip, sp
+ push {r4, r5, r6, r7, r8, r9, r10}

It's usually a good idea to save even number of registers on function
entry to keep the stack 8 bytes aligned. The 8 bytes alignment is
required by EABI and is quite critical if you for example call some
external functions or callbacks from your code or perform 64-bit
aligned memory accesses with stack. Even though this is not strictly
needed here, it's safer just in case. On ARM Cortex-A8 this has no
impact on performance.

Hopefully I did not miss anything important.

-- 
Best regards,
Siarhei Siamashka