On Fri, Apr 21, 2023 at 08:05:50AM -0700, Matt Roper wrote:
> On Wed, Apr 19, 2023 at 02:40:33PM -0700, Matt Atwood wrote:
> > On Tue, Apr 18, 2023 at 03:04:45PM -0700, Radhakrishna Sripada wrote:
> > > From: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@xxxxxxxxx>
> > >
> > > Wa_22011802037 was being applied to all graphics_ver 11 & 12. This patch
> > > updates the if statement to apply the W/A to right platforms and extends
> > > it to MTL-M:A step.
> > >
> > Bspec: 53509
> > > v1.1: Fix checkpatch warning.
> > > v2: Change the check to reflect the wa at other palces(Lucas)
> > s/palces/places.
> > >
> > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> > With that.
> > Reviewed-by: Matt Atwood <matthew.s.atwood@xxxxxxxxx>
> > > Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@xxxxxxxxx>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
>
> It doesn't look like this patch is complete? It's only changing one
> condition for Wa_22011802037, even though there are several in the code.
> From a quick grep, you're still missing updates for at least
> guc_ctl_wa_flags, execlists_reset_prepare, and __intel_engine_stop_cs.
Actually, scratch that. Those other spots already have a MTL clause as
part of the condition. But in that case it means the commit message
here is inaccurate; you're not extending this workaround to MTL a-step
because that already happened on a previous patch. You're just
providing a fix for an incomplete implementation that happened earlier.
The commit message should be explaining that.
>
> Since this workaround is a complicated one that touches so many areas of
> the code, and has a complex platform list, it's probably time to factor
> the condition out into a needs_wa_22011802037() helper or something.
I still suggest doing this, especially since we've clearly screwed up
the handling of this workaround at least once already.
Matt
>
>
> Matt
>
> > > ---
> > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 ++++++------
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > index 88e881b100cf..ee3e8352637f 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > > @@ -1629,16 +1629,16 @@ static void guc_reset_state(struct intel_context *ce, u32 head, bool scrub)
> > >
> > > static void guc_engine_reset_prepare(struct intel_engine_cs *engine)
> > > {
> > > - if (!IS_GRAPHICS_VER(engine->i915, 11, 12))
> > > - return;
> > > -
> > > - intel_engine_stop_cs(engine);
> > > -
> > > /*
> > > * Wa_22011802037: In addition to stopping the cs, we need
> > > * to wait for any pending mi force wakeups
> > > */
> > > - intel_engine_wait_for_pending_mi_fw(engine);
> > > + if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) ||
> > > + (GRAPHICS_VER(engine->i915) >= 11 &&
> > > + GRAPHICS_VER_FULL(engine->i915) < IP_VER(12, 70))) {
> > > + intel_engine_stop_cs(engine);
> > > + intel_engine_wait_for_pending_mi_fw(engine);
> > > + }
> > > }
> > >
> > > static void guc_reset_nop(struct intel_engine_cs *engine)
> > > --
> > > 2.34.1
> > >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
