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 5:21 AM, Souza, Jose wrote:
> On Tue, 2020-11-24 at 16:31 -0800, Aditya Swarup 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];
> 
> Not sure if the above will work, saw a comment from Jani please check that.

This works otherwise I can't use ARRAY_SIZE() macro as it is just an extern declaration,
so the sizeof() doesn't have clue about the size. The only way I can think of working 
around this is by moving tables here but Matt's KBL REVID patch suggests unused variables errors
but my compiler didn't complain.

> 
>> +
>> +#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;
> 
> Why not help readers and go simple? tgl_uy_revids[revid]

Hmm I will have to change the return type then, as you were returning a pointer and introduces
compiler error. I will change the return type.

> 
>> +		} else {
>> +			drm_dbg_kms(&dev_priv->drm,
>> +				    "Unsupported SOC stepping found %u, using %lu instead\n",
>> +				    revid, ARRAY_SIZE(tgl_uy_revids) - 1);
>> +			return tgl_uy_revids + (ARRAY_SIZE(tgl_uy_revids) - 1);
>> +		}
>> +	} else if (TGL_REVID_RANGE(revid)) {
>> +		return tgl_revids + revid;
>> +	} else	{
>> +		drm_dbg_kms(&dev_priv->drm,
>> +			    "Unsupported SOC stepping found %u, using %lu instead\n",
>> +			    revid, ARRAY_SIZE(tgl_revids) - 1);
>> +		return tgl_uy_revids + (ARRAY_SIZE(tgl_revids) - 1);
>> +	}
> 
> I bet you can re arrange it and end up with one drm_dbg_kms() call.

I can but that will involve more macros as we are dealing with two different array tables and each one
with a different range. I will use just one print to say what SOC rev id we get from pci dev and what
we will be using. 

> 
> 
>>  }
>>  
>>
>>
>>
>>
>>
>>
>>
>>  #define IS_TGL_DISP_REVID(p, since, until) \
>> @@ -1591,12 +1612,14 @@ tgl_revids_get(struct drm_i915_private *dev_priv)
>>  
>>
>>
>>
>>
>>
>>
>>
>>  #define IS_TGL_UY_GT_REVID(p, since, until) \
>>  	((IS_TGL_U(p) || IS_TGL_Y(p)) && \
>> +	 TGL_UY_REVID_RANGE(INTEL_REVID(p)) && \
>>  	 tgl_uy_revids->gt_stepping >= (since) && \
>>  	 tgl_uy_revids->gt_stepping <= (until))
>>  
>>
>>
>>
>>
>>
>>
>>
>>  #define IS_TGL_GT_REVID(p, since, until) \
>>  	(IS_TIGERLAKE(p) && \
>>  	 !(IS_TGL_U(p) || IS_TGL_Y(p)) && \
>> +	 TGL_REVID_RANGE(INTEL_REVID(p)) && \
>>  	 tgl_revids->gt_stepping >= (since) && \
>>  	 tgl_revids->gt_stepping <= (until))
>>  
>>
>>
>>
>>
>>
>>
> 
> You did not fixed the issue for GT.

Yes.. I didn't notice that.. Will change in the next revision.

Aditya

> 
>>
>>
>>
>>
>>
> 

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