| Message ID | 20180627210712.25428-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/06/2018 22:07, 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! > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 368a8c74d11d..8b111a268697 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 = GEN8_CSB_ENTRIES - 1; > + WRITE_ONCE(*execlists->csb_write, (GEN8_CSB_ENTRIES - 1) | 0xff << 16); Use _MASKED_FIELD and GEN8_CSB_WRITE_PTR_MASK? > +} > + > static void execlists_cancel_requests(struct intel_engine_cs *engine) > { > struct intel_engine_execlists * const execlists = &engine->execlists; > @@ -1953,7 +1968,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); > > @@ -2452,7 +2467,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)) { > @@ -2467,6 +2481,7 @@ static int logical_ring_init(struct intel_engine_cs *engine) > execlists->csb_write = > &engine->status_page.page_addr[intel_hws_csb_write_index(i915)]; > } > + reset_csb_pointers(execlists); > > return 0; > > Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-06-28 11:06:25) > > On 27/06/2018 22:07, 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! > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 368a8c74d11d..8b111a268697 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 = GEN8_CSB_ENTRIES - 1; > > + WRITE_ONCE(*execlists->csb_write, (GEN8_CSB_ENTRIES - 1) | 0xff << 16); > > Use _MASKED_FIELD and GEN8_CSB_WRITE_PTR_MASK? Hah, there goes my attempt to kill off unused magic. -Chris
On 28/06/2018 11:17, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-06-28 11:06:25) >> >> On 27/06/2018 22:07, 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! >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++-- >>> 1 file changed, 17 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index 368a8c74d11d..8b111a268697 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 = GEN8_CSB_ENTRIES - 1; >>> + WRITE_ONCE(*execlists->csb_write, (GEN8_CSB_ENTRIES - 1) | 0xff << 16); >> >> Use _MASKED_FIELD and GEN8_CSB_WRITE_PTR_MASK? > > Hah, there goes my attempt to kill off unused magic. At least _MASKED_FIELD makes it clearer. But the u8 trick is still evil since here you even explicitly do a fake masked write on hwsp. Ugly and evil. How about storing execlists->csb_write_default at init time and applying it here? Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-06-28 12:02:30) > > On 28/06/2018 11:17, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-06-28 11:06:25) > >> > >> On 27/06/2018 22:07, 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! > >>> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++-- > >>> 1 file changed, 17 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >>> index 368a8c74d11d..8b111a268697 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 = GEN8_CSB_ENTRIES - 1; > >>> + WRITE_ONCE(*execlists->csb_write, (GEN8_CSB_ENTRIES - 1) | 0xff << 16); > >> > >> Use _MASKED_FIELD and GEN8_CSB_WRITE_PTR_MASK? > > > > Hah, there goes my attempt to kill off unused magic. > > At least _MASKED_FIELD makes it clearer. > > But the u8 trick is still evil since here you even explicitly do a fake > masked write on hwsp. Ugly and evil. How about storing > execlists->csb_write_default at init time and applying it here? Honestly, I thought it made sense next to the READ_ONCE(*csb_write). Could I just convince you with another comment pleading guilty to the atrocities? -Chris
On 28/06/2018 12:30, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-06-28 12:02:30) >> >> On 28/06/2018 11:17, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2018-06-28 11:06:25) >>>> >>>> On 27/06/2018 22:07, 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! >>>>> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++-- >>>>> 1 file changed, 17 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>>>> index 368a8c74d11d..8b111a268697 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 = GEN8_CSB_ENTRIES - 1; >>>>> + WRITE_ONCE(*execlists->csb_write, (GEN8_CSB_ENTRIES - 1) | 0xff << 16); >>>> >>>> Use _MASKED_FIELD and GEN8_CSB_WRITE_PTR_MASK? >>> >>> Hah, there goes my attempt to kill off unused magic. >> >> At least _MASKED_FIELD makes it clearer. >> >> But the u8 trick is still evil since here you even explicitly do a fake >> masked write on hwsp. Ugly and evil. How about storing >> execlists->csb_write_default at init time and applying it here? > > Honestly, I thought it made sense next to the READ_ONCE(*csb_write). > > Could I just convince you with another comment pleading guilty to the > atrocities? I'd prefer we did not write random stuff into hwsp. Like if one day someone is debugging something, they could see either 0xff00?? or 0x?? in there depending on the timings. Then would have to waste time figuring out what's happening. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 368a8c74d11d..8b111a268697 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 = GEN8_CSB_ENTRIES - 1; + WRITE_ONCE(*execlists->csb_write, (GEN8_CSB_ENTRIES - 1) | 0xff << 16); +} + static void execlists_cancel_requests(struct intel_engine_cs *engine) { struct intel_engine_execlists * const execlists = &engine->execlists; @@ -1953,7 +1968,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); @@ -2452,7 +2467,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)) { @@ -2467,6 +2481,7 @@ static int logical_ring_init(struct intel_engine_cs *engine) execlists->csb_write = &engine->status_page.page_addr[intel_hws_csb_write_index(i915)]; } + reset_csb_pointers(execlists); return 0;
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! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)