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

[Aditya Swarup <aditya.swarup@intel.com>]

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@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@intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Aditya Swarup <aditya.swarup@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.

Yes.. We will have to take care of it if we are adding rev id to array table(which mostly
should remain a const once we decide to go upstream). Without this declaration, I cannot
use ARRAY_SIZE() macro with revid arrays as the sizeof() operator complains about not
knowing the size of the array in question as it is an extern declaration. 

So, I don't know what other approach you want to suggest? If we move all the array tables to i915_drv.h(which
I feel would be a better approach rather than having it in intel_workarounds.c), Matt
Roper's KBL patch says that compiler complains about unused variables.

We are anyhow going to correct the whole thing with your stepping series anyway. This is supposed
to be a stop gap fix. Revids shouldn't be changing for TGL anymore.

> 
> 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.

So, what you are suggesting is add an info print in that function intel_detect_preproduction_hw() right?
Or something else?

> -Chris
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx