Re: [PATCH 1/3] drm/i915: Prepare for larger CSB status FIFO size

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

On 27/11/2018 09:38, Mika Kuoppala wrote:
> Make csb entry count variable in preparation for larger
> CSB status FIFO size found on gen11+ hardware.

Note that not all registers in the 12-deep CSB fifo are in a contiguous 
range, the new ones (6-11) start at mmio_base + 0x3C0 (Bspec: 11724). If 
we read contiguously we'll access the registers that are in between the 
2 CSB ranges (e.g. RING_CONTEXT_STATUS_PTR). Not a real issue for us 
since we always read from HWSP where the CSBs are contiguous, but we 
need to at least update intel_engine_print_registers(). Also not sure if 
GVT still needs the MMIO mode, if not we could maybe just rip it out 
instead of adapting it.

Daniele

> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c  | 16 +++++++++-------
>   drivers/gpu/drm/i915/intel_lrc.c        | 24 +++++++++++++++++-------
>   drivers/gpu/drm/i915/intel_lrc.h        |  9 +++++----
>   drivers/gpu/drm/i915/intel_ringbuffer.h | 15 +++++++++------
>   4 files changed, 40 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 759c0fd58f8c..218ef817da1d 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1347,27 +1347,29 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
>   		unsigned int idx;
>   		u8 read, write;
>   
> -		drm_printf(m, "\tExeclist status: 0x%08x %08x\n",
> +		drm_printf(m, "\tExeclist status: 0x%08x %08x, entries %u\n",
>   			   I915_READ(RING_EXECLIST_STATUS_LO(engine)),
> -			   I915_READ(RING_EXECLIST_STATUS_HI(engine)));
> +			   I915_READ(RING_EXECLIST_STATUS_HI(engine)),
> +			   execlists->csb_entries);
>   
>   		read = execlists->csb_head;
>   		write = READ_ONCE(*execlists->csb_write);
>   
>   		drm_printf(m, "\tExeclist CSB read %d, write %d [mmio:%d], tasklet queued? %s (%s)\n",
>   			   read, write,
> -			   GEN8_CSB_WRITE_PTR(I915_READ(RING_CONTEXT_STATUS_PTR(engine))),
> +			   I915_READ(RING_CONTEXT_STATUS_PTR(engine)) &
> +			   GEN11_CSB_WRITE_PTR_MASK,
>   			   yesno(test_bit(TASKLET_STATE_SCHED,
>   					  &engine->execlists.tasklet.state)),
>   			   enableddisabled(!atomic_read(&engine->execlists.tasklet.count)));
> -		if (read >= GEN8_CSB_ENTRIES)
> +		if (read >= execlists->csb_entries)
>   			read = 0;
> -		if (write >= GEN8_CSB_ENTRIES)
> +		if (write >= execlists->csb_entries)
>   			write = 0;
>   		if (read > write)
> -			write += GEN8_CSB_ENTRIES;
> +			write += execlists->csb_entries;
>   		while (read < write) {
> -			idx = ++read % GEN8_CSB_ENTRIES;
> +			idx = ++read % execlists->csb_entries;
>   			drm_printf(m, "\tExeclist CSB[%d]: 0x%08x [mmio:0x%08x], context: %d [mmio:%d]\n",
>   				   idx,
>   				   hws[idx * 2],
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 08fd9b12e4d7..5487fe496bb6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -767,6 +767,7 @@ execlists_cancel_port_requests(struct intel_engine_execlists * const execlists)
>   
>   static void reset_csb_pointers(struct intel_engine_execlists *execlists)
>   {
> +	u32 reset_val;
>   	/*
>   	 * After a reset, the HW starts writing into CSB entry [0]. We
>   	 * therefore have to set our HEAD pointer back one entry so that
> @@ -776,8 +777,19 @@ static void reset_csb_pointers(struct intel_engine_execlists *execlists)
>   	 * 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;
> -	WRITE_ONCE(*execlists->csb_write, execlists->csb_write_reset);
> +	execlists->csb_head = execlists->csb_entries - 1;
> +
> +	if (execlists_mmio_mode(execlists)) {
> +		const u32 mask = execlists->csb_entries == GEN8_CSB_ENTRIES ?
> +			GEN8_CSB_WRITE_PTR_MASK :
> +			GEN11_CSB_WRITE_PTR_MASK;
> +
> +		reset_val = _MASKED_FIELD(mask, execlists->csb_head);
> +	} else {
> +		reset_val = execlists->csb_head;
> +	}
> +
> +	WRITE_ONCE(*execlists->csb_write, reset_val);
>   }
>   
>   static void nop_submission_tasklet(unsigned long data)
> @@ -895,7 +907,7 @@ static void process_csb(struct intel_engine_cs *engine)
>   		unsigned int status;
>   		unsigned int count;
>   
> -		if (++head == GEN8_CSB_ENTRIES)
> +		if (++head == execlists->csb_entries)
>   			head = 0;
>   
>   		/*
> @@ -2264,6 +2276,8 @@ static int logical_ring_init(struct intel_engine_cs *engine)
>   			upper_32_bits(ce->lrc_desc);
>   	}
>   
> +	execlists->csb_entries = GEN8_CSB_ENTRIES;
> +
>   	execlists->csb_read =
>   		i915->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine));
>   	if (csb_force_mmio(i915)) {
> @@ -2271,16 +2285,12 @@ 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);
>   
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index f5a5502ecf70..e865cba43a26 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -55,10 +55,11 @@
>   #define GEN8_CSB_PTR_MASK 0x7
>   #define GEN8_CSB_READ_PTR_MASK (GEN8_CSB_PTR_MASK << 8)
>   #define GEN8_CSB_WRITE_PTR_MASK (GEN8_CSB_PTR_MASK << 0)
> -#define GEN8_CSB_WRITE_PTR(csb_status) \
> -	(((csb_status) & GEN8_CSB_WRITE_PTR_MASK) >> 0)
> -#define GEN8_CSB_READ_PTR(csb_status) \
> -	(((csb_status) & GEN8_CSB_READ_PTR_MASK) >> 8)
> +
> +#define GEN11_CSB_ENTRIES 12
> +#define GEN11_CSB_PTR_MASK 0xf
> +#define GEN11_CSB_READ_PTR_MASK (GEN11_CSB_PTR_MASK << 8)
> +#define GEN11_CSB_WRITE_PTR_MASK (GEN11_CSB_PTR_MASK << 0)
>   
>   enum {
>   	INTEL_CONTEXT_SCHEDULE_IN = 0,
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 8a2270b209b0..d9520a52fe29 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -339,13 +339,9 @@ struct intel_engine_execlists {
>   	u32 preempt_complete_status;
>   
>   	/**
> -	 * @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.
> +	 * @csb_entries: number of CSB entries
>   	 */
> -	u32 csb_write_reset;
> +	u8 csb_entries;
>   
>   	/**
>   	 * @csb_head: context status buffer head
> @@ -714,6 +710,13 @@ execlists_is_active(const struct intel_engine_execlists *execlists,
>   	return test_bit(bit, (unsigned long *)&execlists->active);
>   }
>   
> +static inline bool
> +execlists_mmio_mode(const struct intel_engine_execlists * const execlists)
> +{
> +	return (unsigned long)execlists->csb_read ==
> +		(unsigned long)execlists->csb_write;
> +}
> +
>   void execlists_user_begin(struct intel_engine_execlists *execlists,
>   			  const struct execlist_port *port);
>   void execlists_user_end(struct intel_engine_execlists *execlists);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx