| Message ID | 20180628115908.31933-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/06/2018 12:59, Chris Wilson wrote: > On HW reset, the HW clears the write pointer (to 0). But since it also > writes its first CSB entry to slot 0, we need to reset the write pointer > back to the element before (so the first entry we read is 0). > > This is required for the next patch, where we trust the CSB completely! > > v2: Use _MASKED_FIELD > v3: Store the reset value, so that we differentiate between mmio/hwsp > transparently and without pretense. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 23 +++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_ringbuffer.h | 9 +++++++++ > 2 files changed, 30 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 8531a5b6f6ff..49bf5048043c 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -884,6 +884,21 @@ static void reset_irq(struct intel_engine_cs *engine) > clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > } > > +static void reset_csb_pointers(struct intel_engine_execlists *execlists) > +{ > + /* > + * After a reset, the HW starts writing into CSB entry [0]. We > + * therefore have to set our HEAD pointer back one entry so that > + * the *first* entry we check is entry 0. To complicate this further, > + * as we don't wait for the first interrupt after reset, we have to > + * fake the HW write to point back to the last entry so that our > + * inline comparison of our cached head position against the last HW > + * write works even before the first interrupt. > + */ > + execlists->csb_head = execlists->csb_write_reset & 0xff; Idea for avoiding the & 0xff and applying the trick throughout - make csb_head u8. :) > + WRITE_ONCE(*execlists->csb_write, execlists->csb_write_reset); > +} > + > static void execlists_cancel_requests(struct intel_engine_cs *engine) > { > struct intel_engine_execlists * const execlists = &engine->execlists; > @@ -1970,7 +1985,7 @@ static void execlists_reset(struct intel_engine_cs *engine, > __unwind_incomplete_requests(engine); > > /* Following the reset, we need to reload the CSB read/write pointers */ > - engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1; > + reset_csb_pointers(&engine->execlists); > > spin_unlock_irqrestore(&engine->timeline.lock, flags); > > @@ -2469,7 +2484,6 @@ static int logical_ring_init(struct intel_engine_cs *engine) > upper_32_bits(ce->lrc_desc); > } > > - execlists->csb_head = GEN8_CSB_ENTRIES - 1; > execlists->csb_read = > i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)); > if (csb_force_mmio(i915)) { > @@ -2477,13 +2491,18 @@ static int logical_ring_init(struct intel_engine_cs *engine) > (i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0))); > > execlists->csb_write = (u32 __force *)execlists->csb_read; > + execlists->csb_write_reset = > + _MASKED_FIELD(GEN8_CSB_WRITE_PTR_MASK, > + GEN8_CSB_ENTRIES - 1); > } else { > execlists->csb_status = > &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX]; > > execlists->csb_write = > &engine->status_page.page_addr[intel_hws_csb_write_index(i915)]; > + execlists->csb_write_reset = GEN8_CSB_ENTRIES - 1; > } > + reset_csb_pointers(execlists); > > return 0; > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 970dbb3c9812..6ec7c019b5d9 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -304,6 +304,15 @@ struct intel_engine_execlists { > */ > unsigned int csb_head; > > + /** > + * @csb_write_reset: reset value for CSB write pointer > + * > + * As the CSB write pointer maybe either in HWSP or as a field > + * inside an mmio register, we want to reprogram it slightly > + * differently to avoid later confusion. > + */ > + u32 csb_write_reset; > + > /** > * @csb_read: control register for Context Switch buffer > * > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-06-28 13:15:07) > > On 28/06/2018 12:59, Chris Wilson wrote: > > On HW reset, the HW clears the write pointer (to 0). But since it also > > writes its first CSB entry to slot 0, we need to reset the write pointer > > back to the element before (so the first entry we read is 0). > > > > This is required for the next patch, where we trust the CSB completely! > > > > v2: Use _MASKED_FIELD > > v3: Store the reset value, so that we differentiate between mmio/hwsp > > transparently and without pretense. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 23 +++++++++++++++++++++-- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 9 +++++++++ > > 2 files changed, 30 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 8531a5b6f6ff..49bf5048043c 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -884,6 +884,21 @@ static void reset_irq(struct intel_engine_cs *engine) > > clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); > > } > > > > +static void reset_csb_pointers(struct intel_engine_execlists *execlists) > > +{ > > + /* > > + * After a reset, the HW starts writing into CSB entry [0]. We > > + * therefore have to set our HEAD pointer back one entry so that > > + * the *first* entry we check is entry 0. To complicate this further, > > + * as we don't wait for the first interrupt after reset, we have to > > + * fake the HW write to point back to the last entry so that our > > + * inline comparison of our cached head position against the last HW > > + * write works even before the first interrupt. > > + */ > > + execlists->csb_head = execlists->csb_write_reset & 0xff; > > Idea for avoiding the & 0xff and applying the trick throughout - make > csb_head u8. :) Just leaves intel_engine_execlists full of holes. It did close my mind. They may just spontaneously rearrange themselves... -Chris
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 8531a5b6f6ff..49bf5048043c 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -884,6 +884,21 @@ static void reset_irq(struct intel_engine_cs *engine) clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted); } +static void reset_csb_pointers(struct intel_engine_execlists *execlists) +{ + /* + * After a reset, the HW starts writing into CSB entry [0]. We + * therefore have to set our HEAD pointer back one entry so that + * the *first* entry we check is entry 0. To complicate this further, + * as we don't wait for the first interrupt after reset, we have to + * fake the HW write to point back to the last entry so that our + * inline comparison of our cached head position against the last HW + * write works even before the first interrupt. + */ + execlists->csb_head = execlists->csb_write_reset & 0xff; + WRITE_ONCE(*execlists->csb_write, execlists->csb_write_reset); +} + static void execlists_cancel_requests(struct intel_engine_cs *engine) { struct intel_engine_execlists * const execlists = &engine->execlists; @@ -1970,7 +1985,7 @@ static void execlists_reset(struct intel_engine_cs *engine, __unwind_incomplete_requests(engine); /* Following the reset, we need to reload the CSB read/write pointers */ - engine->execlists.csb_head = GEN8_CSB_ENTRIES - 1; + reset_csb_pointers(&engine->execlists); spin_unlock_irqrestore(&engine->timeline.lock, flags); @@ -2469,7 +2484,6 @@ static int logical_ring_init(struct intel_engine_cs *engine) upper_32_bits(ce->lrc_desc); } - execlists->csb_head = GEN8_CSB_ENTRIES - 1; execlists->csb_read = i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)); if (csb_force_mmio(i915)) { @@ -2477,13 +2491,18 @@ static int logical_ring_init(struct intel_engine_cs *engine) (i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_BUF_LO(engine, 0))); execlists->csb_write = (u32 __force *)execlists->csb_read; + execlists->csb_write_reset = + _MASKED_FIELD(GEN8_CSB_WRITE_PTR_MASK, + GEN8_CSB_ENTRIES - 1); } else { execlists->csb_status = &engine->status_page.page_addr[I915_HWS_CSB_BUF0_INDEX]; execlists->csb_write = &engine->status_page.page_addr[intel_hws_csb_write_index(i915)]; + execlists->csb_write_reset = GEN8_CSB_ENTRIES - 1; } + reset_csb_pointers(execlists); return 0; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 970dbb3c9812..6ec7c019b5d9 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -304,6 +304,15 @@ struct intel_engine_execlists { */ unsigned int csb_head; + /** + * @csb_write_reset: reset value for CSB write pointer + * + * As the CSB write pointer maybe either in HWSP or as a field + * inside an mmio register, we want to reprogram it slightly + * differently to avoid later confusion. + */ + u32 csb_write_reset; + /** * @csb_read: control register for Context Switch buffer *
On HW reset, the HW clears the write pointer (to 0). But since it also writes its first CSB entry to slot 0, we need to reset the write pointer back to the element before (so the first entry we read is 0). This is required for the next patch, where we trust the CSB completely! v2: Use _MASKED_FIELD v3: Store the reset value, so that we differentiate between mmio/hwsp transparently and without pretense. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 23 +++++++++++++++++++++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 9 +++++++++ 2 files changed, 30 insertions(+), 2 deletions(-)