[Pixman] [PATCH] MIPS: DSPr2: Basic infrastructure for MIPS architecture

[Lukic, Nemanja]

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:sandmann at cs.au.dk] 
Sent: Wednesday, January 18, 2012 6:20 PM
To: Lukic, Nemanja
Cc: pixman at 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