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

[Ben Widawsky]

On Mon, Mar 07, 2016 at 10:11:11PM -0800, Matt Turner wrote:
> 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?
> 

No. If somehow the query from the kernel fails, we get nothing. That might be
some inexplicable IOCTL fail, or some bug in the kernel. In this case, I'd like
mesa to keep running, since by and large, nobody really cares about brand
strings except irrelevant people.

Note that in a previous patch, we fall back to sane defaults under that failure
condition.

> > +      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.
> 

Do you want me to fix this up? AFAICS, I didn't actually make anything less
threadsafe.

> > +   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>

Thanks. Just make sure you're okay with my comments above. I got the rest.

-- 
Ben Widawsky, Intel Open Source Technology Center