[Mesa-dev] [PATCH 1/5] i965: Query and store GPU properties from kernel

[Ben Widawsky]

On Mon, Mar 07, 2016 at 10:16:41PM -0800, Matt Turner wrote:
> On Mon, Mar 7, 2016 at 5:39 PM, Ben Widawsky
> <benjamin.widawsky at intel.com> wrote:
> > Certain products are not uniquely identifiable based on device id alone. The
> > kernel exports an interface to help deal with this. This patch merely introduces
> > the consumer of the interface and makes sure nothing breaks.
> >
> > It is also possible to use these values for programming GPGPU mode, and I plan
> > to do that as well.
> >
> > The interface was introduced in libdrm 2.4.60, which is already required, so it
> > should all be fine.
> >
> > Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
> > ---
> >  src/mesa/drivers/dri/i965/intel_screen.c | 21 +++++++++++++++++++++
> >  src/mesa/drivers/dri/i965/intel_screen.h | 12 +++++++++++-
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> > index ee7c1d7..343b497 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -1082,6 +1082,7 @@ static bool
> >  intel_init_bufmgr(struct intel_screen *intelScreen)
> >  {
> >     __DRIscreen *spriv = intelScreen->driScrnPriv;
> > +   bool devid_override = getenv("INTEL_DEVID_OVERRIDE") != NULL;
> >
> >     intelScreen->no_hw = getenv("INTEL_NO_HW") != NULL;
> >
> > @@ -1099,6 +1100,26 @@ intel_init_bufmgr(struct intel_screen *intelScreen)
> >        return false;
> >     }
> >
> > +   intelScreen->subslice_total = -1;
> > +   intelScreen->eu_total = -1;
> > +
> > +   /* Everything below this is for real hardware only */
> > +   if (intelScreen->no_hw || devid_override)
> > +      return true;
> > +
> > +   intel_get_param(spriv, I915_PARAM_SUBSLICE_TOTAL,
> > +                   &intelScreen->subslice_total);
> > +   intel_get_param(spriv, I915_PARAM_EU_TOTAL, &intelScreen->eu_total);
> > +
> > +   /* Without this information, we cannot get the right Braswell brandstrings,
> > +    * and we have to use conservative numbers for GPGPU on many platforms, but
> > +    * otherwise, things will just work.
> > +    */
> > +   if (intelScreen->subslice_total == -1 ||
> > +       intelScreen->eu_total == -1)
> 
> I think this condition will fit on one line.
> 
> > +      _mesa_warning(NULL,
> > +                    "Kernel 4.1 required to properly query GPU properties.\n");
> > +
> >     return true;
> >  }
> >
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.h b/src/mesa/drivers/dri/i965/intel_screen.h
> > index 3a5f22c..695ed50 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.h
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.h
> > @@ -81,7 +81,17 @@ struct intel_screen
> >      * I915_PARAM_CMD_PARSER_VERSION parameter
> >      */
> >     int cmd_parser_version;
> > - };
> > +
> > +   /**
> > +    * Best effort attempt to get system information. Needed for GPGPU, and brand
> > +    * strings (sigh)
> 
> The comment doesn't really describe the fields. Maybe
> 
> /**
>  * Number of subslices reported by the I915_PARAM_SUBSLICE_TOTAL parameter
>  */
> int subslice_total;
> 
> /**
>  * Number of EUs reported by the I915_PARAM_EU_TOTAL parameter
>  */
> int eu_total;
> 
> (Might have to linewrap the comments, not sure)
> 
> > +    * I915_PARAM_SUBSLICE_TOTAL, and I915_PARAM_EU_TOTAL
> > +    */
> > +   struct {
> 
> Do these need to be together in a struct?
> 

I like the idea of using the anonymous struct to logically group them - though
looking back now, I think it'd be cool to put all the params we get from the
kernel in a struct (easy debug to just dump struct contents in gdb). Anyway, I
don't feel strongly.

> > +      int subslice_total;
> > +      int eu_total;
> > +   };
> > +};

I took all of your suggestions. Thanks.

-- 
Ben Widawsky, Intel Open Source Technology Center