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

[Matt Turner]

On Wed, Mar 9, 2016 at 10:36 AM, Ben Widawsky <ben at bwidawsk.net> wrote:
> 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.

Fine by me.

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

Nope. It was just something I noticed.

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

Yep!