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

Mon Mar 8 20:16:08 UTC 2021

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 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?

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