| Message ID | 20210119094053.6919-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/gt: One more flush for Baytrail clear residuals | expand |
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. As in this point the 'ring must be empty' is satisfied. For this patch, Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > + ENGINE_WRITE_FW(engine, RING_HEAD, 0); > + ENGINE_WRITE_FW(engine, RING_TAIL, 0); > + > + /* The ring must be empty before it is disabled */ > + ENGINE_WRITE_FW(engine, RING_CTL, 0); > + > + return (ENGINE_READ_FW(engine, RING_HEAD) & HEAD_ADDR) == 0; > +} > + > static void reset_prepare(struct intel_engine_cs *engine) > { > - struct intel_uncore *uncore = engine->uncore; > - const u32 base = engine->mmio_base; > - > /* > * We stop engines, otherwise we might get failed reset and a > * dead gpu (on elk). Also as modern gpu as kbl can suffer > @@ -355,30 +325,35 @@ static void reset_prepare(struct intel_engine_cs *engine) > * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES) > * > * WaMediaResetMainRingCleanup:ctg,elk (presumably) > + * WaClearRingBufHeadRegAtInit:ctg,elk > * > * FIXME: Wa for more modern gens needs to be validated > */ > ENGINE_TRACE(engine, "\n"); > + intel_engine_stop_cs(engine); > > - if (intel_engine_stop_cs(engine)) > - ENGINE_TRACE(engine, "timed out on STOP_RING\n"); > + if (!stop_ring(engine)) { > + /* G45 ring initialization often fails to reset head to zero */ > + drm_dbg(&engine->i915->drm, > + "%s head not reset to zero " > + "ctl %08x head %08x tail %08x start %08x\n", > + engine->name, > + ENGINE_READ_FW(engine, RING_CTL), > + ENGINE_READ_FW(engine, RING_HEAD), > + ENGINE_READ_FW(engine, RING_TAIL), > + ENGINE_READ_FW(engine, RING_START)); > + } > > - intel_uncore_write_fw(uncore, > - RING_HEAD(base), > - intel_uncore_read_fw(uncore, RING_TAIL(base))); > - intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */ > - > - intel_uncore_write_fw(uncore, RING_HEAD(base), 0); > - intel_uncore_write_fw(uncore, RING_TAIL(base), 0); > - intel_uncore_posting_read_fw(uncore, RING_TAIL(base)); > - > - /* The ring must be empty before it is disabled */ > - intel_uncore_write_fw(uncore, RING_CTL(base), 0); > - > - /* Check acts as a post */ > - if (intel_uncore_read_fw(uncore, RING_HEAD(base))) > - ENGINE_TRACE(engine, "ring head [%x] not parked\n", > - intel_uncore_read_fw(uncore, RING_HEAD(base))); > + if (!stop_ring(engine)) { > + drm_err(&engine->i915->drm, > + "failed to set %s head to zero " > + "ctl %08x head %08x tail %08x start %08x\n", > + engine->name, > + ENGINE_READ_FW(engine, RING_CTL), > + ENGINE_READ_FW(engine, RING_HEAD), > + ENGINE_READ_FW(engine, RING_TAIL), > + ENGINE_READ_FW(engine, RING_START)); > + } > } > > static void reset_rewind(struct intel_engine_cs *engine, bool stalled) > -- > 2.20.1
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
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)); + + ENGINE_WRITE_FW(engine, RING_HEAD, 0); + ENGINE_WRITE_FW(engine, RING_TAIL, 0); + + /* The ring must be empty before it is disabled */ + ENGINE_WRITE_FW(engine, RING_CTL, 0); + + return (ENGINE_READ_FW(engine, RING_HEAD) & HEAD_ADDR) == 0; +} + static void reset_prepare(struct intel_engine_cs *engine) { - struct intel_uncore *uncore = engine->uncore; - const u32 base = engine->mmio_base; - /* * We stop engines, otherwise we might get failed reset and a * dead gpu (on elk). Also as modern gpu as kbl can suffer @@ -355,30 +325,35 @@ static void reset_prepare(struct intel_engine_cs *engine) * WaKBLVECSSemaphoreWaitPoll:kbl (on ALL_ENGINES) * * WaMediaResetMainRingCleanup:ctg,elk (presumably) + * WaClearRingBufHeadRegAtInit:ctg,elk * * FIXME: Wa for more modern gens needs to be validated */ ENGINE_TRACE(engine, "\n"); + intel_engine_stop_cs(engine); - if (intel_engine_stop_cs(engine)) - ENGINE_TRACE(engine, "timed out on STOP_RING\n"); + if (!stop_ring(engine)) { + /* G45 ring initialization often fails to reset head to zero */ + drm_dbg(&engine->i915->drm, + "%s head not reset to zero " + "ctl %08x head %08x tail %08x start %08x\n", + engine->name, + ENGINE_READ_FW(engine, RING_CTL), + ENGINE_READ_FW(engine, RING_HEAD), + ENGINE_READ_FW(engine, RING_TAIL), + ENGINE_READ_FW(engine, RING_START)); + } - intel_uncore_write_fw(uncore, - RING_HEAD(base), - intel_uncore_read_fw(uncore, RING_TAIL(base))); - intel_uncore_posting_read_fw(uncore, RING_HEAD(base)); /* paranoia */ - - intel_uncore_write_fw(uncore, RING_HEAD(base), 0); - intel_uncore_write_fw(uncore, RING_TAIL(base), 0); - intel_uncore_posting_read_fw(uncore, RING_TAIL(base)); - - /* The ring must be empty before it is disabled */ - intel_uncore_write_fw(uncore, RING_CTL(base), 0); - - /* Check acts as a post */ - if (intel_uncore_read_fw(uncore, RING_HEAD(base))) - ENGINE_TRACE(engine, "ring head [%x] not parked\n", - intel_uncore_read_fw(uncore, RING_HEAD(base))); + if (!stop_ring(engine)) { + drm_err(&engine->i915->drm, + "failed to set %s head to zero " + "ctl %08x head %08x tail %08x start %08x\n", + engine->name, + ENGINE_READ_FW(engine, RING_CTL), + ENGINE_READ_FW(engine, RING_HEAD), + ENGINE_READ_FW(engine, RING_TAIL), + ENGINE_READ_FW(engine, RING_START)); + } } static void reset_rewind(struct intel_engine_cs *engine, bool stalled)
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(-)