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

[Matt Roper <matthew.d.roper@intel.com>]

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@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@intel.com>
> > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > With that.
> > Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com>
> > > Signed-off-by: Madhumitha Tolakanahalli Pradeep <madhumitha.tolakanahalli.pradeep@intel.com>
> > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@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.


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