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

[Lucas De Marchi]

On Wed, Nov 25, 2020 at 03:33:23PM +0000, Chris Wilson wrote:
>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

it doesn't complain because it actually works. The extern is declaring
that amount of storage size... I think people here are confusing with
accepting an array as a parameter, in which case it decays to a
pointer.

Since this is all obscure semantics of C, for the quick fix here maybe
better to just define the size and reuse it both in header and .c?

and then work in the refactor that will actually remove all of this.

Lucas De Marchi

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