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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 


On 24/09/2019 00:51, John.C.Harrison@xxxxxxxxx wrote:

From: John Harrison <John.C.Harrison@xxxxxxxxx>

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@xxxxxxxxx>
CC: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
CC: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
CC: Chris P Wilson <chris.p.wilson@xxxxxxxxx>
CC: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
---
  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux