On Tue, Apr 12, 2011 at 4:26 PM, Taekyun Kim <[email protected]> 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 _______________________________________________ Pixman mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/pixman
