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@xxxxxxxxxxxxxxxxxx>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>> ---
> >>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
