Re: [PATCH 1/2] drm/i915: Engine relative MMIO

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

On 24/09/2019 00:51, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> With virtual engines, it is no longer possible to know which specific
> physical engine a given request will be executed on at the time that
> request is generated. This means that the request itself must be engine
> agnostic - any direct register writes must be relative to the engine
> and not absolute addresses.
> 
> The LRI command has support for engine relative addressing. However,
> the mechanism is not transparent to the driver. The scheme for Gen11
> (MI_LRI_ADD_CS_MMIO_START) requires the LRI address to have no
> absolute engine base component. The hardware then adds on the correct
> engine offset at execution time.
> 
> Due to the non-trivial and differing schemes on different hardware, it
> is not possible to simply update the code that creates the LRI
> commands to set a remap flag and let the hardware get on with it.
> Instead, this patch adds function wrappers for generating the LRI
> command itself and then for constructing the correct address to use
> with the LRI.
> 
> v2: Fix build break in GVT. Remove flags parameter [review feedback
> from Chris W].
> 
> v3: Fix build break in selftest. Rebase to newer base tree and fix
> merge conflict.
> 
> v4: More rebasing. Rmoved relative addressing support from Gen7-9 only
> code paths [review feedback from Chris W].
> 
> v5: More rebasing (new 'gt' directory). Fix white space issue. Use
> COPY class rather than BCS0 id for checking against BCS engine.
> 
> v6: Change to MI_LRI for generic code and __MI_LRI for legacy code.
> MI_LRI is now a macro wrapper around an engine function pointer to
> reduce runtime checks on LRI relative support. [review feedback from
> Tvrtko]
> 
> v7: Replace engine function pointer with a per engine flags field that
> is added by a common function. [Daniele]
> 
> v8: Re-write on top of patch series by Mika.
> 
> v9: Fix workaround IGT failure. Not sure why but my local test runs
> were passing by claiming no workarounds whereas the pre-commit check
> failed. As all the workaround registers are currently engine
> independent, it is safe to simply not add the engine relative flag
> (which is what the selftest was expecting).
> 
> Bspec: 45606
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> CC: Chris P Wilson <chris.p.wilson@intel.com>
> CC: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c  |  7 ++--
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 24 ++++++++++++
>   drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 ++
>   drivers/gpu/drm/i915/gt/intel_gpu_commands.h |  7 +++-
>   drivers/gpu/drm/i915/gt/intel_lrc.c          | 40 ++++++++++----------
>   drivers/gpu/drm/i915/gt/intel_mocs.c         |  6 +--
>   drivers/gpu/drm/i915/gt/intel_ringbuffer.c   | 12 +++---
>   drivers/gpu/drm/i915/i915_perf.c             |  8 +++-
>   8 files changed, 73 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 4a34c4f62065..c8d7075a481d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -980,7 +980,7 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>   {
>   	struct i915_address_space *vm = rq->hw_context->vm;
>   	struct intel_engine_cs *engine = rq->engine;
> -	u32 base = engine->mmio_base;
> +	u32 base = engine->lri_mmio_base;
>   	u32 *cs;
>   	int i;
>   
> @@ -992,7 +992,7 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>   		if (IS_ERR(cs))
>   			return PTR_ERR(cs);
>   
> -		*cs++ = MI_LOAD_REGISTER_IMM(2);
> +		*cs++ = MI_LOAD_REGISTER_IMM(2) | engine->lri_cmd_mode;

I missed the part where we ended up with this new flavour and I can't 
say I am a fan.

What's the point in having to remember to or-in the flags at so many 
call sites? Are you now touching all mi_lri, or most, or how many? If 
all or most then why not MI_LOAD_REGISTER_IMM(engine, 2)? If not most 
then the same with _REL suffix as discussed before and leave the legacy 
untouched.

Don't know really on all the details since I lost track, but the current 
option leaves me concerned.

Regards,

Tvrtko

>   
>   		*cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_UDW(base, 0));
>   		*cs++ = upper_32_bits(pd_daddr);
> @@ -1014,7 +1014,8 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>   		if (IS_ERR(cs))
>   			return PTR_ERR(cs);
>   
> -		*cs++ = MI_LOAD_REGISTER_IMM(2 * GEN8_3LVL_PDPES) | MI_LRI_FORCE_POSTED;
> +		*cs++ = MI_LOAD_REGISTER_IMM(2 * GEN8_3LVL_PDPES) |
> +			MI_LRI_FORCE_POSTED | engine->lri_cmd_mode;
>   		for (i = GEN8_3LVL_PDPES; i--; ) {
>   			const dma_addr_t pd_daddr = i915_page_dir_dma_addr(ppgtt, i);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index f451d5076bde..5aa58e069806 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -236,6 +236,28 @@ static u32 __engine_mmio_base(struct drm_i915_private *i915,
>   	return bases[i].base;
>   }
>   
> +static bool i915_engine_has_relative_lri(const struct intel_engine_cs *engine)
> +{
> +	if (INTEL_GEN(engine->i915) < 11)
> +		return false;
> +
> +	if (engine->class == COPY_ENGINE_CLASS)
> +		return false;
> +
> +	return true;
> +}
> +
> +static void lri_init(struct intel_engine_cs *engine)
> +{
> +	if (i915_engine_has_relative_lri(engine)) {
> +		engine->lri_cmd_mode = MI_LRI_ADD_CS_MMIO_START_GEN11;
> +		engine->lri_mmio_base = 0;
> +	} else {
> +		engine->lri_cmd_mode = 0;
> +		engine->lri_mmio_base = engine->mmio_base;
> +	}
> +}
> +
>   static void __sprint_engine_name(struct intel_engine_cs *engine)
>   {
>   	/*
> @@ -320,6 +342,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
>   	/* Nothing to do here, execute in order of dependencies */
>   	engine->schedule = NULL;
>   
> +	lri_init(engine);
> +
>   	seqlock_init(&engine->stats.lock);
>   
>   	ATOMIC_INIT_NOTIFIER_HEAD(&engine->context_status_notifier);
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 943f0663837e..38f486f7f7e3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -306,6 +306,9 @@ struct intel_engine_cs {
>   	u32 context_size;
>   	u32 mmio_base;
>   
> +	u32 lri_mmio_base;
> +	u32 lri_cmd_mode;
> +
>   	u32 uabi_capabilities;
>   
>   	struct rb_node uabi_node;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> index f78b13d74e17..bfb51d8d7ce2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> @@ -133,11 +133,16 @@
>    *   simply ignores the register load under certain conditions.
>    * - One can actually load arbitrary many arbitrary registers: Simply issue x
>    *   address/value pairs. Don't overdue it, though, x <= 2^4 must hold!
> + * - Newer hardware supports engine relative addressing but older hardware does
> + *   not. This is required for hw engine load balancing. Hence the MI_LRI
> + *   instruction itself is prefixed with '__' and should only be used on
> + *   legacy hardware code paths. Generic code must always use the MI_LRI
> + *   and i915_get_lri_reg() helper functions instead.
>    */
>   #define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
>   /* Gen11+. addr = base + (ctx_restore ? offset & GENMASK(12,2) : offset) */
> -#define   MI_LRI_CS_MMIO		(1<<19)
>   #define   MI_LRI_FORCE_POSTED		(1<<12)
> +#define   MI_LRI_ADD_CS_MMIO_START_GEN11	BIT(19)
>   #define MI_STORE_REGISTER_MEM        MI_INSTR(0x24, 1)
>   #define MI_STORE_REGISTER_MEM_GEN8   MI_INSTR(0x24, 2)
>   #define   MI_SRM_LRM_GLOBAL_GTT		(1<<22)
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 6cfdc0f9f2b9..7cd59d29c4e4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2010,11 +2010,12 @@ struct lri {
>   	u32 value;
>   };
>   
> -static u32 *emit_lri(u32 *batch, const struct lri *lri, unsigned int count)
> +static u32 *emit_lri(struct intel_engine_cs *engine, u32 *batch,
> +		     const struct lri *lri, unsigned int count)
>   {
>   	GEM_BUG_ON(!count || count > 63);
>   
> -	*batch++ = MI_LOAD_REGISTER_IMM(count);
> +	*batch++ = MI_LOAD_REGISTER_IMM(count) | engine->lri_cmd_mode;
>   	do {
>   		*batch++ = i915_mmio_reg_offset(lri->reg);
>   		*batch++ = lri->value;
> @@ -2054,7 +2055,7 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
>   	/* WaFlushCoherentL3CacheLinesAtContextSwitch:skl,bxt,glk */
>   	batch = gen8_emit_flush_coherentl3_wa(engine, batch);
>   
> -	batch = emit_lri(batch, lri, ARRAY_SIZE(lri));
> +	batch = emit_lri(engine, batch, lri, ARRAY_SIZE(lri));
>   
>   	/* WaMediaPoolStateCmdInWABB:bxt,glk */
>   	if (HAS_POOLED_EU(engine->i915)) {
> @@ -3282,7 +3283,7 @@ static void init_common_reg_state(u32 * const regs,
>   				  struct intel_engine_cs *engine,
>   				  struct intel_ring *ring)
>   {
> -	const u32 base = engine->mmio_base;
> +	const u32 base = engine->lri_mmio_base;
>   
>   	CTX_REG(regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(base),
>   		_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT) |
> @@ -3307,7 +3308,7 @@ static void init_wa_bb_reg_state(u32 * const regs,
>   				 u32 pos_bb_per_ctx)
>   {
>   	struct i915_ctx_workarounds * const wa_ctx = &engine->wa_ctx;
> -	const u32 base = engine->mmio_base;
> +	const u32 base = engine->lri_mmio_base;
>   	const u32 pos_indirect_ctx = pos_bb_per_ctx + 2;
>   	const u32 pos_indirect_ctx_offset = pos_indirect_ctx + 2;
>   
> @@ -3335,6 +3336,7 @@ static void init_wa_bb_reg_state(u32 * const regs,
>   }
>   
>   static void init_ppgtt_reg_state(u32 *regs, u32 base,
> +				 struct intel_engine_cs *engine,
>   				 struct i915_ppgtt *ppgtt)
>   {
>   	/* PDP values well be assigned later if needed */
> @@ -3376,14 +3378,12 @@ static void gen8_init_reg_state(u32 * const regs,
>   {
>   	struct i915_ppgtt * const ppgtt = vm_alias(ce->vm);
>   	const bool rcs = engine->class == RENDER_CLASS;
> -	const u32 base = engine->mmio_base;
> -	const u32 lri_base =
> -		intel_engine_has_relative_mmio(engine) ? MI_LRI_CS_MMIO : 0;
> +	const u32 base = engine->lri_mmio_base;
>   
>   	regs[CTX_LRI_HEADER_0] =
>   		MI_LOAD_REGISTER_IMM(rcs ? 14 : 11) |
>   		MI_LRI_FORCE_POSTED |
> -		lri_base;
> +		engine->lri_cmd_mode;
>   
>   	init_common_reg_state(regs, ppgtt, engine, ring);
>   	CTX_REG(regs, CTX_SECOND_BB_HEAD_U, RING_SBBADDR_UDW(base), 0);
> @@ -3395,15 +3395,17 @@ static void gen8_init_reg_state(u32 * const regs,
>   	regs[CTX_LRI_HEADER_1] =
>   		MI_LOAD_REGISTER_IMM(9) |
>   		MI_LRI_FORCE_POSTED |
> -		lri_base;
> +		engine->lri_cmd_mode;
>   
>   	CTX_REG(regs, CTX_CTX_TIMESTAMP, RING_CTX_TIMESTAMP(base), 0);
>   
> -	init_ppgtt_reg_state(regs, base, ppgtt);
> +	init_ppgtt_reg_state(regs, base, engine, ppgtt);
>   
>   	if (rcs) {
> -		regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1) | lri_base;
> -		CTX_REG(regs, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE, 0);
> +		regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1) |
> +					 engine->lri_cmd_mode;
> +		CTX_REG(regs, CTX_R_PWR_CLK_STATE,
> +			GEN8_R_PWR_CLK_STATE, 0);
>   	}
>   
>   	regs[CTX_END] = MI_BATCH_BUFFER_END;
> @@ -3418,14 +3420,12 @@ static void gen12_init_reg_state(u32 * const regs,
>   {
>   	struct i915_ppgtt * const ppgtt = i915_vm_to_ppgtt(ce->vm);
>   	const bool rcs = engine->class == RENDER_CLASS;
> -	const u32 base = engine->mmio_base;
> -	const u32 lri_base =
> -		intel_engine_has_relative_mmio(engine) ? MI_LRI_CS_MMIO : 0;
> +	const u32 base = engine->lri_mmio_base;
>   
>   	regs[CTX_LRI_HEADER_0] =
>   		MI_LOAD_REGISTER_IMM(rcs ? 11 : 9) |
>   		MI_LRI_FORCE_POSTED |
> -		lri_base;
> +		engine->lri_cmd_mode;
>   
>   	init_common_reg_state(regs, ppgtt, engine, ring);
>   
> @@ -3435,15 +3435,15 @@ static void gen12_init_reg_state(u32 * const regs,
>   	regs[CTX_LRI_HEADER_1] =
>   		MI_LOAD_REGISTER_IMM(9) |
>   		MI_LRI_FORCE_POSTED |
> -		lri_base;
> +		engine->lri_cmd_mode;
>   
>   	CTX_REG(regs, CTX_CTX_TIMESTAMP, RING_CTX_TIMESTAMP(base), 0);
>   
> -	init_ppgtt_reg_state(regs, base, ppgtt);
> +	init_ppgtt_reg_state(regs, base, engine, ppgtt);
>   
>   	if (rcs) {
>   		regs[GEN12_CTX_LRI_HEADER_3] =
> -			MI_LOAD_REGISTER_IMM(1) | lri_base;
> +			MI_LOAD_REGISTER_IMM(1) | engine->lri_cmd_mode;
>   		CTX_REG(regs, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE, 0);
>   
>   		/* TODO: oa_init_reg_state ? */
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index 728704bbbe18..8e8fe3deb95c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -368,9 +368,6 @@ static u32 get_entry_control(const struct drm_i915_mocs_table *table,
>   /**
>    * intel_mocs_init_engine() - emit the mocs control table
>    * @engine:	The engine for whom to emit the registers.
> - *
> - * This function simply emits a MI_LOAD_REGISTER_IMM command for the
> - * given table starting at the given address.
>    */
>   void intel_mocs_init_engine(struct intel_engine_cs *engine)
>   {
> @@ -456,7 +453,8 @@ static int emit_mocs_control_table(struct i915_request *rq,
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(table->n_entries);
> +	*cs++ = MI_LOAD_REGISTER_IMM(table->n_entries) |
> +		rq->engine->lri_cmd_mode;
>   
>   	for (index = 0; index < table->size; index++) {
>   		u32 value = get_entry_control(table, index);
> diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> index 0747b8c9f768..7027367b1f1a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> @@ -1536,12 +1536,13 @@ static int load_pd_dir(struct i915_request *rq, const struct i915_ppgtt *ppgtt)
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(1);
> -	*cs++ = i915_mmio_reg_offset(RING_PP_DIR_DCLV(engine->mmio_base));
> +	/* Can these not be merged into a single LRI??? */
> +	*cs++ = MI_LOAD_REGISTER_IMM(1) | engine->lri_cmd_mode;
> +	*cs++ = i915_mmio_reg_offset(RING_PP_DIR_DCLV(engine->lri_mmio_base));
>   	*cs++ = PP_DIR_DCLV_2G;
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(1);
> -	*cs++ = i915_mmio_reg_offset(RING_PP_DIR_BASE(engine->mmio_base));
> +	*cs++ = MI_LOAD_REGISTER_IMM(1) | engine->lri_cmd_mode;
> +	*cs++ = i915_mmio_reg_offset(RING_PP_DIR_BASE(engine->lri_mmio_base));
>   	*cs++ = px_base(ppgtt->pd)->ggtt_offset << 10;
>   
>   	intel_ring_advance(rq, cs);
> @@ -1709,7 +1710,8 @@ static int remap_l3_slice(struct i915_request *rq, int slice)
>   	 * here because no other code should access these registers other than
>   	 * at initialization time.
>   	 */
> -	*cs++ = MI_LOAD_REGISTER_IMM(GEN7_L3LOG_SIZE/4);
> +	*cs++ = MI_LOAD_REGISTER_IMM(GEN7_L3LOG_SIZE / 4) |
> +		rq->engine->lri_cmd_mode;
>   	for (i = 0; i < GEN7_L3LOG_SIZE/4; i++) {
>   		*cs++ = i915_mmio_reg_offset(GEN7_L3LOG(slice, i));
>   		*cs++ = remap_info[i];
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c1b764233761..8b85cdfada21 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1693,6 +1693,11 @@ gen8_update_reg_state_unlocked(struct i915_perf_stream *stream,
>   	};
>   	int i;
>   
> +	/*
> +	 * NB: The LRI instruction is generated by the hardware.
> +	 * Should we read it in and assert that the offset flag is set?
> +	 */
> +
>   	CTX_REG(reg_state, ctx_oactxctrl, GEN8_OACTXCONTROL,
>   		(stream->period_exponent << GEN8_OA_TIMER_PERIOD_SHIFT) |
>   		(stream->periodic ? GEN8_OA_TIMER_ENABLE : 0) |
> @@ -1752,8 +1757,9 @@ gen8_load_flex(struct i915_request *rq,
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(count);
> +	*cs++ = MI_LOAD_REGISTER_IMM(count) | rq->engine->lri_cmd_mode;
>   	do {
> +		/* FIXME: Is this table LRI remap/offset friendly? */
>   		*cs++ = i915_mmio_reg_offset(flex->reg);
>   		*cs++ = flex->value;
>   	} while (flex++, --count);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx