| Message ID | 20221118115249.2683946-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix workarounds on Gen2-3 | expand |
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. > > 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; Do we even need this early return at all? As far as I can see, letting this function run its course doesn't wind up having any effect or cause any problems (you still wind up with an empty list). Regardless, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > 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 >
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
On 18/11/2022 17:14, Matt Roper wrote: > 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. >> >> 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; > > Do we even need this early return at all? As far as I can see, letting > this function run its course doesn't wind up having any effect or cause > any problems (you still wind up with an empty list). True, it looks to me like that as well, now that you are pointing it out. Btw originally I was most perplexed by the "selftests only" annotation, but did not find time to go digging through history to figure out why was that even needed. I left the return as is for now and pushed it to fix the breakage. Will try to revisit this at some point. Thanks for the review! Regards, Tvrtko > > Regardless, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > >> >> 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 >> >
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);