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 21:30, Lucas De Marchi wrote:
> On Fri, Dec 10, 2021 at 05:41:46PM -0800, John Harrison wrote:
>> 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.
>
> as I said in my other reply to Matt, it's not GuC sumbmission that got
> broken though:  it's enable_guc=2.
My point is that it wasn't broken until we added the GuC submission 
support. Doesn't matter whether submission is enabled or not. Until it 
was implemented, there was no save/restore list. After it was 
implemented, the list was created even for enable_guc=2 as it is part of 
initialising the GuC.

John.

>
>>
>>
>>> 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.
>
> so that rules out 5.10 and the only stable we need this on is 5.15 which
> is pretty easy and applies cleanly.
>
> Lucas De Marchi