Re: [Intel-gfx] [v2] drm/i915/gen11: Moving WAs to icl_gt_workarounds_init()

[John Harrison <john.c.harrison@intel.com>]

On 12/10/2021 17:22, Matt Roper wrote:
> On Thu, Dec 09, 2021 at 09:21:39AM -0800, Lucas De Marchi wrote:
>> On Fri, Dec 03, 2021 at 08:26:03PM +0530, ravitejax.goud.talla@intel.com wrote:
>>> From: Raviteja Goud Talla <ravitejax.goud.talla@intel.com>
>>>
>>> Bspec page says "Reset: BUS", Accordingly moving w/a's:
>>> Wa_1407352427,Wa_1406680159 to proper function icl_gt_workarounds_init()
>>> Which will resolve guc enabling error
>>>
>>> v2:
>>>   - Previous patch rev2 was created by email client which caused the
>>>     Build failure, This v2 is to resolve the previous broken series
>>>
>>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>>> Signed-off-by: Raviteja Goud Talla <ravitejax.goud.talla@intel.com>
>> It seems like this patch is needed to be able to load GuC in ICL:
>> https://gitlab.freedesktop.org/drm/intel/-/issues/4067#note_1184951
>>
>> And that is failing on Linus' master branch as of
>> 2a987e65025e ("Merge tag 'perf-tools-fixes-for-v5.16-2021-12-07' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux")
>> and (at least) 5.15.*. Looking at the appropriate "Fixes" tag I found these commits:
>>
>>    1) 1cd21a7c5679 ("drm/i915: Add Wa_1407352427:icl,ehl") - original
>>       commit adding the WA
>>    2) 3551ff928744 ("drm/i915/gen11: Moving WAs to rcs_engine_wa_init()")
>>       moving the WA to rcs_engine_wa_init()
>>
>> However (2) is on v5.7-rc1 and (1) is on v5.6-rc1 and according to the
>> bug report GuC loading was working on 5.13. So I suspect the move
>> to GuC 62.0.0 made the checks more strict, or there is another patch
> This is correct.  Having "GT workarounds" on the engine list by accident
> used to be harmless (because i915 [with execlist submission] and the guc
> [with guc submission]) would simply re-apply the register setting more
> often than it really needed to.  However the more recent GuC versions
> have become more picky about the set of registers they're willing to
Actually, I think the GuC was always picky but we haven't supported GuC 
submission upstream for quite some time. There was very old support 
(never enabled by default) in the VLV timescale. And at that time, we 
were not using GuC scheduling anyway, so no save/restore list. I think 
by ICL it had already been removed as broken. All you could do was load 
the GuC in order to load the HuC. It is only recently with the ADL-P/DG1 
support that GuC submission has been re-implemented and the save/restore 
list added in.


> save/restore for workarounds and will fail to load if they see a
> register on the save/restore list that isn't something they think is
> appropriate for an engine reset.
>
> GuC submission isn't officially supported on ICL; you can force it via
> module parameter, which taints the kernel and takes you through untested
> codepaths, so you do so at your own risk.  Given that, I don't think
> there's any real need to worry about getting this backported to specific
> stable kernels; having the workaround in the wrong place doesn't
> actually harm anything on the official and default non-GuC mode.
As above, even with GuC enabled, I still don't think it is a problem for 
any kernel using a GuC version earlier than 62.0.0.

John.


>
>
> Matt
>
>> that paired with that one makes load fail to load.
>>
>> Anyway, it seems that commit is the one to fix and we probably need to
>> bring both moves to stable (5.15.y and 5.10.y ?), besides propagating
>> it to drm-intel-fixes so it applies to 5.16
>>
>> Cc'ing some more people.
>>
>>
>> Lucas De Marchi
>>
>>
>>
>>
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_workarounds.c | 18 +++++++++---------
>>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>> index c3211325c2d3..3113266c286e 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>> @@ -1224,6 +1224,15 @@ icl_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
>>> 		    GAMT_CHKN_BIT_REG,
>>> 		    GAMT_CHKN_DISABLE_L3_COH_PIPE);
>>>
>>> +	/* Wa_1407352427:icl,ehl */
>>> +	wa_write_or(wal, UNSLICE_UNIT_LEVEL_CLKGATE2,
>>> +		    PSDUNIT_CLKGATE_DIS);
>>> +
>>> +	/* Wa_1406680159:icl,ehl */
>>> +	wa_write_or(wal,
>>> +		    SUBSLICE_UNIT_LEVEL_CLKGATE,
>>> +		    GWUNIT_CLKGATE_DIS);
>>> +
>>> 	/* Wa_1607087056:icl,ehl,jsl */
>>> 	if (IS_ICELAKE(i915) ||
>>> 	    IS_JSL_EHL_GRAPHICS_STEP(i915, STEP_A0, STEP_B0))
>>> @@ -2269,15 +2278,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>>> 		wa_write_or(wal, UNSLICE_UNIT_LEVEL_CLKGATE,
>>> 			    VSUNIT_CLKGATE_DIS | HSUNIT_CLKGATE_DIS);
>>>
>>> -		/* Wa_1407352427:icl,ehl */
>>> -		wa_write_or(wal, UNSLICE_UNIT_LEVEL_CLKGATE2,
>>> -			    PSDUNIT_CLKGATE_DIS);
>>> -
>>> -		/* Wa_1406680159:icl,ehl */
>>> -		wa_write_or(wal,
>>> -			    SUBSLICE_UNIT_LEVEL_CLKGATE,
>>> -			    GWUNIT_CLKGATE_DIS);
>>> -
>>> 		/*
>>> 		 * Wa_1408767742:icl[a2..forever],ehl[all]
>>> 		 * Wa_1605460711:icl[a0..c0]
>>> -- 
>>> 2.34.1
>>>