[Mesa-dev] [PATCH 5/5] i965/chv: Display proper branding

[Matt Turner]

On Mon, Mar 7, 2016 at 5:39 PM, Ben Widawsky
<benjamin.widawsky at intel.com> wrote:
> "Braswell" is a Cherryview based *thing*. It unfortunately requires extra
> information to determine its marketing name. Unlike all previous products, and
> hopefully all future ones, there is no unique 1:1 mapping of PCI device ID to
> brand string.
>
> I put up a fight about adding any complexity to our GL renderer string code for
> a very long time. However, a wise man made a comment to me that I couldn't argue
> with: if a user installs Windows on their hardware, the brand string should be
> the same as what we display in Linux. The Windows driver apparently does this
> check, so we should too.
>
> Note that I did manage to find a good use for this info anyway in the computer
> shader thread counts.
>
> Cc: Kaveh Nasri <kaveh.nasri at intel.com>
> Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
> ---
>  include/pci_ids/i965_pci_ids.h           |  4 ++--
>  src/mesa/drivers/dri/i965/brw_context.c  | 33 +++++++++++++++++++++++++++++---
>  src/mesa/drivers/dri/i965/brw_context.h  |  3 ++-
>  src/mesa/drivers/dri/i965/intel_screen.c |  2 +-
>  4 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/include/pci_ids/i965_pci_ids.h b/include/pci_ids/i965_pci_ids.h
> index bdfbefe..d783e39 100644
> --- a/include/pci_ids/i965_pci_ids.h
> +++ b/include/pci_ids/i965_pci_ids.h
> @@ -156,8 +156,8 @@ CHIPSET(0x5932, kbl_gt4, "Intel(R) Kabylake GT4")
>  CHIPSET(0x593A, kbl_gt4, "Intel(R) Kabylake GT4")
>  CHIPSET(0x593B, kbl_gt4, "Intel(R) Kabylake GT4")
>  CHIPSET(0x593D, kbl_gt4, "Intel(R) Kabylake GT4")
> -CHIPSET(0x22B0, chv,     "Intel(R) HD Graphics (Cherryview)")
> -CHIPSET(0x22B1, chv,     "Intel(R) HD Graphics (Cherryview)")
> +CHIPSET(0x22B0, chv,     "Intel(R) HD Graphics (Cherrytrail)")
> +CHIPSET(0x22B1, chv,     "Intel(R) HD Graphics XXX (Braswell)") /* Overriden in brw_get_renderer_string */

Typo: Overridden

>  CHIPSET(0x22B2, chv,     "Intel(R) HD Graphics (Cherryview)")
>  CHIPSET(0x22B3, chv,     "Intel(R) HD Graphics (Cherryview)")
>  CHIPSET(0x0A84, bxt,     "Intel(R) HD Graphics (Broxton)")
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index df0f6bb..f57184f 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -77,13 +77,27 @@
>
>  const char *const brw_vendor_string = "Intel Open Source Technology Center";
>
> +static const char *
> +get_bsw_model(const struct intel_screen *intelScreen)
> +{
> +   switch (intelScreen->eu_total) {
> +   case 16:
> +      return "405";
> +   case 12:
> +      return "400";
> +   default:

I think this is safe to just mark unreachable(), right?

> +      return "   ";
> +   }
> +}
> +
>  const char *
> -brw_get_renderer_string(unsigned deviceID)
> +brw_get_renderer_string(const struct intel_screen *intelScreen)
>  {
>     const char *chipset;
>     static char buffer[128];

Not your fault, but driGetRendererString() into this static buffer
isn't thread-safe. I ran into a similar problem in EGL with
shader-db's run.c last year.

> +   char *bsw = NULL;

Thought the initialization wasn't necessary at first, but indeed it is
if you want to unconditionally call free().

>
> -   switch (deviceID) {
> +   switch (intelScreen->deviceID) {
>  #undef CHIPSET
>  #define CHIPSET(id, symbol, str) case id: chipset = str; break;
>  #include "pci_ids/i965_pci_ids.h"
> @@ -92,7 +106,20 @@ brw_get_renderer_string(unsigned deviceID)
>        break;
>     }
>
> +   /* Braswell branding is funny, so we have to fix it up here */
> +   if (intelScreen->deviceID == 0x22B1) {
> +      char *needle;
> +
> +      bsw = strdup(chipset);
> +      needle = strstr(bsw, "XXX");

Could declare char *needle here and initialize on one line if you wanted.

> +      if (needle) {
> +         strncpy(needle, get_bsw_model(intelScreen), strlen("XXX"));

Don't actually need (or want) any of the features of strncpy. Should
just use memcpy.

> +         chipset = bsw;
> +      }
> +   }
> +
>     (void) driGetRendererString(buffer, chipset, 0);
> +   free(bsw);
>     return buffer;
>  }
>
> @@ -107,7 +134,7 @@ intel_get_string(struct gl_context * ctx, GLenum name)
>
>     case GL_RENDERER:
>        return
> -         (GLubyte *) brw_get_renderer_string(brw->intelScreen->deviceID);
> +         (GLubyte *) brw_get_renderer_string(brw->intelScreen);
>

With the couple things fixed,

Reviewed-by: Matt Turner <mattst88 at gmail.com>