Re: [Intel-gfx] [PATCH] drm/i915/gt: Reset twice

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Rodrigo,

On Mon, Dec 12, 2022 at 11:55:10AM -0500, Rodrigo Vivi wrote:
> On Mon, Dec 12, 2022 at 05:13:38PM +0100, Andi Shyti wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > After applying an engine reset, on some platforms like Jasperlake, we
> > occasionally detect that the engine state is not cleared until shortly
> > after the resume. As we try to resume the engine with volatile internal
> > state, the first request fails with a spurious CS event (it looks like
> > it reports a lite-restore to the hung context, instead of the expected
> > idle->active context switch).
> > 
> > Signed-off-by: Chris Wilson <hris@chris-wilson.co.uk>
> 
> There's a typo in the signature email I'm afraid...

oh yes, I forgot the 'C' :)

> Other than that, have we checked the possibility of using the driver-initiated-flr bit
> instead of this second loop? That should be the right way to guarantee everything is
> cleared on gen11+...

maybe I am misinterpreting it, but is FLR the same as resetting
hardware domains individually?

How am I supposed to use driver_initiated_flr() in this context?

Thanks,
Andi

> > Cc: stable@vger.kernel.org
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_reset.c | 34 ++++++++++++++++++++++-----
> >  1 file changed, 28 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> > index ffde89c5835a4..88dfc0c5316ff 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> > @@ -268,6 +268,7 @@ static int ilk_do_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask,
> >  static int gen6_hw_domain_reset(struct intel_gt *gt, u32 hw_domain_mask)
> >  {
> >  	struct intel_uncore *uncore = gt->uncore;
> > +	int loops = 2;
> >  	int err;
> >  
> >  	/*
> > @@ -275,18 +276,39 @@ static int gen6_hw_domain_reset(struct intel_gt *gt, u32 hw_domain_mask)
> >  	 * for fifo space for the write or forcewake the chip for
> >  	 * the read
> >  	 */
> > -	intel_uncore_write_fw(uncore, GEN6_GDRST, hw_domain_mask);
> > +	do {
> > +		intel_uncore_write_fw(uncore, GEN6_GDRST, hw_domain_mask);
> >  
> > -	/* Wait for the device to ack the reset requests */
> > -	err = __intel_wait_for_register_fw(uncore,
> > -					   GEN6_GDRST, hw_domain_mask, 0,
> > -					   500, 0,
> > -					   NULL);
> > +		/*
> > +		 * Wait for the device to ack the reset requests.
> > +		 *
> > +		 * On some platforms, e.g. Jasperlake, we see see that the
> > +		 * engine register state is not cleared until shortly after
> > +		 * GDRST reports completion, causing a failure as we try
> > +		 * to immediately resume while the internal state is still
> > +		 * in flux. If we immediately repeat the reset, the second
> > +		 * reset appears to serialise with the first, and since
> > +		 * it is a no-op, the registers should retain their reset
> > +		 * value. However, there is still a concern that upon
> > +		 * leaving the second reset, the internal engine state
> > +		 * is still in flux and not ready for resuming.
> > +		 */
> > +		err = __intel_wait_for_register_fw(uncore, GEN6_GDRST,
> > +						   hw_domain_mask, 0,
> > +						   2000, 0,
> > +						   NULL);
> > +	} while (err == 0 && --loops);
> >  	if (err)
> >  		GT_TRACE(gt,
> >  			 "Wait for 0x%08x engines reset failed\n",
> >  			 hw_domain_mask);
> >  
> > +	/*
> > +	 * As we have observed that the engine state is still volatile
> > +	 * after GDRST is acked, impose a small delay to let everything settle.
> > +	 */
> > +	udelay(50);
> > +
> >  	return err;
> >  }
> >  
> > -- 
> > 2.38.1
> >