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.widaw...@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.widaw...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
