[Libva] [PATCH 1/2] Use the inline CPUID assembly to obtain CPUID string instead of /proc/cpuinfo

[Gwenole Beauchesne]

Hi,

Great to see that finally fixed.

2014-09-24 9:13 GMT+02:00 Zhao Yakui <yakui.zhao at intel.com>:
> On some systems there is no access to /proc/cpuinfo. So the inline assembly
> is used directly to detect the CPUID string.
>
> Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
> ---
>  src/i965_device_info.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/src/i965_device_info.c b/src/i965_device_info.c
> index 5ebea2a..4ffba61 100755
> --- a/src/i965_device_info.c
> +++ b/src/i965_device_info.c
> @@ -378,6 +378,7 @@ i965_get_device_info(int devid)
>      }
>  }
>
> +#if 0
>  static int intel_driver_detect_cpustring(char *model_id)
>  {
>      FILE *fp;
> @@ -416,7 +417,52 @@ static int intel_driver_detect_cpustring(char *model_id)
>      else
>          return -EINVAL;
>  }
> +#else
> +static inline void cpuid(unsigned int op,
> +                         unsigned int *eax, unsigned int *ebx,
> +                         unsigned int *ecx, unsigned int *edx)

Use uint32_t so that to not make the code below confusing when
different 32-bit/64-bit asm is involved and you are sure the operands
are 32-bits, just say so.

> +{
> +        *eax = op;
> +        *ecx = 0;
> +        /* ecx is often an input as well as an output. */
> +        asm volatile("cpuid"
> +            : "=a" (*eax),
> +              "=b" (*ebx),
> +              "=c" (*ecx),
> +              "=d" (*edx)
> +            : "0" (*eax), "2" (*ecx)
> +               :"memory");
> +}

This will likely fail in 32-bit mode. %ebx is the PIC register there.
e.g. use %edi as a temporary in this case. And split into two
versions.

Besides, choose between in/out operands (+) and keep *eax/*ecx
initializations, or use separate outputs and inputs and initialize
eax/ecx there, e.g. "a" (op), "c" (0) and drop the *eax/*ecx lines.
Don't mix both.

> +
> +/*
> + * This function doesn't check the length. And the caller should
> + * assure that the length of input string should be greater than 48.
> + */
> +static int intel_driver_detect_cpustring(char *model_id)

This is totally useless then, worded as is, and doesn't bring in more
robustness. Use dynamic allocation, or model_id + length of model_id.

> +{
> +    unsigned int *rdata;
>
> +    if (model_id == NULL)
> +        return -EINVAL;
> +
> +    rdata = (unsigned int *)model_id;
> +
> +    /* obtain the max supported extended CPUID info */
> +    cpuid(0x80000000, &rdata[0], &rdata[1], &rdata[2], &rdata[3]);
> +
> +    /* If the max extended CPUID info is less than 0x80000004, fail */
> +    if (rdata[0] < 0x80000004)
> +       return -EINVAL;
> +
> +    /* obtain the CPUID string */
> +    cpuid(0x80000002, &rdata[0], &rdata[1], &rdata[2], &rdata[3]);
> +    cpuid(0x80000003, &rdata[4], &rdata[5], &rdata[6], &rdata[7]);
> +    cpuid(0x80000004, &rdata[8], &rdata[9], &rdata[10], &rdata[11]);
> +
> +    *(model_id + 48) = '\0';

That's model_id[48] = '\0'; much simpler to read.

> +    return 0;
> +}
> +#endif
>  /*
>   * the hook_list for HSW.
>   * It is captured by /proc/cpuinfo and the space character is stripped.
> --
> 1.7.10.1
>
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libva

Regards,
-- 
Gwenole Beauchesne
Intel Corporation SAS / 2 rue de Paris, 92196 Meudon Cedex, France
Registration Number (RCS): Nanterre B 302 456 199