| Message ID | 20201117185029.22078-3-aditya.swarup@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce Alderlake-S | expand |
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. > > Also, remove redundant macros and simplify it to use GT and DISP > macros for getting applicable stepping for TGL. > > 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> > --- > .../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); > > > > > > > >
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. > >> >> 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@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> >> --- >> .../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@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >@@ -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); oohh, no. You have to at least check you are not accessing out of bounds. New HW running on old kernel should not access create invalid accesses like this. Lucas De Marchi
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)] 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@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> > > > --- > > > .../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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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> >> > > --- >> > > .../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@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >>@@ -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); > > oohh, no. You have to at least check you are not accessing out of > bounds. New HW running on old kernel should not access create invalid > accesses like this. And this is just one reason why exposing arrays directly as an interface to the rest of the driver is a bad idea. Basically I look at *all* externs in the driver with suspicion, and they're all exceptions that should not be repeated. The revid arrays are a direct invitation to keep adding more and more extern arrays. And more ways to go out of bounds. I'd rather we seek for ways to either nuke the revid arrays altogether, or encapsulate them within a .c file with static scope. And for that .c file... the arrays are now in gt/intel_workarounds.c which is a really weird place for stuff that's used for generic stepping info, and particularly for *display* stepping info. BR, Jani.
On 11/18/20 1:18 AM, Jani Nikula wrote: > On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >>> @@ -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); >> >> oohh, no. You have to at least check you are not accessing out of >> bounds. New HW running on old kernel should not access create invalid >> accesses like this. > > And this is just one reason why exposing arrays directly as an interface > to the rest of the driver is a bad idea. Basically I look at *all* > externs in the driver with suspicion, and they're all exceptions that > should not be repeated. The revid arrays are a direct invitation to keep > adding more and more extern arrays. And more ways to go out of bounds. We definitely need an array table for the SOC -> Display, GT stepping mapping. SOC steppings were usually the same as display steppings/GT steppings until TGL and therefore didn't require special mapping cases. But from TGL onwards, we have different combinations of Disp and GT steppings per SOC stepping. Alderlake-S makes this direct mapping even more difficult without the array requiring more macros to deal with SOC -> DISP/GT stepping differences. Will fix the array bound checks but the possibility of SOC revision id from drm struct going out of bounds is minimal. Can only happen if we don't have support for latest SOC -> Disp/GT table for TGL from Bspec and if we are picking up wrong revision id from drm struct that means the platform information obtained itself is wrong which will be a general platform problem unrelated to Gfx driver. > > I'd rather we seek for ways to either nuke the revid arrays altogether, > or encapsulate them within a .c file with static scope. I don't think we should nuke the revid arrays but I agree with finding a more appropriate place to parse the gt/display stepping info. This should be an exercise for a later patch that takes care of kbl,tgl and adl-s mappings. > > And for that .c file... the arrays are now in gt/intel_workarounds.c > which is a really weird place for stuff that's used for generic stepping > info, and particularly for *display* stepping info. I agree and we can change the approach with a different patch later. > > BR, > Jani. > >
On Mon, Nov 23, 2020 at 05:32:22PM -0800, Aditya Swarup wrote: >On 11/18/20 1:18 AM, Jani Nikula wrote: >> On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >>>> @@ -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); >>> >>> oohh, no. You have to at least check you are not accessing out of >>> bounds. New HW running on old kernel should not access create invalid >>> accesses like this. >> >> And this is just one reason why exposing arrays directly as an interface >> to the rest of the driver is a bad idea. Basically I look at *all* >> externs in the driver with suspicion, and they're all exceptions that >> should not be repeated. The revid arrays are a direct invitation to keep >> adding more and more extern arrays. And more ways to go out of bounds. > >We definitely need an array table for the SOC -> Display, GT stepping mapping. the mapping could be very well in the define iff you don't have different mappings per sku as is the case with TGL. Example: #define ADLS_REVID_A0 0 #define ADLS_REVID_A1 5 #define ADLS_DISP_REVID_A0 0 #define ADLS_DISP_REVID_B0 5 The actual value is actually the *SoC* revid, regardless the name of the macro. Since we already have to use a different macro - IS_DISP_REVID() - I don't think this is much worse and would allow us to get rid of the table *for ADL-S*, at the expense of having to pass as argument the ADLS_DISP_REVID_*. However this doesn't apply to TGL as TGL has a different mapping per sku. >SOC steppings were usually the same as display steppings/GT steppings until TGL and therefore >didn't require special mapping cases. But from TGL onwards, we have different combinations of >Disp and GT steppings per SOC stepping. Alderlake-S makes this direct mapping even more difficult >without the array requiring more macros to deal with SOC -> DISP/GT stepping differences. > >Will fix the array bound checks but the possibility of SOC revision id from drm struct going >out of bounds is minimal. Can only happen if we don't have support for latest SOC -> Disp/GT table this is very common. It's just a matter of trying to run a slightly old kernel in a slightly newer rev of the hardware. >for TGL from Bspec and if we are picking up wrong revision id from drm struct that means the platform >information obtained itself is wrong which will be a general platform problem unrelated to Gfx driver. Nothing else should really be a problem. We don't really use the revid much, mostly for WAs. And if other parts of the kernel are trying to use the SoC revid, then they are reading that info themselves, not using something we read. We are simply reading the revid from hardware and using that value without checking and that needs to change. > >> >> I'd rather we seek for ways to either nuke the revid arrays altogether, >> or encapsulate them within a .c file with static scope. > >I don't think we should nuke the revid arrays but I agree with finding a more appropriate place to >parse the gt/display stepping info. This should be an exercise for a later patch that takes >care of kbl,tgl and adl-s mappings. > >> >> And for that .c file... the arrays are now in gt/intel_workarounds.c >> which is a really weird place for stuff that's used for generic stepping >> info, and particularly for *display* stepping info. > >I agree and we can change the approach with a different patch later. > >> >> BR, >> Jani. >> >> >
On Tue, 24 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Mon, Nov 23, 2020 at 05:32:22PM -0800, Aditya Swarup wrote: >>On 11/18/20 1:18 AM, Jani Nikula wrote: >>> On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>> On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >>>>> @@ -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); >>>> >>>> oohh, no. You have to at least check you are not accessing out of >>>> bounds. New HW running on old kernel should not access create invalid >>>> accesses like this. >>> >>> And this is just one reason why exposing arrays directly as an interface >>> to the rest of the driver is a bad idea. Basically I look at *all* >>> externs in the driver with suspicion, and they're all exceptions that >>> should not be repeated. The revid arrays are a direct invitation to keep >>> adding more and more extern arrays. And more ways to go out of bounds. >> >>We definitely need an array table for the SOC -> Display, GT stepping mapping. > > the mapping could be very well in the define iff you don't have > different mappings per sku as is the case with TGL. Example: > > #define ADLS_REVID_A0 0 > #define ADLS_REVID_A1 5 > > #define ADLS_DISP_REVID_A0 0 > #define ADLS_DISP_REVID_B0 5 > > The actual value is actually the *SoC* revid, regardless the name of the > macro. Since we already have to use a different macro - > IS_DISP_REVID() - I don't think this is much worse and would allow us to > get rid of the table *for ADL-S*, at the expense of having to pass as > argument the ADLS_DISP_REVID_*. However this doesn't apply to TGL as TGL > has a different mapping per sku. > > >>SOC steppings were usually the same as display steppings/GT steppings until TGL and therefore >>didn't require special mapping cases. But from TGL onwards, we have different combinations of >>Disp and GT steppings per SOC stepping. Alderlake-S makes this direct mapping even more difficult >>without the array requiring more macros to deal with SOC -> DISP/GT stepping differences. >> >>Will fix the array bound checks but the possibility of SOC revision id from drm struct going >>out of bounds is minimal. Can only happen if we don't have support for latest SOC -> Disp/GT table > > this is very common. It's just a matter of trying to run a slightly old > kernel in a slightly newer rev of the hardware. Indeed. All kernels released with the arrays are simply bust for any new hardware revisions. They'll need a minimal Cc: stable fix. Here's something I drafted [1] to fix the situation more generally. There are still some issues to overcome, though they exist already in the current code. This could be followed up with converting *all* platforms to the scheme, making it universal, regardless of whether the revids in the hardware are consecutive or not. BR, Jani. [1] https://cgit.freedesktop.org/~jani/drm/log/?h=revid-stepping-scheme > >>for TGL from Bspec and if we are picking up wrong revision id from drm struct that means the platform >>information obtained itself is wrong which will be a general platform problem unrelated to Gfx driver. > > Nothing else should really be a problem. We don't really use the revid > much, mostly for WAs. And if other parts of the kernel are trying to use > the SoC revid, then they are reading that info themselves, not using > something we read. > > We are simply reading the revid from hardware and using that value > without checking and that needs to change. > > >> >>> >>> I'd rather we seek for ways to either nuke the revid arrays altogether, >>> or encapsulate them within a .c file with static scope. >> >>I don't think we should nuke the revid arrays but I agree with finding a more appropriate place to >>parse the gt/display stepping info. This should be an exercise for a later patch that takes >>care of kbl,tgl and adl-s mappings. >> >>> >>> And for that .c file... the arrays are now in gt/intel_workarounds.c >>> which is a really weird place for stuff that's used for generic stepping >>> info, and particularly for *display* stepping info. >> >>I agree and we can change the approach with a different patch later. >> >>> >>> BR, >>> Jani. >>> >>> >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
+Matt Roper, see question in item (3) below On Tue, Nov 24, 2020 at 04:20:40PM +0200, Jani Nikula wrote: >On Tue, 24 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >> On Mon, Nov 23, 2020 at 05:32:22PM -0800, Aditya Swarup wrote: >>>On 11/18/20 1:18 AM, Jani Nikula wrote: >>>> On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>> On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >>>>>> @@ -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); >>>>> >>>>> oohh, no. You have to at least check you are not accessing out of >>>>> bounds. New HW running on old kernel should not access create invalid >>>>> accesses like this. >>>> >>>> And this is just one reason why exposing arrays directly as an interface >>>> to the rest of the driver is a bad idea. Basically I look at *all* >>>> externs in the driver with suspicion, and they're all exceptions that >>>> should not be repeated. The revid arrays are a direct invitation to keep >>>> adding more and more extern arrays. And more ways to go out of bounds. >>> >>>We definitely need an array table for the SOC -> Display, GT stepping mapping. >> >> the mapping could be very well in the define iff you don't have >> different mappings per sku as is the case with TGL. Example: >> >> #define ADLS_REVID_A0 0 >> #define ADLS_REVID_A1 5 >> >> #define ADLS_DISP_REVID_A0 0 >> #define ADLS_DISP_REVID_B0 5 >> >> The actual value is actually the *SoC* revid, regardless the name of the >> macro. Since we already have to use a different macro - >> IS_DISP_REVID() - I don't think this is much worse and would allow us to >> get rid of the table *for ADL-S*, at the expense of having to pass as >> argument the ADLS_DISP_REVID_*. However this doesn't apply to TGL as TGL >> has a different mapping per sku. >> >> >>>SOC steppings were usually the same as display steppings/GT steppings until TGL and therefore >>>didn't require special mapping cases. But from TGL onwards, we have different combinations of >>>Disp and GT steppings per SOC stepping. Alderlake-S makes this direct mapping even more difficult >>>without the array requiring more macros to deal with SOC -> DISP/GT stepping differences. >>> >>>Will fix the array bound checks but the possibility of SOC revision id from drm struct going >>>out of bounds is minimal. Can only happen if we don't have support for latest SOC -> Disp/GT table >> >> this is very common. It's just a matter of trying to run a slightly old >> kernel in a slightly newer rev of the hardware. > >Indeed. All kernels released with the arrays are simply bust for any new >hardware revisions. They'll need a minimal Cc: stable fix. > >Here's something I drafted [1] to fix the situation more >generally. There are still some issues to overcome, though they exist >already in the current code. > >This could be followed up with converting *all* platforms to the scheme, >making it universal, regardless of whether the revids in the hardware >are consecutive or not. > >BR, >Jani. > > >[1] https://cgit.freedesktop.org/~jani/drm/log/?h=revid-stepping-scheme That is looking good. Some feedback I can give before this series being sent for review: 1) You need to call the init function from somewhere 2) For the FIXMEs: + /* + * FIXME: We should be able to take into account new revids not + * recognized by this kernel version. + */ + /* + * FIXME: We should be able to handle gaps in revid arrays gracefully, + * and in a way that works sensibly for the range checks. This is true + * for the existing revid range checks; it's fine if a new id pops up in + * the middle. + * + * It's okay for the display stepping to be zero, though in an array all + * or none should be set to non-zero, not a mix. + */ Maybe consider that gt_stepping will never be 0 and in the case it is (or size > ARRAY_SIZE), just backtrack to use the first one we find with gt_stepping != 0? then we probably should add a warning that we are not actually using the correct one, but it's the best we can do. 3) REVID_BXT_B_LAST what is that? The only thing that comes to mind is for "matching all B steps". Matt Roper had a patch to change the way we interpret the WA ranges so the bounds are [lower, upper) rather than [lower, upper]. Matt, any problem you faced with that patch? It makes more sense because we know the stepping in which it's fixed, but we may have additional revids before that But I don't see any trace of REVID_BXT_B_LAST in the tree, so not sure what's this about. 4) Lastly, I'd still like the simple fix for TGL without all the noise and without the refactor. It's the simplest fix we can do for the 5.10 timeframe. Lucas De Marchi > > > > >> >>>for TGL from Bspec and if we are picking up wrong revision id from drm struct that means the platform >>>information obtained itself is wrong which will be a general platform problem unrelated to Gfx driver. >> >> Nothing else should really be a problem. We don't really use the revid >> much, mostly for WAs. And if other parts of the kernel are trying to use >> the SoC revid, then they are reading that info themselves, not using >> something we read. >> >> We are simply reading the revid from hardware and using that value >> without checking and that needs to change. >> >> >>> >>>> >>>> I'd rather we seek for ways to either nuke the revid arrays altogether, >>>> or encapsulate them within a .c file with static scope. >>> >>>I don't think we should nuke the revid arrays but I agree with finding a more appropriate place to >>>parse the gt/display stepping info. This should be an exercise for a later patch that takes >>>care of kbl,tgl and adl-s mappings. >>> >>>> >>>> And for that .c file... the arrays are now in gt/intel_workarounds.c >>>> which is a really weird place for stuff that's used for generic stepping >>>> info, and particularly for *display* stepping info. >>> >>>I agree and we can change the approach with a different patch later. >>> >>>> >>>> BR, >>>> Jani. >>>> >>>> >>> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Jani Nikula, Intel Open Source Graphics Center
On 11/24/20 12:11 PM, Lucas De Marchi wrote: > +Matt Roper, see question in item (3) below > > On Tue, Nov 24, 2020 at 04:20:40PM +0200, Jani Nikula wrote: >> On Tue, 24 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>> On Mon, Nov 23, 2020 at 05:32:22PM -0800, Aditya Swarup wrote: >>>> On 11/18/20 1:18 AM, Jani Nikula wrote: >>>>> On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>>> On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >>>>>>> @@ -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); >>>>>> >>>>>> oohh, no. You have to at least check you are not accessing out of >>>>>> bounds. New HW running on old kernel should not access create invalid >>>>>> accesses like this. >>>>> >>>>> And this is just one reason why exposing arrays directly as an interface >>>>> to the rest of the driver is a bad idea. Basically I look at *all* >>>>> externs in the driver with suspicion, and they're all exceptions that >>>>> should not be repeated. The revid arrays are a direct invitation to keep >>>>> adding more and more extern arrays. And more ways to go out of bounds. >>>> >>>> We definitely need an array table for the SOC -> Display, GT stepping mapping. >>> >>> the mapping could be very well in the define iff you don't have >>> different mappings per sku as is the case with TGL. Example: >>> >>> #define ADLS_REVID_A0 0 >>> #define ADLS_REVID_A1 5 >>> >>> #define ADLS_DISP_REVID_A0 0 >>> #define ADLS_DISP_REVID_B0 5 >>> >>> The actual value is actually the *SoC* revid, regardless the name of the >>> macro. Since we already have to use a different macro - >>> IS_DISP_REVID() - I don't think this is much worse and would allow us to >>> get rid of the table *for ADL-S*, at the expense of having to pass as >>> argument the ADLS_DISP_REVID_*. However this doesn't apply to TGL as TGL >>> has a different mapping per sku. >>> >>> >>>> SOC steppings were usually the same as display steppings/GT steppings until TGL and therefore >>>> didn't require special mapping cases. But from TGL onwards, we have different combinations of >>>> Disp and GT steppings per SOC stepping. Alderlake-S makes this direct mapping even more difficult >>>> without the array requiring more macros to deal with SOC -> DISP/GT stepping differences. >>>> >>>> Will fix the array bound checks but the possibility of SOC revision id from drm struct going >>>> out of bounds is minimal. Can only happen if we don't have support for latest SOC -> Disp/GT table >>> >>> this is very common. It's just a matter of trying to run a slightly old >>> kernel in a slightly newer rev of the hardware. >> >> Indeed. All kernels released with the arrays are simply bust for any new >> hardware revisions. They'll need a minimal Cc: stable fix. >> >> Here's something I drafted [1] to fix the situation more >> generally. There are still some issues to overcome, though they exist >> already in the current code. >> >> This could be followed up with converting *all* platforms to the scheme, >> making it universal, regardless of whether the revids in the hardware >> are consecutive or not. >> >> BR, >> Jani. >> >> >> [1] https://cgit.freedesktop.org/~jani/drm/log/?h=revid-stepping-scheme > > That is looking good. Some feedback I can give before this series being > sent for review: I like this approach as well and we were discussing this in the ADLS rev ID thread. With the tables it makes it simpler to manage rather than worrying about individual macros. Jani do you want me to rebase ADLS changes on top of your patches and resubmit your patches for review or you will be submitting this series yourself? > > 1) You need to call the init function from somewhere > 2) For the FIXMEs: > > + /* > + * FIXME: We should be able to take into account new revids not > + * recognized by this kernel version. > + */ > > + /* > + * FIXME: We should be able to handle gaps in revid arrays gracefully, > + * and in a way that works sensibly for the range checks. This is true > + * for the existing revid range checks; it's fine if a new id pops up in > + * the middle. > + * > + * It's okay for the display stepping to be zero, though in an array all > + * or none should be set to non-zero, not a mix. > + */ > > Maybe consider that gt_stepping will never be 0 and in the case it is (or > size > ARRAY_SIZE), just backtrack to use the first one we find with > gt_stepping != 0? then we probably should add a warning that we are not > actually using the correct one, but it's the best we can do. > > 3) REVID_BXT_B_LAST I couldn't spot REVID_NONE as well in the patches. > > what is that? The only thing that comes to mind is for "matching all B > steps". Matt Roper had a patch to change the way we interpret the WA > ranges so the bounds are [lower, upper) rather than [lower, upper]. > Matt, any problem you faced with that patch? It makes more sense > because we know the stepping in which it's fixed, but we may have > additional revids before that > > But I don't see any trace of REVID_BXT_B_LAST in the tree, so not sure > what's this about. > > 4) > > Lastly, I'd still like the simple fix for TGL without all the noise and > without the refactor. It's the simplest fix we can do for the 5.10 > timeframe. I just submitted the fix. Aditya > > > Lucas De Marchi > >> >> >> >> >>> >>>> for TGL from Bspec and if we are picking up wrong revision id from drm struct that means the platform >>>> information obtained itself is wrong which will be a general platform problem unrelated to Gfx driver. >>> >>> Nothing else should really be a problem. We don't really use the revid >>> much, mostly for WAs. And if other parts of the kernel are trying to use >>> the SoC revid, then they are reading that info themselves, not using >>> something we read. >>> >>> We are simply reading the revid from hardware and using that value >>> without checking and that needs to change. >>> >>> >>>> >>>>> >>>>> I'd rather we seek for ways to either nuke the revid arrays altogether, >>>>> or encapsulate them within a .c file with static scope. >>>> >>>> I don't think we should nuke the revid arrays but I agree with finding a more appropriate place to >>>> parse the gt/display stepping info. This should be an exercise for a later patch that takes >>>> care of kbl,tgl and adl-s mappings. >>>> >>>>> >>>>> And for that .c file... the arrays are now in gt/intel_workarounds.c >>>>> which is a really weird place for stuff that's used for generic stepping >>>>> info, and particularly for *display* stepping info. >>>> >>>> I agree and we can change the approach with a different patch later. >>>> >>>>> >>>>> BR, >>>>> Jani. >>>>> >>>>> >>>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Tue, 24 Nov 2020, Aditya Swarup <aditya.swarup@intel.com> wrote: > On 11/24/20 12:11 PM, Lucas De Marchi wrote: >> +Matt Roper, see question in item (3) below >> >> On Tue, Nov 24, 2020 at 04:20:40PM +0200, Jani Nikula wrote: >>> On Tue, 24 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>> On Mon, Nov 23, 2020 at 05:32:22PM -0800, Aditya Swarup wrote: >>>>> On 11/18/20 1:18 AM, Jani Nikula wrote: >>>>>> On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote: >>>>>>> On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote: >>>>>>>> @@ -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); >>>>>>> >>>>>>> oohh, no. You have to at least check you are not accessing out of >>>>>>> bounds. New HW running on old kernel should not access create invalid >>>>>>> accesses like this. >>>>>> >>>>>> And this is just one reason why exposing arrays directly as an interface >>>>>> to the rest of the driver is a bad idea. Basically I look at *all* >>>>>> externs in the driver with suspicion, and they're all exceptions that >>>>>> should not be repeated. The revid arrays are a direct invitation to keep >>>>>> adding more and more extern arrays. And more ways to go out of bounds. >>>>> >>>>> We definitely need an array table for the SOC -> Display, GT stepping mapping. >>>> >>>> the mapping could be very well in the define iff you don't have >>>> different mappings per sku as is the case with TGL. Example: >>>> >>>> #define ADLS_REVID_A0 0 >>>> #define ADLS_REVID_A1 5 >>>> >>>> #define ADLS_DISP_REVID_A0 0 >>>> #define ADLS_DISP_REVID_B0 5 >>>> >>>> The actual value is actually the *SoC* revid, regardless the name of the >>>> macro. Since we already have to use a different macro - >>>> IS_DISP_REVID() - I don't think this is much worse and would allow us to >>>> get rid of the table *for ADL-S*, at the expense of having to pass as >>>> argument the ADLS_DISP_REVID_*. However this doesn't apply to TGL as TGL >>>> has a different mapping per sku. >>>> >>>> >>>>> SOC steppings were usually the same as display steppings/GT steppings until TGL and therefore >>>>> didn't require special mapping cases. But from TGL onwards, we have different combinations of >>>>> Disp and GT steppings per SOC stepping. Alderlake-S makes this direct mapping even more difficult >>>>> without the array requiring more macros to deal with SOC -> DISP/GT stepping differences. >>>>> >>>>> Will fix the array bound checks but the possibility of SOC revision id from drm struct going >>>>> out of bounds is minimal. Can only happen if we don't have support for latest SOC -> Disp/GT table >>>> >>>> this is very common. It's just a matter of trying to run a slightly old >>>> kernel in a slightly newer rev of the hardware. >>> >>> Indeed. All kernels released with the arrays are simply bust for any new >>> hardware revisions. They'll need a minimal Cc: stable fix. >>> >>> Here's something I drafted [1] to fix the situation more >>> generally. There are still some issues to overcome, though they exist >>> already in the current code. >>> >>> This could be followed up with converting *all* platforms to the scheme, >>> making it universal, regardless of whether the revids in the hardware >>> are consecutive or not. >>> >>> BR, >>> Jani. >>> >>> >>> [1] https://cgit.freedesktop.org/~jani/drm/log/?h=revid-stepping-scheme >> >> That is looking good. Some feedback I can give before this series being >> sent for review: > > I like this approach as well and we were discussing this in the ADLS > rev ID thread. With the tables it makes it simpler to manage rather > than worrying about individual macros. Jani do you want me to rebase > ADLS changes on top of your patches and resubmit your patches for > review or you will be submitting this series yourself? Please proceed with ADL as you were, I'll do the refactoring afterwards on top. It'll take a while anyway, we don't want to delay ADL with this. Just add the required bounds checks to the ADL patches. >> >> 1) You need to call the init function from somewhere Yeah, that's a FIXME in a commit message. Draft patches and all that. ;) I'll need to figure out where and how early we need to set this up, as it needs to be set up before any users, obviously. >> 2) For the FIXMEs: >> >> + /* >> + * FIXME: We should be able to take into account new revids not >> + * recognized by this kernel version. >> + */ >> >> + /* >> + * FIXME: We should be able to handle gaps in revid arrays gracefully, >> + * and in a way that works sensibly for the range checks. This is true >> + * for the existing revid range checks; it's fine if a new id pops up in >> + * the middle. >> + * >> + * It's okay for the display stepping to be zero, though in an array all >> + * or none should be set to non-zero, not a mix. >> + */ >> >> Maybe consider that gt_stepping will never be 0 and in the case it is (or >> size > ARRAY_SIZE), just backtrack to use the first one we find with >> gt_stepping != 0? then we probably should add a warning that we are not >> actually using the correct one, but it's the best we can do. Yeah, it'll probably need to be something like that. I'll figure something out. >> >> 3) REVID_BXT_B_LAST > > I couldn't spot REVID_NONE as well in the patches. For now REVID_NONE was intended as a placeholder to ensure all valid symbolic revids are non-zero, for array initialization purposes. >> >> what is that? The only thing that comes to mind is for "matching all B >> steps". Matt Roper had a patch to change the way we interpret the WA >> ranges so the bounds are [lower, upper) rather than [lower, upper]. >> Matt, any problem you faced with that patch? It makes more sense >> because we know the stepping in which it's fixed, but we may have >> additional revids before that >> >> But I don't see any trace of REVID_BXT_B_LAST in the tree, so not sure >> what's this about. I just indiscriminately scooped up all revid macros from i915_drv.h for starters. We have BXT_REVID_B_LAST to identify the last pre-pro bxt before C0. Needs cleanup, agreed. >> >> 4) >> >> Lastly, I'd still like the simple fix for TGL without all the noise and >> without the refactor. It's the simplest fix we can do for the 5.10 >> timeframe. Agreed. > I just submitted the fix. Thanks. Let's get that done first, then the ADL enabling, then I'll do the refactoring afterwards. BR, Jani. > > Aditya > >> >> >> Lucas De Marchi >> >>> >>> >>> >>> >>>> >>>>> for TGL from Bspec and if we are picking up wrong revision id from drm struct that means the platform >>>>> information obtained itself is wrong which will be a general platform problem unrelated to Gfx driver. >>>> >>>> Nothing else should really be a problem. We don't really use the revid >>>> much, mostly for WAs. And if other parts of the kernel are trying to use >>>> the SoC revid, then they are reading that info themselves, not using >>>> something we read. >>>> >>>> We are simply reading the revid from hardware and using that value >>>> without checking and that needs to change. >>>> >>>> >>>>> >>>>>> >>>>>> I'd rather we seek for ways to either nuke the revid arrays altogether, >>>>>> or encapsulate them within a .c file with static scope. >>>>> >>>>> I don't think we should nuke the revid arrays but I agree with finding a more appropriate place to >>>>> parse the gt/display stepping info. This should be an exercise for a later patch that takes >>>>> care of kbl,tgl and adl-s mappings. >>>>> >>>>>> >>>>>> And for that .c file... the arrays are now in gt/intel_workarounds.c >>>>>> which is a really weird place for stuff that's used for generic stepping >>>>>> info, and particularly for *display* stepping info. >>>>> >>>>> I agree and we can change the approach with a different patch later. >>>>> >>>>>> >>>>>> BR, >>>>>> Jani. >>>>>> >>>>>> >>>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> >>> -- >>> Jani Nikula, Intel Open Source Graphics Center >
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, }; 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); } #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);
Fix macros for applying TGL SOC WAs by using INTEL_REVID() as index to fetch correct revision offset in TGL GT/DISP stepping table. Also, remove redundant macros and simplify it to use GT and DISP macros for getting applicable stepping for TGL. 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> --- .../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(-)