[Intel-gfx] [PATCH 1/4] drm/i915/gen12: Add recommended hardware tuning value

[Matt Roper]

On Tue, Mar 02, 2021 at 05:07:25PM -0800, Lucas De Marchi wrote:
> From: Caz Yokoyama <caz.yokoyama at 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 at intel.com>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Caz Yokoyama <caz.yokoyama at intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>

A couple minor nitpicks about the comments, but you can tweak those
while applying.

Reviewed-by: Matt Roper <matthew.d.roper at 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
> 

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795