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

Andi Shyti andi.shyti at linux.intel.com
Thu Dec 22 13:47:12 UTC 2022


Hi GG,

> > > > > > >   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);

> Andi, fast_timeout_us is increased from 500 to 2000, and if it fails, it
> tries to reset it once more. How was this value of 2000 calculated?

No real reason, it's just an empiric choice to make the call a
bit more robust and suffer less from delayed feedback.

> > > > > > > +       } while (err == 0 && --loops);
> > > > > > >          if (err)
> > > > > > >                  GT_TRACE(gt,
> > > > > > >                           "Wait for 0x%08x engines reset
> > > > > > > failed\n",
> > > > > > >                           hw_domain_mask);

> Did GT_TRACE report an error in a situation where the problem was reported?

I guess so, in Jasperlake.

> > > > > > > +       /*
> > > > > > > +        * 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);

> udelay(50) affects all platforms that can call gen6_hw_domain_reset(), is
> that intended?

Yes, that's intended as apparently we need to give it a bit more
time for the engines to recover from the reset. We are here in
atomic context and we need udelay to wait atomically, thus
udelay().

Thank you,
Andi


More information about the dri-devel mailing list