On Wed, Mar 9, 2016 at 10:36 AM, Ben Widawsky <b...@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.widaw...@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.na...@intel.com>
>> > Signed-off-by: Ben Widawsky <benjamin.widaw...@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 <matts...@gmail.com>
>
> Thanks. Just make sure you're okay with my comments above. I got the rest.
Yep!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
