[Intel-gfx] [PATCH 02/21] drm/i915/tgl: Fix macros for TGL SOC based WA

Lucas De Marchi lucas.demarchi at intel.com
Wed Nov 18 07:56:33 UTC 2020 

On Tue, Nov 17, 2020 at 07:33:11PM +0000, Jose Souza wrote:
>On Tue, 2020-11-17 at 11:28 -0800, Lucas De Marchi wrote:
>> On Tue, Nov 17, 2020 at 07:03:05PM +0000, Jose Souza wrote:
>> > On Tue, 2020-11-17 at 10:50 -0800, Aditya Swarup wrote:
>> > > Fix macros for applying TGL SOC WAs by using INTEL_REVID()
>> > > as index to fetch correct revision offset in TGL GT/DISP stepping
>> > > table.
>> >
>> > Please explain what exactly is the issue you are fixing, the change you did in tgl_revids_get() + IS_TGL_GT_REVID looks a improvement but not a fix.
>>
>> otherwise it always gets the first entry from the table, regardless
>> what' s the revid we are running on... so it does look like a very
>> important fix.
>
>Ooh okay, so the fix should only be
>tgl_uy_revids/tgl_revids->gt_stepping[INTEL_REVID(dev_priv)]

as noted in my review to the patch, after checking the array bounds

Lucas De marchi

>
>Then the improvements in other patches.
>
>>
>> >
>> > >
>> > > Also, remove redundant macros and simplify it to use GT and DISP
>> > > macros for getting applicable stepping for TGL.
>>
>> As a fix, this should not be mixed with the noisy s/TGL_REVID/REVID/, as
>> it makes it much more difficult for backports and to review. Please
>> split it in another patch (I actually don' t see a reason to do it
>> actually... I'd rather try to move away from these tables if possible).
>>
>> Lucas De Marchi
>>
>> > >
>> > > 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>
>> > > ---
>> > >  .../drm/i915/display/intel_display_power.c    |  2 +-
>> > >  drivers/gpu/drm/i915/display/intel_psr.c      |  4 ++--
>> > >  drivers/gpu/drm/i915/display/intel_sprite.c   |  2 +-
>> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c   | 20 ++++++++--------
>> > >  drivers/gpu/drm/i915/i915_drv.h               | 24 +++++++------------
>> > >  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
>> > >  6 files changed, 24 insertions(+), 30 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > > index fe2d90bba536..06c036e2092c 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > > @@ -5283,7 +5283,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv)
>> > >  	int config, i;
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	if (IS_DG1_REVID(dev_priv, DG1_REVID_A0, DG1_REVID_A0) ||
>> > > -	    IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0))
>> > > +	    IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_B0))
>> > >  		/* Wa_1409767108:tgl,dg1 */
>> > >  		table = wa_1409767108_buddy_page_masks;
>> > >  	else
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > index b3631b722de3..c057a03b2ed4 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > @@ -550,7 +550,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	if (dev_priv->psr.psr2_sel_fetch_enabled) {
>> > >  		/* WA 1408330847 */
>> > > -		if (IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0) ||
>> > > +		if (IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_A0) ||
>> > >  		    IS_RKL_REVID(dev_priv, RKL_REVID_A0, RKL_REVID_A0))
>> > >  			intel_de_rmw(dev_priv, CHICKEN_PAR1_1,
>> > >  				     DIS_RAM_BYPASS_PSR2_MAN_TRACK,
>> > > @@ -1102,7 +1102,7 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	/* WA 1408330847 */
>> > >  	if (dev_priv->psr.psr2_sel_fetch_enabled &&
>> > > -	    (IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0) ||
>> > > +	    (IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_A0) ||
>> > >  	     IS_RKL_REVID(dev_priv, RKL_REVID_A0, RKL_REVID_A0)))
>> > >  		intel_de_rmw(dev_priv, CHICKEN_PAR1_1,
>> > >  			     DIS_RAM_BYPASS_PSR2_MAN_TRACK, 0);
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
>> > > index a3ab44694118..f7da4a56054e 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>> > > @@ -3022,7 +3022,7 @@ static bool gen12_plane_supports_mc_ccs(struct drm_i915_private *dev_priv,
>> > >  {
>> > >  	/* Wa_14010477008:tgl[a0..c0],rkl[all],dg1[all] */
>> > >  	if (IS_DG1(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
>> > > -	    IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_C0))
>> > > +	    IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_C0))
>> > >  		return false;
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	return plane_id < PLANE_SPRITE4;
>> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > > index a82554baa6ac..d756155d82ea 100644
>> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> > > @@ -71,16 +71,16 @@ const struct i915_rev_steppings kbl_revids[] = {
>> > >  };
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  const struct i915_rev_steppings tgl_uy_revids[] = {
>> > > -	[0] = { .gt_stepping = TGL_REVID_A0, .disp_stepping = TGL_REVID_A0 },
>> > > -	[1] = { .gt_stepping = TGL_REVID_B0, .disp_stepping = TGL_REVID_C0 },
>> > > -	[2] = { .gt_stepping = TGL_REVID_B1, .disp_stepping = TGL_REVID_C0 },
>> > > -	[3] = { .gt_stepping = TGL_REVID_C0, .disp_stepping = TGL_REVID_D0 },
>> > > +	[0] = { .gt_stepping = REVID_A0, .disp_stepping = REVID_A0 },
>> > > +	[1] = { .gt_stepping = REVID_B0, .disp_stepping = REVID_C0 },
>> > > +	[2] = { .gt_stepping = REVID_B1, .disp_stepping = REVID_C0 },
>> > > +	[3] = { .gt_stepping = REVID_C0, .disp_stepping = REVID_D0 },
>> > >  };
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  /* Same GT stepping between tgl_uy_revids and tgl_revids don't mean the same HW */
>> > >  const struct i915_rev_steppings tgl_revids[] = {
>> > > -	[0] = { .gt_stepping = TGL_REVID_A0, .disp_stepping = TGL_REVID_B0 },
>> > > -	[1] = { .gt_stepping = TGL_REVID_B0, .disp_stepping = TGL_REVID_D0 },
>> > > +	[0] = { .gt_stepping = REVID_A0, .disp_stepping = REVID_B0 },
>> > > +	[1] = { .gt_stepping = REVID_B0, .disp_stepping = REVID_D0 },
>> > >  };
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  static void wa_init_start(struct i915_wa_list *wal, const char *name, const char *engine_name)
>> > > @@ -1250,13 +1250,13 @@ tgl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>> > >  	gen12_gt_workarounds_init(i915, wal);
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	/* Wa_1409420604:tgl */
>> > > -	if (IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
>> > > +	if (IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0))
>> > >  		wa_write_or(wal,
>> > >  			    SUBSLICE_UNIT_LEVEL_CLKGATE2,
>> > >  			    CPSSUNIT_CLKGATE_DIS);
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	/* Wa_1607087056:tgl also know as BUG:1409180338 */
>> > > -	if (IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
>> > > +	if (IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0))
>> > >  		wa_write_or(wal,
>> > >  			    SLICE_UNIT_LEVEL_CLKGATE,
>> > >  			    L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
>> > > @@ -1734,7 +1734,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>> > >  	struct drm_i915_private *i915 = engine->i915;
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	if (IS_DG1_REVID(i915, DG1_REVID_A0, DG1_REVID_A0) ||
>> > > -	    IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
>> > > +	    IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0)) {
>> > >  		/*
>> > >  		 * Wa_1607138336:tgl[a0],dg1[a0]
>> > >  		 * Wa_1607063988:tgl[a0],dg1[a0]
>> > > @@ -1744,7 +1744,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>> > >  			    GEN12_DISABLE_POSH_BUSY_FF_DOP_CG);
>> > >  	}
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > -	if (IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
>> > > +	if (IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0)) {
>> > >  		/*
>> > >  		 * Wa_1606679103:tgl
>> > >  		 * (see also Wa_1606682166:icl)
>> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> > > index 15be8debae54..437916aacaa6 100644
>> > > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > > @@ -1565,11 +1565,11 @@ extern const struct i915_rev_steppings kbl_revids[];
>> > >  	(IS_JSL_EHL(p) && IS_REVID(p, since, until))
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  enum {
>> > > -	TGL_REVID_A0,
>> > > -	TGL_REVID_B0,
>> > > -	TGL_REVID_B1,
>> > > -	TGL_REVID_C0,
>> > > -	TGL_REVID_D0,
>> > > +	REVID_A0,
>> > > +	REVID_B0,
>> > > +	REVID_B1,
>> > > +	REVID_C0,
>> > > +	REVID_D0,
>> >
>> > Better keep "TGL_" otherwise this could be used in other platforms that have different values for each revision.
>> >
>> > >  };
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  extern const struct i915_rev_steppings tgl_uy_revids[];
>> > > @@ -1579,9 +1579,9 @@ 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;
>> > > +		return tgl_uy_revids + INTEL_REVID(dev_priv);
>> > >  	else
>> > > -		return tgl_revids;
>> > > +		return tgl_revids + INTEL_REVID(dev_priv);
>> >
>> > better do tgl_revids[INTEL_REVID(dev_priv)] with a array size check first.
>> >
>> > >  }
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  #define IS_TGL_DISP_REVID(p, since, until) \
>> > > @@ -1589,16 +1589,10 @@ tgl_revids_get(struct drm_i915_private *dev_priv)
>> > >  	 tgl_revids_get(p)->disp_stepping >= (since) && \
>> > >  	 tgl_revids_get(p)->disp_stepping <= (until))
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > -#define IS_TGL_UY_GT_REVID(p, since, until) \
>> > > -	((IS_TGL_U(p) || IS_TGL_Y(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_revids->gt_stepping >= (since) && \
>> > > -	 tgl_revids->gt_stepping <= (until))
>> > > +	 tgl_revids_get(p)->gt_stepping >= (since) && \
>> > > +	 tgl_revids_get(p)->gt_stepping <= (until))
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  #define RKL_REVID_A0		0x0
>> > >  #define RKL_REVID_B0		0x1
>> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > > index a20b5051f18c..69840aa0d4db 100644
>> > > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > > @@ -7110,7 +7110,7 @@ static void tgl_init_clock_gating(struct drm_i915_private *dev_priv)
>> > >  		   ILK_DPFC_CHICKEN_COMP_DUMMY_PIXEL);
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >  	/* Wa_1409825376:tgl (pre-prod)*/
>> > > -	if (IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B1))
>> > > +	if (IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_B1))
>> > >  		I915_WRITE(GEN9_CLKGATE_DIS_3, I915_READ(GEN9_CLKGATE_DIS_3) |
>> > >  			   TGL_VRH_GATING_DIS);
>> > >  
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

More information about the Intel-gfx mailing list