[Intel-gfx] [PATCH 08/34] drm/i915: Make all GPU resets atomic

[Chris Wilson]

Quoting John Harrison (2019-01-22 22:19:04)
> On 1/21/2019 14:20, Chris Wilson wrote:
> > In preparation for the next few commits, make resetting the GPU atomic.
> > Currently, we have prepared gen6+ for atomic resetting of individual
> > engines, but now there is a requirement to perform the whole device
> > level reset (just the register poking) from inside an atomic context.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_reset.c | 50 +++++++++++++++++--------------
> >   1 file changed, 27 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> > index 342d9ee42601..b9d0ea70361c 100644
> > --- a/drivers/gpu/drm/i915/i915_reset.c
> > +++ b/drivers/gpu/drm/i915/i915_reset.c
> > @@ -144,14 +144,14 @@ static int i915_do_reset(struct drm_i915_private *i915,
> >   
> >       /* Assert reset for at least 20 usec, and wait for acknowledgement. */
> >       pci_write_config_byte(pdev, I915_GDRST, GRDOM_RESET_ENABLE);
> > -     usleep_range(50, 200);
> > -     err = wait_for(i915_in_reset(pdev), 500);
> > +     udelay(50);
> > +     err = wait_for_atomic(i915_in_reset(pdev), 50);

> Is it known to be safe to reduce all of these time out values? Where did 
> the originally 500ms value come from?

I chose it entirely upon a whim, picking a huge number unlikely to ever
be exceeded, and if it were we would be right to conclude the HW was
unrecoverable.

> Is there any chance of getting 
> sporadic failures because 50ms is borderline in the worst case scenario? 
> It still sounds huge but an order of magnitude change in a timeout 
> always seems worrying!

Whereas 50us is more in line with the little bits of documentation that
still exist.

> > @@ -218,27 +218,29 @@ static int ironlake_do_reset(struct drm_i915_private *dev_priv,
> >   {
> >       int ret;
> >   
> > -     I915_WRITE(ILK_GDSR, ILK_GRDOM_RENDER | ILK_GRDOM_RESET_ENABLE);
> > -     ret = intel_wait_for_register(dev_priv,
> > -                                   ILK_GDSR, ILK_GRDOM_RESET_ENABLE, 0,
> > -                                   500);
> > +     I915_WRITE_FW(ILK_GDSR, ILK_GRDOM_RENDER | ILK_GRDOM_RESET_ENABLE);
> > +     ret = __intel_wait_for_register_fw(dev_priv, ILK_GDSR,
> > +                                        ILK_GRDOM_RESET_ENABLE, 0,
> > +                                        5000, 0,
> > +                                        NULL);
> These two timeouts are now two orders of magnitude smaller? It was 500ms 
> but is now 5000us (=5ms)?

0.5 was the same number plucked from the air. No guidance here, that I
know of, except we have lots of runs through CI to try and estimate
bounds.
-Chris