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 <ch...@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 <h...@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: sta...@vger.kernel.org > > Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com> > > Signed-off-by: Andi Shyti <andi.sh...@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 > >