Re: [Pixman] [PATCH 11/12] MIPS: runtime detection extended

2013-09-27 Thread Matt Turner
On Thu, Sep 19, 2013 at 3:00 PM, Søren Sandmann  wrote:
> I assume there is a good reason for those spaces in front of the
> keywords, but they definitely set off my "wrong formatting" detector,
> especially because there is no space in front of "Loongson" and because
> Loongson is capitalized while they others are not.

Not sure about the spaces, but the kernel sources display "ICT
Loongson-2" or "Loongson 1B" (the latter of which I don't believe have
MMI).

> Do we know if the loongson MMI instruction set shows up in /proc/cpuinfo
> in newer kernel versions?

I checked the upstream kernel and I don't see any evidence that it does.
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman


Re: [Pixman] [PATCH 11/12] MIPS: runtime detection extended

2013-09-19 Thread Søren Sandmann
Nemanja Lukic  writes:

> This patch extend runtime detection in way that for kernel version
> newer than 3.7, searches for strings in added fields to file
> /proc/cpuinfo: "isa" and "ASEs implemented". Check for the exact MIPS
> core types is used for older kernel versions.
> ---
>  pixman/pixman-mips.c |   46 +-
>  1 files changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/pixman/pixman-mips.c b/pixman/pixman-mips.c
> index eadf912..53c1023 100644
> --- a/pixman/pixman-mips.c
> +++ b/pixman/pixman-mips.c
> @@ -53,7 +53,7 @@ static const char *mips_loongson_cores[] = {"Loongson", 
> NULL};
>  defined(USE_MIPS32R2) || defined(USE_LOONGSON_MMI)
>  
>  static pixman_bool_t
> -have_feature (const char **cores)
> +have_feature (const char **cores, const char *search_string)
>  {
>  #if defined (__linux__) /* linux ELF */
>  /* Simple detection of MIPS features at runtime for Linux.
> @@ -62,18 +62,46 @@ have_feature (const char **cores)
>   * facility is universally available on the MIPS architectures, so it's 
> up
>   * to individual OSes to provide such.
>   */
> -const char *file_name = "/proc/cpuinfo";
> -char cpuinfo_line[256];
> +const char *file_name = "/proc/version";
> +char line[256];
> +int v0 = 0, v1 = 0;
>  FILE *f = NULL;
>  
> +if ((f = fopen (file_name, "r")) != NULL)
> +{
> +if (fgets (line, sizeof (line), f) != NULL)
> +{
> +sscanf (line, "%*s%*s%d%*c%d", &v0, &v1);
> +}

Formatting nitpick: Braces should not be used around a single line.

> +fclose (f);
> +}
> +
> +file_name = "/proc/cpuinfo";
> +
>  if ((f = fopen (file_name, "r")) == NULL)
>  return FALSE;
>  
> +if ((v0 >= 3) && (v1 >= 7))
> +{

Is there any possibility that they keywords you search could show up in
older kernels as false positives? If not, it seems like they could just
be searched for unconditionally.

> +/* isa filed (mips32r2) is available from kernel version 3.9
> + * ASEs implemented field (dsp, dsp2) is available from 3.7
> + */
> +while (fgets (line, sizeof (line), f) != NULL)
> +{
> +if (strstr (line, search_string) != NULL)
> +{
> +fclose (f);
> +return TRUE;
> +}
> +}
> +rewind(f);
> +}
> +

If you changed the nesting of the loops as mentioned in the earlier
mail, you could also absorb the search string check into that loop.

Also, with these new fields, is there a possibility that 256 byte lines
is no longer enough? On my Intel CPU, the "flags" line in /proc/cpuinfo
is 467 bytes long.

>  while (*cores)
>  {
> -while (fgets (cpuinfo_line, sizeof (cpuinfo_line), f) != NULL)
> +while (fgets (line, sizeof (line), f) != NULL)
>  {
> -if (strstr (cpuinfo_line, *cores) != NULL)
> +if (strstr (line, *cores) != NULL)
>  {
>  fclose (f);
>  return TRUE;
> @@ -100,7 +128,7 @@ _pixman_mips_get_implementations (pixman_implementation_t 
> *imp)
>  #ifdef USE_LOONGSON_MMI
>  /* I really don't know if some Loongson CPUs don't have MMI. */
>  if (!_pixman_disabled ("loongson-mmi") &&
> -have_feature (mips_loongson_cores))
> +have_feature (mips_loongson_cores, "Loongson"))
>   imp = _pixman_implementation_create_mmx (imp);
>  #endif
>  
> @@ -117,7 +145,7 @@ _pixman_mips_get_implementations (pixman_implementation_t 
> *imp)
>  already_compiling_everything_for_mips32r2 = 1;
>  #endif
>  if (already_compiling_everything_for_mips32r2 ||
> -have_feature (mips32r2_cores))
> +have_feature (mips32r2_cores, " mips32r2"))
>  {
>  imp = _pixman_implementation_create_mips32r2 (imp);
>  }
> @@ -132,7 +160,7 @@ _pixman_mips_get_implementations (pixman_implementation_t 
> *imp)
>  already_compiling_everything_for_dspr1 = 1;
>  #endif
>  if (already_compiling_everything_for_dspr1 ||
> -have_feature (mips_dspr1_cores))
> +have_feature (mips_dspr1_cores, " dsp"))
>  {
>  imp = _pixman_implementation_create_mips_dspr1 (imp);
>  }
> @@ -147,7 +175,7 @@ _pixman_mips_get_implementations (pixman_implementation_t 
> *imp)
>   already_compiling_everything_for_dspr2 = 1;
>  #endif
>   if (already_compiling_everything_for_dspr2 ||
> - have_feature (mips_dspr2_cores))
> + have_feature (mips_dspr2_cores, " dsp2"))
>   {
>   imp = _pixman_implementation_create_mips_dspr2 (imp);
>   }

I assume there is a good reason for those spaces in front of the
keywords, but they definitely set off my "wrong formatting" detector,
especially because there is no space in front of "Loongson" and because
Loongson is capitalized while they others are not.

Do we know if the lo