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

[Chris Wilson]

Quoting Aditya Swarup (2020-11-25 17:51:04)
> On 11/25/20 7:33 AM, 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:
> >>>  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.
> 
> So, what you are suggesting is add an info print in that function intel_detect_preproduction_hw() right?
> Or something else?

I wouldn't put it in detect_preproduction, just using that as an example
of when we do probes for unexpected revids. E.g.,

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ca16ea541ecc..f1ff5509c23a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -273,6 +273,21 @@ static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv)
        }
 }

+/*
+ * HW that is more recent than the kernel runs the risk of us applying
+ * stale and disruptive w/a. Leave a debug tell-tale just in case.
+ */
+static void intel_detect_unknown_hw(struct drm_i915_private *dev_priv)
+{
+       bool post = false;
+
+       if (post) {
+               drm_dbg(&dev_priv->drm,
+                       "This machine is more recent than the w/a database!\n");
+               add_taint(TAINT_MACHINE_CHECK, LOCKDEP_STILL_OK);
+       }
+}
+
 static void sanitize_gpu(struct drm_i915_private *i915)
 {
        if (!INTEL_INFO(i915)->gpu_reset_clobbers_display)
@@ -343,6 +358,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
        intel_init_audio_hooks(dev_priv);

        intel_detect_preproduction_hw(dev_priv);
+       intel_detect_unknown_hw(dev_priv);

        return 0;


The taint is probably not justified in this case.
-Chris