[Intel-gfx] [PATCH v2 3/4] drm/i915/mtl: Extend Wa_22011802037 to MTL A-step

[Sripada, Radhakrishna]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Friday, April 21, 2023 8:09 AM
> To: Atwood, Matthew S <matthew.s.atwood at intel.com>
> Cc: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 3/4] drm/i915/mtl: Extend Wa_22011802037
> to MTL A-step
> 
> 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 at intel.com>
> > > >
> > > > 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 at intel.com>
> > > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> > > With that.
> > > Reviewed-by: Matt Atwood <matthew.s.atwood at intel.com>
> > > > Signed-off-by: Madhumitha Tolakanahalli Pradeep
> <madhumitha.tolakanahalli.pradeep at intel.com>
> > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> >
> > 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.
> 

Sure Matt. With the patch already merged, I will handle this as a separate patch.

- Radhakrishna(RK) Sripada
> 
> 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