Re: [Intel-gfx] [PATCH] drm/i915: Fix workarounds on Gen2-3

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 18, 2022 at 11:52:49AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> In 3653727560d0 ("drm/i915: Simplify internal helper function signature")
> I broke the old platforms by not noticing engine workaround init does not
> initialize the list on old platforms. Fix it by always initializing which
> already does the right thing by mostly not doing anything if there aren't
> any workarounds on the list.

Was going to give this a quick smoke test on my 865 but I can't even
reproduce the original issue on it.

Turns out the 64bit compiler is too smart:
0000000000000000 <wa_list_apply>:
       0:       8b 77 20                mov    0x20(%rdi),%esi
       3:       85 f6                   test   %esi,%esi
       5:       75 01                   jne    8 <wa_list_apply+0x8>
       7:       c3                      ret
       8:       41 57                   push   %r15
       a:       41 56                   push   %r14
       c:       41 55                   push   %r13
       e:       41 54                   push   %r12
      10:       55                      push   %rbp
      11:       53                      push   %rbx
      12:       48 83 ec 10             sub    $0x10,%rsp
      16:       48 89 fd                mov    %rdi,%rbp
      19:       4c 8b 2f                mov    (%rdi),%r13

So it has moved the wal->count check to be the very first thing,
even before even doing any stack setup.

The 32bit compiler is somewhat less smart:
00000000 <wa_list_apply>:
       0:       55                      push   %ebp
       1:       89 e5                   mov    %esp,%ebp
       3:       57                      push   %edi
       4:       56                      push   %esi
       5:       53                      push   %ebx
       6:       83 ec 10                sub    $0x10,%esp
       9:       89 45 f0                mov    %eax,-0x10(%ebp)
       c:       8b 58 10                mov    0x10(%eax),%ebx
       f:       8b 38                   mov    (%eax),%edi
      11:       85 db                   test   %ebx,%ebx
      13:       89 7d e8                mov    %edi,-0x18(%ebp)
      16:       8b 7f 0c                mov    0xc(%edi),%edi
      19:       75 0d                   jne    28 <wa_list_apply+0x28>

Not only does it do all that potentially pointless stack
setup, but then it has decided to do a bunch of stuff
with wal->gt before the jne.

That presumably explains why CI is still green despite blb/pnv.

Hmm. Now a different 32bit build also failed to hit this:
00000003 <wa_list_apply>:
       3:       55                      push   %ebp
       4:       89 e5                   mov    %esp,%ebp
       6:       57                      push   %edi
       7:       56                      push   %esi
       8:       53                      push   %ebx
       9:       83 ec 14                sub    $0x14,%esp
       c:       89 45 f0                mov    %eax,-0x10(%ebp)
       f:       8b 58 10                mov    0x10(%eax),%ebx
      12:       85 db                   test   %ebx,%ebx
      14:       75 08                   jne    1e <wa_list_apply+0x1b>

So this time it moved the wal->gt stuff to some later point.
Same compiler, different .config. Not sure which knob is
causing the difference here.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: 3653727560d0 ("drm/i915: Simplify internal helper function signature")
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 213160f29ec3..4d7a01b45e09 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -2991,7 +2991,7 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li
>  static void
>  engine_init_workarounds(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>  {
> -	if (I915_SELFTEST_ONLY(GRAPHICS_VER(engine->i915) < 4))
> +	if (GRAPHICS_VER(engine->i915) < 4)
>  		return;
>  
>  	engine_fake_wa_init(engine, wal);
> @@ -3016,9 +3016,6 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
>  {
>  	struct i915_wa_list *wal = &engine->wa_list;
>  
> -	if (GRAPHICS_VER(engine->i915) < 4)
> -		return;
> -
>  	wa_init_start(wal, engine->gt, "engine", engine->name);
>  	engine_init_workarounds(engine, wal);
>  	wa_init_finish(wal);
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel