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

[Andi Shyti]

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