| Message ID | 20210303010728.3605269-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915/gen12: Add recommended hardware tuning value | expand |
On Tue, Mar 02, 2021 at 05:07:25PM -0800, Lucas De Marchi wrote: > From: Caz Yokoyama <caz.yokoyama@intel.com> > > Follow Bspec 31870 to set recommended tuning values for certain GT > register. These values aren't workarounds per-se, but it's best to > handle them in the same general area of the driver, especially since > there may be real workarounds that update other bits of the same > registers. > > At the moment the only value we need to worry about is the > TDS_TIMER setting in FF_MODE2. This setting was previously > described as "Wa_1604555607" on some platforms, but the spec > tells us that we should continue to program this on all current > gen12 platforms, even those that do not have that WA. > > Bspec: 31870 > > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> A couple minor nitpicks about the comments, but you can tweak those while applying. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> There appear to be some other registers recommended on the same bspec tuning page that we don't seem to be handling yet. Will those be coming as additional patches? > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 48 ++++++++++++++++----- > 1 file changed, 37 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 3b4a7da60f0b..f6d9b849aa62 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -646,9 +646,38 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, > wa_masked_en(wal, GEN9_ROW_CHICKEN4, GEN11_DIS_PICK_2ND_EU); > } > > +/* > + * These settings aren't actually workarounds, but general tuning settings that > + * need to be programmed on several platforms. > + */ > +static void gen12_ctx_gt_tuning_init(struct intel_engine_cs *engine, > + struct i915_wa_list *wal) > +{ > + /* > + * Although some platforms refer to it as Wa_1604555607, we need to > + * program it even on those that don't explicitly list that > + * workaround. > + * > + * Note that the implementation of this workaround is further modified Since we just got done saying that this technically isn't a workaround, even though some of the older platforms still list it that way, I'd re-word the comment here. Maybe "Note that the programming of this register is further modified..." would be more appropriate. > + * according to the FF_MODE2 guidance given by Wa_1608008084:gen12. > + * Minor nitpick; the new paragraph start here makes it sound like we've moved on to describing something else, rather than explaining what Wa_1608008084:gen12 asks us to do. I'd remove the blank line here and start the next sentence with "Wa_1608008084 tells us..." to link the statements together. Matt > + * FF_MODE2 register will return the wrong value when read. The default > + * value for this register is zero for all fields and there are no bit > + * masks. So instead of doing a RMW we should just write TDS timer > + * value. For the same reason read verification is ignored. > + */ > + wa_add(wal, > + FF_MODE2, > + FF_MODE2_TDS_TIMER_MASK, > + FF_MODE2_TDS_TIMER_128, > + 0); > +} > + > static void gen12_ctx_workarounds_init(struct intel_engine_cs *engine, > struct i915_wa_list *wal) > { > + gen12_ctx_gt_tuning_init(engine, wal); > + > /* > * Wa_1409142259:tgl > * Wa_1409347922:tgl > @@ -675,19 +704,15 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine, > gen12_ctx_workarounds_init(engine, wal); > > /* > - * Wa_1604555607:tgl,rkl > + * Wa_16011163337 > * > - * Note that the implementation of this workaround is further modified > - * according to the FF_MODE2 guidance given by Wa_1608008084:gen12. > - * FF_MODE2 register will return the wrong value when read. The default > - * value for this register is zero for all fields and there are no bit > - * masks. So instead of doing a RMW we should just write the GS Timer > - * and TDS timer values for Wa_1604555607 and Wa_16011163337. > + * Like in gen12_ctx_gt_tuning_init(), read verification is ignored due > + * to Wa_1608008084. > */ > wa_add(wal, > FF_MODE2, > - FF_MODE2_GS_TIMER_MASK | FF_MODE2_TDS_TIMER_MASK, > - FF_MODE2_GS_TIMER_224 | FF_MODE2_TDS_TIMER_128, > + FF_MODE2_GS_TIMER_MASK, > + FF_MODE2_GS_TIMER_224, > 0); > } > > @@ -707,12 +732,13 @@ static void dg1_ctx_workarounds_init(struct intel_engine_cs *engine, > /* > * Wa_16011163337 > * > - * Like in tgl_ctx_workarounds_init(), read verification is ignored due > + * Like in gen12_ctx_gt_tuning_init(), read verification is ignored due > * to Wa_1608008084. > */ > wa_add(wal, > FF_MODE2, > - FF_MODE2_GS_TIMER_MASK, FF_MODE2_GS_TIMER_224, 0); > + FF_MODE2_GS_TIMER_MASK, > + FF_MODE2_GS_TIMER_224, 0); > } > > static void > -- > 2.30.1 >
On Tue, Mar 02, 2021 at 07:14:07PM -0800, Matt Roper wrote: >On Tue, Mar 02, 2021 at 05:07:25PM -0800, Lucas De Marchi wrote: >> From: Caz Yokoyama <caz.yokoyama@intel.com> >> >> Follow Bspec 31870 to set recommended tuning values for certain GT >> register. These values aren't workarounds per-se, but it's best to >> handle them in the same general area of the driver, especially since >> there may be real workarounds that update other bits of the same >> registers. >> >> At the moment the only value we need to worry about is the >> TDS_TIMER setting in FF_MODE2. This setting was previously >> described as "Wa_1604555607" on some platforms, but the spec >> tells us that we should continue to program this on all current >> gen12 platforms, even those that do not have that WA. >> >> Bspec: 31870 >> >> Cc: Clinton Taylor <clinton.a.taylor@intel.com> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> Signed-off-by: Caz Yokoyama <caz.yokoyama@intel.com> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > >A couple minor nitpicks about the comments, but you can tweak those >while applying. > >Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >There appear to be some other registers recommended on the same bspec >tuning page that we don't seem to be handling yet. Will those be coming >as additional patches? I don't have those ready, but that is the intention, yes. Here I basically wanted to get the basic changes in place to make those possible, while moving the one we already have in the driver there. > >> --- >> drivers/gpu/drm/i915/gt/intel_workarounds.c | 48 ++++++++++++++++----- >> 1 file changed, 37 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c >> index 3b4a7da60f0b..f6d9b849aa62 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c >> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c >> @@ -646,9 +646,38 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, >> wa_masked_en(wal, GEN9_ROW_CHICKEN4, GEN11_DIS_PICK_2ND_EU); >> } >> >> +/* >> + * These settings aren't actually workarounds, but general tuning settings that >> + * need to be programmed on several platforms. >> + */ >> +static void gen12_ctx_gt_tuning_init(struct intel_engine_cs *engine, >> + struct i915_wa_list *wal) >> +{ >> + /* >> + * Although some platforms refer to it as Wa_1604555607, we need to >> + * program it even on those that don't explicitly list that >> + * workaround. >> + * >> + * Note that the implementation of this workaround is further modified > >Since we just got done saying that this technically isn't a workaround, >even though some of the older platforms still list it that way, I'd >re-word the comment here. Maybe "Note that the programming of this >register is further modified..." would be more appropriate. > >> + * according to the FF_MODE2 guidance given by Wa_1608008084:gen12. >> + * > >Minor nitpick; the new paragraph start here makes it sound like we've >moved on to describing something else, rather than explaining what >Wa_1608008084:gen12 asks us to do. I'd remove the blank line here and >start the next sentence with "Wa_1608008084 tells us..." to link the >statements together. thanks. I changed those and will resubmit. Lucas De Marchi > > >Matt > >> + * FF_MODE2 register will return the wrong value when read. The default >> + * value for this register is zero for all fields and there are no bit >> + * masks. So instead of doing a RMW we should just write TDS timer >> + * value. For the same reason read verification is ignored. >> + */ >> + wa_add(wal, >> + FF_MODE2, >> + FF_MODE2_TDS_TIMER_MASK, >> + FF_MODE2_TDS_TIMER_128, >> + 0); >> +} >> + >> static void gen12_ctx_workarounds_init(struct intel_engine_cs *engine, >> struct i915_wa_list *wal) >> { >> + gen12_ctx_gt_tuning_init(engine, wal); >> + >> /* >> * Wa_1409142259:tgl >> * Wa_1409347922:tgl >> @@ -675,19 +704,15 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine, >> gen12_ctx_workarounds_init(engine, wal); >> >> /* >> - * Wa_1604555607:tgl,rkl >> + * Wa_16011163337 >> * >> - * Note that the implementation of this workaround is further modified >> - * according to the FF_MODE2 guidance given by Wa_1608008084:gen12. >> - * FF_MODE2 register will return the wrong value when read. The default >> - * value for this register is zero for all fields and there are no bit >> - * masks. So instead of doing a RMW we should just write the GS Timer >> - * and TDS timer values for Wa_1604555607 and Wa_16011163337. >> + * Like in gen12_ctx_gt_tuning_init(), read verification is ignored due >> + * to Wa_1608008084. >> */ >> wa_add(wal, >> FF_MODE2, >> - FF_MODE2_GS_TIMER_MASK | FF_MODE2_TDS_TIMER_MASK, >> - FF_MODE2_GS_TIMER_224 | FF_MODE2_TDS_TIMER_128, >> + FF_MODE2_GS_TIMER_MASK, >> + FF_MODE2_GS_TIMER_224, >> 0); >> } >> >> @@ -707,12 +732,13 @@ static void dg1_ctx_workarounds_init(struct intel_engine_cs *engine, >> /* >> * Wa_16011163337 >> * >> - * Like in tgl_ctx_workarounds_init(), read verification is ignored due >> + * Like in gen12_ctx_gt_tuning_init(), read verification is ignored due >> * to Wa_1608008084. >> */ >> wa_add(wal, >> FF_MODE2, >> - FF_MODE2_GS_TIMER_MASK, FF_MODE2_GS_TIMER_224, 0); >> + FF_MODE2_GS_TIMER_MASK, >> + FF_MODE2_GS_TIMER_224, 0); >> } >> >> static void >> -- >> 2.30.1 >> > >-- >Matt Roper >Graphics Software Engineer >VTT-OSGC Platform Enablement >Intel Corporation >(916) 356-2795
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c index 3b4a7da60f0b..f6d9b849aa62 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c @@ -646,9 +646,38 @@ static void icl_ctx_workarounds_init(struct intel_engine_cs *engine, wa_masked_en(wal, GEN9_ROW_CHICKEN4, GEN11_DIS_PICK_2ND_EU); } +/* + * These settings aren't actually workarounds, but general tuning settings that + * need to be programmed on several platforms. + */ +static void gen12_ctx_gt_tuning_init(struct intel_engine_cs *engine, + struct i915_wa_list *wal) +{ + /* + * Although some platforms refer to it as Wa_1604555607, we need to + * program it even on those that don't explicitly list that + * workaround. + * + * Note that the implementation of this workaround is further modified + * according to the FF_MODE2 guidance given by Wa_1608008084:gen12. + * + * FF_MODE2 register will return the wrong value when read. The default + * value for this register is zero for all fields and there are no bit + * masks. So instead of doing a RMW we should just write TDS timer + * value. For the same reason read verification is ignored. + */ + wa_add(wal, + FF_MODE2, + FF_MODE2_TDS_TIMER_MASK, + FF_MODE2_TDS_TIMER_128, + 0); +} + static void gen12_ctx_workarounds_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) { + gen12_ctx_gt_tuning_init(engine, wal); + /* * Wa_1409142259:tgl * Wa_1409347922:tgl @@ -675,19 +704,15 @@ static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine, gen12_ctx_workarounds_init(engine, wal); /* - * Wa_1604555607:tgl,rkl + * Wa_16011163337 * - * Note that the implementation of this workaround is further modified - * according to the FF_MODE2 guidance given by Wa_1608008084:gen12. - * FF_MODE2 register will return the wrong value when read. The default - * value for this register is zero for all fields and there are no bit - * masks. So instead of doing a RMW we should just write the GS Timer - * and TDS timer values for Wa_1604555607 and Wa_16011163337. + * Like in gen12_ctx_gt_tuning_init(), read verification is ignored due + * to Wa_1608008084. */ wa_add(wal, FF_MODE2, - FF_MODE2_GS_TIMER_MASK | FF_MODE2_TDS_TIMER_MASK, - FF_MODE2_GS_TIMER_224 | FF_MODE2_TDS_TIMER_128, + FF_MODE2_GS_TIMER_MASK, + FF_MODE2_GS_TIMER_224, 0); } @@ -707,12 +732,13 @@ static void dg1_ctx_workarounds_init(struct intel_engine_cs *engine, /* * Wa_16011163337 * - * Like in tgl_ctx_workarounds_init(), read verification is ignored due + * Like in gen12_ctx_gt_tuning_init(), read verification is ignored due * to Wa_1608008084. */ wa_add(wal, FF_MODE2, - FF_MODE2_GS_TIMER_MASK, FF_MODE2_GS_TIMER_224, 0); + FF_MODE2_GS_TIMER_MASK, + FF_MODE2_GS_TIMER_224, 0); } static void