Re: [Intel-gfx] [PATCH 3/5] drm/i915/gt: Lift stop_ring() to reset_prepare

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Mika Kuoppala (2021-01-19 10:33:17)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Push the sleeping stop_ring() out of the reset resume function to reset
> > prepare; we are not allowed to sleep in the former.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  .../gpu/drm/i915/gt/intel_ring_submission.c   | 97 +++++++------------
> >  1 file changed, 36 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > index 8d0964d2d597..44159595d909 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> > @@ -157,21 +157,6 @@ static void ring_setup_status_page(struct intel_engine_cs *engine)
> >       flush_cs_tlb(engine);
> >  }
> >  
> > -static bool stop_ring(struct intel_engine_cs *engine)
> > -{
> > -     intel_engine_stop_cs(engine);
> > -
> > -     ENGINE_WRITE(engine, RING_HEAD, ENGINE_READ(engine, RING_TAIL));
> > -
> > -     ENGINE_WRITE(engine, RING_HEAD, 0);
> > -     ENGINE_WRITE(engine, RING_TAIL, 0);
> > -
> > -     /* The ring must be empty before it is disabled */
> > -     ENGINE_WRITE(engine, RING_CTL, 0);
> > -
> > -     return (ENGINE_READ(engine, RING_HEAD) & HEAD_ADDR) == 0;
> > -}
> > -
> >  static struct i915_address_space *vm_alias(struct i915_address_space *vm)
> >  {
> >       if (i915_is_ggtt(vm))
> > @@ -213,31 +198,6 @@ static int xcs_resume(struct intel_engine_cs *engine)
> >  
> >       intel_uncore_forcewake_get(engine->uncore, FORCEWAKE_ALL);
> >  
> > -     /* WaClearRingBufHeadRegAtInit:ctg,elk */
> > -     if (!stop_ring(engine)) {
> > -             /* G45 ring initialization often fails to reset head to zero */
> > -             drm_dbg(&dev_priv->drm, "%s head not reset to zero "
> > -                     "ctl %08x head %08x tail %08x start %08x\n",
> > -                     engine->name,
> > -                     ENGINE_READ(engine, RING_CTL),
> > -                     ENGINE_READ(engine, RING_HEAD),
> > -                     ENGINE_READ(engine, RING_TAIL),
> > -                     ENGINE_READ(engine, RING_START));
> > -
> > -             if (!stop_ring(engine)) {
> > -                     drm_err(&dev_priv->drm,
> > -                             "failed to set %s head to zero "
> > -                             "ctl %08x head %08x tail %08x start %08x\n",
> > -                             engine->name,
> > -                             ENGINE_READ(engine, RING_CTL),
> > -                             ENGINE_READ(engine, RING_HEAD),
> > -                             ENGINE_READ(engine, RING_TAIL),
> > -                             ENGINE_READ(engine, RING_START));
> > -                     ret = -EIO;
> > -                     goto out;
> > -             }
> > -     }
> > -
> >       if (HWS_NEEDS_PHYSICAL(dev_priv))
> >               ring_setup_phys_status_page(engine);
> >       else
> > @@ -339,11 +299,21 @@ static void xcs_sanitize(struct intel_engine_cs *engine)
> >       clflush_cache_range(engine->status_page.addr, PAGE_SIZE);
> >  }
> >  
> > +static bool stop_ring(struct intel_engine_cs *engine)
> > +{
> > +     ENGINE_WRITE_FW(engine, RING_HEAD, ENGINE_READ_FW(engine, RING_TAIL));
> > +
> 
> Not related to this patch but this makes me wondering if the actual
> disable should be at this point before zeroing as manipulating the
> head again might kick the hardware forward.

We move HEAD to TAIL, with the intent of kicking the HW to the end and
"emptying the ring".

If we move HEAD to 0 first, it will start executing random stuff. And by
the time we set TAIL to 0, HEAD will have moved on.

So I think this is correct: set HEAD to TAIL to empty the ring.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx