Re: [PATCH v3] drm/i915/execlists: Reset CSB write pointer after reset

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx