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

[Lucas De Marchi]

On Fri, Dec 10, 2021 at 05:22:54PM -0800, 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 at intel.com wrote:
>> > From: Raviteja Goud Talla <ravitejax.goud.talla at 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 at Intel.com>
>> > Signed-off-by: Raviteja Goud Talla <ravitejax.goud.talla at 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
>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.

the discussion on gitlab sidetracked talking about GuC submission. This
is _not_ about GuC submission though:  it's enable_guc=2 so it's simply
being used to load HuC.

Given it's a trivial move of the WA to the _right_ place that allows
people to continue using HuC as they were, I strongly disagree with
that statement. Yes, people use the module parameters at their own risk,
but we do fix it when it makes sense.  If it was a pretty complex patch
we could decide to the other side of not porting it, but it's not the
case here.

Lucas De Marchi