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

[Søren Sandmann]

Nemanja Lukic <nemanja.lukic at rt-rk.com> 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 loongson MMI instruction set shows up in /proc/cpuinfo
in newer kernel versions?


Søren