Re: [Intel-gfx] [PATCH] drm/i915: Add Wa_18028616096

["Chauhan, Shekhar" <shekhar.chauhan@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3078 bytes --]

Quoting Gustavo Sousa:

On 9/22/2023 02:31, Gustavo Sousa wrote:
> Quoting Shekhar Chauhan (2023-09-21 11:30:28-03:00)
>> Drop UGM per set fragment threshold to 3
>>
>> BSpec: 54833
>> Signed-off-by: Shekhar Chauhan<shekhar.chauhan@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_gt_regs.h     | 1 +
>> drivers/gpu/drm/i915/gt/intel_workarounds.c | 3 +++
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> index a00ff51c681d..431c575c532b 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> @@ -1230,6 +1230,7 @@
>> #define   DISABLE_D8_D16_COASLESCE                REG_BIT(30)
>> #define   FORCE_1_SUB_MESSAGE_PER_FRAGMENT        REG_BIT(15)
>> #define LSC_CHICKEN_BIT_0_UDW                        MCR_REG(0xe7c8 + 4)
>> +#define   UGM_FRAGMENT_THRESHOLD_TO_3                REG_BIT(58 - 32)
>> #define   DIS_CHAIN_2XSIMD8                        REG_BIT(55 - 32)
>> #define   FORCE_SLM_FENCE_SCOPE_TO_TILE                REG_BIT(42 - 32)
>> #define   FORCE_UGM_FENCE_SCOPE_TO_TILE                REG_BIT(41 - 32)
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 660d4f358eab..992041e3776c 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -2914,6 +2914,9 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li
>>                   * Wa_22015475538:dg2
>>                   */
>>                  wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, DIS_CHAIN_2XSIMD8);
>> +
>> +                /* Wa_18028616096:dg2 */
> This is not a blocker, but I would prefer to remove the ":dg2" suffix.
>
> There was an effort to remove them from our driver[1], but it kinda of
> stalled. I myself agree that we would be better off without them.
>
> [1]https://lore.kernel.org/all/20221222082557.1364711-1-lucas.demarchi@intel.com
Ack'ed in the new version.
>
>> +                wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, UGM_FRAGMENT_THRESHOLD_TO_3);
> This workaround applies to (i) DG2 G10 from stepping C0 to forever and
> (ii) any stepping of DG2 G12. Here you are applying this workaround to
> any variant of DG2.
>
> It should be moved out of this "if" statement and rather be guarded by
> something like:
>
> 	if ((IS_DG2_G10(i915) && IS_GRAPHICS_STEP(i915, STEP_C0, STEP_FOREVER)) ||
> 	    IS_DG2_G12(i915))
>
> Note that we are there is still a pending decision for G11, so we may
> need to update this in the future.

I believe we're only supporting production steppings for DG2, 
henceforth, not really interacting with the "older" steppings.

Please have a look:

https://lore.kernel.org/intel-gfx/20230816214201.534095-7-matthew.d.roper@intel.com/

Although, I could be wrong, if I am, I'll send in another version, 
modifying the patch as you've suggested.

> --
> Gustavo Sousa
>
>>          }
>>
>>          if (IS_DG2_G11(i915)) {
>> -- 
>> 2.34.1
>>
-- 
-shekhar

[-- Attachment #2: Type: text/html, Size: 40158 bytes --]