[Cogl] [PATCH 1/2] gpu-info: Detect more info including architecture

[Robert Bragg]

On Tue, May 15, 2012 at 12:27 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Robert Bragg <robert at sixbynine.org> writes:
>
>> +static CoglBool
>> +check_sgx_architecture (const CoglGpuInfoStrings *strings)
>> +{
>> +  if (strncmp (strings->renderer_string, "PowerVR ", 9) != 0)
>> +    return FALSE;
>> +
>> +  /* In some cases you'll see "PowerVR SGX535" and in others it may be
>> +   * "PowerVR SGX 535"
>> +   */
>> +  if (strncmp (&strings->renderer_string[8], "SGX", 3) != 0)
>> +    return FALSE;
>
> Why does it do this comparison in two parts? Isn't this equivalent to
> just strncmp (strings->renderer_string, "PowerVR SGX", 12)?

Oops, I think at one point I was using the match_phrase function here
which would get upset withing something like PowerVR SGX535 and then
didn't think it through when I updated the code.

>
>> +static CoglBool
>> +check_mesa_vendor (const CoglGpuInfoStrings *strings)
>> +{
>> +  if (strcmp (strings->vendor_string, "Tungsten Graphics, Inc") == 0)
>> +    return TRUE;
>> +  else if (strcmp (strings->vendor_string, "VMware, Inc.") == 0)
>> +    return TRUE;
>> +
>> +  return TRUE;
>> +}
>
> This function always returns TRUE!

Er, yeah hmm, oops the last one should return FALSE to fall through to
the "Unknown" vendor.

>
>>      /* Must be last */
>>      {
>> +      COGL_GPU_INFO_VENDOR_MESA,
>> +      "Mesa",
>> +      check_mesa_vendor,
>> +      mesa_architectures
>> +    },
>> +    {
>>        COGL_GPU_INFO_VENDOR_UNKNOWN,
>>        "Unknown",
>> -      check_unknown_vendor
>> +      check_true
>>      }
>>    };
>
> It looks like the 'must be last' comment ends up in the wrong place
> after this patch.

No, this was intentional, since the "Mesa" vendor is intended as a
fallback for mesa drivers if we don't find a better hardware vendor
name. Although at the moment this could go higher up and it wouldn't
be a problem (modulo the bug you pointed out) since it checks for
explicit vendor strings, we might later make it check for (Mesa|mesa)
in any of the strings which would be more likely to match than the
hardware vendor check functions.

>
> The 'unknown' vendor doesn't have any architectures so the pointer
> remains NULL and it will crash if its actually hit.

Ah oops.

>
> Otherwise looks good to me.
>
> Reviewed by: Neil Roberts <neil at linux.intel.com>
>
> - Neil

Sorry the patch was a bit sloppy, but thanks for the review.

regards,
- Robert