On Sat, Jun 24, 2023 at 10:17:53AM -0700, Lucas De Marchi wrote:
> Right now context workarounds don't do a rmw and instead only write to
> the register. Since 2 separate programmings to the same register are
> coalesced into a single write, this is not problematic for
> GEN12_FF_MODE2 since both TDS and GS timer are going to be written
> together and the other remaining bits be zeroed.
>
> However in order to fix other workarounds that may want to preserve the
> unrelated bits in the same register, context workarounds need to
> be changed to a rmw. To prepare for that, move the programming of
> GEN12_FF_MODE2 to a single place so the value passed for "clear" can
> be all the bits. Otherwise the second workaround would be dropped as
> it'd be detected as overwriting a previously programmed workaround.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 51 +++++++--------------
> 1 file changed, 17 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 8f8346df3c18..7d48bd57b6ef 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -693,40 +693,11 @@ static void dg2_ctx_gt_tuning_init(struct intel_engine_cs *engine,
> 0, false);
> }
>
> -/*
> - * 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 programming of this register is further modified
> - * according to the FF_MODE2 guidance given by Wa_1608008084:gen12.
> - * Wa_1608008084 tells us the 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,
> - GEN12_FF_MODE2,
> - FF_MODE2_TDS_TIMER_MASK,
> - FF_MODE2_TDS_TIMER_128,
> - 0, false);
> -}
> -
> static void gen12_ctx_workarounds_init(struct intel_engine_cs *engine,
> struct i915_wa_list *wal)
> {
> struct drm_i915_private *i915 = engine->i915;
>
> - gen12_ctx_gt_tuning_init(engine, wal);
> -
> /*
> * Wa_1409142259:tgl,dg1,adl-p
> * Wa_1409347922:tgl,dg1,adl-p
> @@ -748,15 +719,27 @@ static void gen12_ctx_workarounds_init(struct intel_engine_cs *engine,
> GEN9_PREEMPT_GPGPU_THREAD_GROUP_LEVEL);
>
> /*
> - * Wa_16011163337
> + * Wa_16011163337 - GS_TIMER
> + *
> + * TDS_TIMER: 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 programming of GEN12_FF_MODE2 is further modified
> + * according to the FF_MODE2 guidance given by Wa_1608008084.
> + * Wa_1608008084 tells us the FF_MODE2 register will return the wrong
> + * value when read from the CPU.
> *
> - * Like in gen12_ctx_gt_tuning_init(), read verification is ignored due
> - * to Wa_1608008084.
> + * The default value for this register is zero for all fields.
> + * So instead of doing a RMW we should just write the desired values
> + * for TDS and GS timers. Note that since the readback can't be trusted,
> + * the clear mask is just set to ~0 to make sure other bits are not
> + * inadvertently set. For the same reason read verification is ignored.
> */
> wa_add(wal,
> GEN12_FF_MODE2,
> - FF_MODE2_GS_TIMER_MASK,
> - FF_MODE2_GS_TIMER_224,
> + ~0,
> + FF_MODE2_TDS_TIMER_128 | FF_MODE2_GS_TIMER_224,
> 0, false);
>
> if (!IS_DG1(i915)) {
> --
> 2.40.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation