[Intel-gfx] [PATCH] drm/i915/tgl: Fix REVID macros for TGL to fetch correct stepping

[Chris Wilson]

Quoting Jani Nikula (2020-11-25 11:45:56)
> On Tue, 24 Nov 2020, Aditya Swarup <aditya.swarup at intel.com> wrote:
> > Fix TGL REVID macros to fetch correct display/gt stepping based
> > on SOC rev id from INTEL_REVID() macro. Previously, we were just
> > returning the first element of the revid array instead of using
> > the correct index based on SOC rev id.
> >
> > Also, add array bound checks for TGL REV ID array. Since, there
> > might be a possibility of using older kernels on latest platform
> > revision, resulting in out of bounds access for rev ID array.
> > In this scenario, print message for unsupported rev ID and apply
> > settings for latest rev ID available.
> >
> > Fixes: ("drm/i915/tgl: Fix stepping WA matching")
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Aditya Swarup <aditya.swarup at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 35 +++++++++++++++++++++++++++------
> >  1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 15be8debae54..29d55b7017be 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1572,16 +1572,37 @@ enum {
> >       TGL_REVID_D0,
> >  };
> >  
> > -extern const struct i915_rev_steppings tgl_uy_revids[];
> > -extern const struct i915_rev_steppings tgl_revids[];
> > +extern const struct i915_rev_steppings tgl_uy_revids[4];
> > +extern const struct i915_rev_steppings tgl_revids[2];
> 
> Just a quick note, the compiler does not check that the size in the
> extern declaration matches the size in the array definition. So you
> might end up with a mismatch without noticing.

What surprised me is that this defeated the __must_be_array() check.
I thought these were just pointers to C

> > +#define TGL_UY_REVID_RANGE(revid) \
> > +     ((revid) < ARRAY_SIZE(tgl_uy_revids))
> > +
> > +#define TGL_REVID_RANGE(revid) \
> > +     ((revid) < ARRAY_SIZE(tgl_revids))
> >  
> >  static inline const struct i915_rev_steppings *
> >  tgl_revids_get(struct drm_i915_private *dev_priv)
> >  {
> > -     if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv))
> > -             return tgl_uy_revids;
> > -     else
> > -             return tgl_revids;
> > +     const u8 revid = INTEL_REVID(dev_priv);
> > +
> > +     if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv)) {
> > +             if (TGL_UY_REVID_RANGE(revid)) {
> > +                     return tgl_uy_revids + revid;
> > +             } else {
> > +                     drm_dbg_kms(&dev_priv->drm,
> > +                                 "Unsupported SOC stepping found %u, using %lu instead\n",
> > +                                 revid, ARRAY_SIZE(tgl_uy_revids) - 1);

Also please don't have a dbg for every single IS_TGL_*_REVID
invocation. And this is not _kms, but driver; better yet, don't bother
with a drm_dbg_kms here at all.

If you want to actually check, add something like
intel_detect_preproduction_hw() and warn about unknown future revids.
Or include the info when we print the revid in the caps.
-Chris