Hi Soren, All MIPS DSP related files are renamed to have "dspr2" string. Run-time detection of the DSP extensions is left as it was in the original patch. Yes, there is no easy way to run-time distinguish if CPU supports DSPr1 or DSPr2. In case of "ddsp" string in the cpuinfo, we don't want to return TRUE. TRUE should be returned only if there is just "dsp" string found. Coding style issues are fixed. Benchmark results (including both lowlevel-blt-bench and cairo-perf-trace) are included in the log message of the commit containing initial SRC optimizations.
New patches (two patches, one with the basic infrastructure and one with the initial optimization of the SRC routines) are sent for review. Any comments to these patches are welcome. Best Regards, Nemanja Lukic -----Original Message----- From: Søren Sandmann [mailto:sandm...@cs.au.dk] Sent: Wednesday, January 18, 2012 6:20 PM To: Lukic, Nemanja Cc: pixman@lists.freedesktop.org Subject: Re: [Pixman] [PATCH] MIPS: DSPr2: Basic infrastructure for MIPS architecture Hi, Thanks for the patch. The main comment I have is about being precise that this is about DSPr2 and not r1. This has some consequences: - I think a consistent string "dspr2" should be used to refer to this functionality. Ie., the files should be called pixman-mips-dspr2.[ch], the functions and variables should have "dspr2" in their names, and not "dsp" etc. The SSE2 implementation used to use "sse" and "sse2" interchangably and it resulted in total confusion. - The runtime CPU detection seems to check whether the "dsp" is found in /proc/cpuinfo. However, from reading the kernel source, it looks like "dsp" only indicates that rev. 1 of the DSP ASE is available. I also think the search function may be buggy. Consider what happens when given the string "ddsp" > + while (EOF != (k = fgetc(f))) { > + if (k == *what) { > + ++what; > + while ((*what != '\0') && (*what == fgetc(f))) { > + ++what; > + } > + if (*what == '\0') { > + fclose(f); > + return TRUE; > + } else { > + what = search_string; > + } > + } > + } Consider what it does when given the string "ddsp". The "k == *what will be true, and so the second fgetc(f) will eat the second 'd'. Since *what is not \0 at this point, it will be reset to "dsp", but the file is now at the "sp" point. Ie., it will miss the "dsp" string in "ddsp". Personally, I think I'd do it as just a loop using fgets() into some reasonable-sized buffer, and then checking each line with strstr(). But the bigger problem is that the kernel apparently doesn't offer any way to detect dspr2 that I can tell. Also, there are some coding style issues. Please see the file CODING_STYLE. In particular, - Use /* ... */ for comments, not // - Don't use braces with single-line statements - Put braces on their own line - Format functions with their return type on its own line Finally, feel free to send all the patches that you think are ready to be reviewed. There is no need to wait for each one to be reviewed. Note that git send-email <last commit-id before your patches> is convenient for this. Søren _______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
