* [PATCH 0/2] drm/i915: Engine relative MMIO
@ 2019-08-22 18:02 John.C.Harrison
2019-08-22 18:02 ` [PATCH 1/2] " John.C.Harrison
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: John.C.Harrison @ 2019-08-22 18:02 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
Newer hardware has support for making LRI accesses to MMIO registers
relative to the engine that is executing the LRI instruction. This is
required for things like hardware based load balancing across engines.
John Harrison (2):
drm/i915: Engine relative MMIO
drm/i915: Engine relative MMIO for Gen12
drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +-
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 +-
drivers/gpu/drm/i915/gt/intel_engine.h | 5 +
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 219 ++++++++++++++++++
drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +
drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 9 +-
drivers/gpu/drm/i915/gt/intel_lrc.c | 77 +++---
drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
drivers/gpu/drm/i915/gt/intel_mocs.c | 17 +-
drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 25 +-
drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 +-
.../gpu/drm/i915/gt/selftest_workarounds.c | 15 +-
drivers/gpu/drm/i915/gvt/mmio_context.c | 16 +-
drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +-
drivers/gpu/drm/i915/i915_perf.c | 17 +-
15 files changed, 361 insertions(+), 80 deletions(-)
--
2.21.0.5.gaeb582a983
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] drm/i915: Engine relative MMIO
2019-08-22 18:02 [PATCH 0/2] drm/i915: Engine relative MMIO John.C.Harrison
@ 2019-08-22 18:02 ` John.C.Harrison
2019-08-22 18:02 ` [PATCH 2/2] drm/i915: Engine relative MMIO for Gen12 John.C.Harrison
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: John.C.Harrison @ 2019-08-22 18:02 UTC (permalink / raw)
To: Intel-GFX; +Cc: Chris P Wilson
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]
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 | 14 ++--
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 +-
drivers/gpu/drm/i915/gt/intel_engine.h | 5 ++
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 42 ++++++++++
drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 +
drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 8 +-
drivers/gpu/drm/i915/gt/intel_lrc.c | 77 ++++++++++---------
drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
drivers/gpu/drm/i915/gt/intel_mocs.c | 17 ++--
drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 25 ++++--
drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 +-
.../gpu/drm/i915/gt/selftest_workarounds.c | 15 ++--
drivers/gpu/drm/i915/gvt/mmio_context.c | 16 +++-
drivers/gpu/drm/i915/i915_cmd_parser.c | 6 +-
drivers/gpu/drm/i915/i915_perf.c | 16 ++--
15 files changed, 175 insertions(+), 80 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 1cdfe05514c3..b8f23148ac80 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -992,11 +992,11 @@ 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(engine, 2);
- *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_UDW(base, 0));
+ *cs++ = i915_get_lri_reg(engine, GEN8_RING_PDP_UDW(base, 0));
*cs++ = upper_32_bits(pd_daddr);
- *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_LDW(base, 0));
+ *cs++ = i915_get_lri_reg(engine, GEN8_RING_PDP_LDW(base, 0));
*cs++ = lower_32_bits(pd_daddr);
*cs++ = MI_NOOP;
@@ -1008,13 +1008,15 @@ 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);
+ *cs++ = MI_LOAD_REGISTER_IMM(engine, 2 * GEN8_3LVL_PDPES);
for (i = GEN8_3LVL_PDPES; i--; ) {
const dma_addr_t pd_daddr = i915_page_dir_dma_addr(ppgtt, i);
- *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_UDW(base, i));
+ *cs++ = i915_get_lri_reg(engine,
+ GEN8_RING_PDP_UDW(base, i));
*cs++ = upper_32_bits(pd_daddr);
- *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_LDW(base, i));
+ *cs++ = i915_get_lri_reg(engine,
+ GEN8_RING_PDP_LDW(base, i));
*cs++ = lower_32_bits(pd_daddr);
}
*cs++ = MI_NOOP;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 2dca2962c73a..13bad7806f52 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1947,7 +1947,8 @@ static int i915_reset_gen7_sol_offsets(struct i915_request *rq)
if (IS_ERR(cs))
return PTR_ERR(cs);
- *cs++ = MI_LOAD_REGISTER_IMM(4);
+ /* Gen7 only so no need to support relative offsets */
+ *cs++ = __MI_LOAD_REGISTER_IMM(4);
for (i = 0; i < 4; i++) {
*cs++ = i915_mmio_reg_offset(GEN7_SO_WRITE_OFFSET(i));
*cs++ = 0;
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index d3c6993f4f46..09d7208b0127 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -511,4 +511,9 @@ void intel_engine_init_active(struct intel_engine_cs *engine,
#define ENGINE_MOCK 1
#define ENGINE_VIRTUAL 2
+#define MI_LOAD_REGISTER_IMM(engine, count) \
+ (engine)->get_lri_cmd((engine), (count))
+
+u32 i915_get_lri_reg(const struct intel_engine_cs *engine, i915_reg_t reg);
+
#endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 82630db0394b..efe1c377d797 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -236,6 +236,46 @@ static u32 __engine_mmio_base(struct drm_i915_private *i915,
return bases[i].base;
}
+static u32 i915_get_lri_cmd_legacy(const struct intel_engine_cs *engine,
+ u32 word_count)
+{
+ return __MI_LOAD_REGISTER_IMM(word_count);
+}
+
+static u32 i915_get_lri_cmd_add_offset(const struct intel_engine_cs *engine,
+ u32 word_count)
+{
+ return __MI_LOAD_REGISTER_IMM(word_count) |
+ MI_LRI_ADD_CS_MMIO_START_GEN11;
+}
+
+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->get_lri_cmd = i915_get_lri_cmd_add_offset;
+ else
+ engine->get_lri_cmd = i915_get_lri_cmd_legacy;
+}
+
+u32 i915_get_lri_reg(const struct intel_engine_cs *engine, i915_reg_t reg)
+{
+ if (!i915_engine_has_relative_lri(engine))
+ return i915_mmio_reg_offset(reg);
+
+ return i915_mmio_reg_offset(reg) - engine->mmio_base;
+}
+
static void __sprint_engine_name(struct intel_engine_cs *engine)
{
/*
@@ -320,6 +360,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 a82cea95c2f2..7ca6c86a33f6 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 (*get_lri_cmd)(const struct intel_engine_cs *engine,
+ u32 word_count);
+
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 86e00a2db8a4..eaa019df0ce7 100644
--- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
+++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
@@ -130,9 +130,15 @@
* 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)
+#define __MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
#define MI_LRI_FORCE_POSTED (1<<12)
+#define MI_LRI_ADD_CS_MMIO_START_GEN11 (1<<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 d42584439f51..e154786814bc 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1902,14 +1902,15 @@ static int emit_pdps(struct i915_request *rq)
return PTR_ERR(cs);
/* Ensure the LRI have landed before we invalidate & continue */
- *cs++ = MI_LOAD_REGISTER_IMM(2 * GEN8_3LVL_PDPES) | MI_LRI_FORCE_POSTED;
+ *cs++ = MI_LOAD_REGISTER_IMM(engine, 2 * GEN8_3LVL_PDPES) |
+ MI_LRI_FORCE_POSTED;
for (i = GEN8_3LVL_PDPES; i--; ) {
const dma_addr_t pd_daddr = i915_page_dir_dma_addr(ppgtt, i);
u32 base = engine->mmio_base;
- *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_UDW(base, i));
+ *cs++ = i915_get_lri_reg(engine, GEN8_RING_PDP_UDW(base, i));
*cs++ = upper_32_bits(pd_daddr);
- *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_LDW(base, i));
+ *cs++ = i915_get_lri_reg(engine, GEN8_RING_PDP_LDW(base, i));
*cs++ = lower_32_bits(pd_daddr);
}
*cs++ = MI_NOOP;
@@ -1984,7 +1985,8 @@ gen8_emit_flush_coherentl3_wa(struct intel_engine_cs *engine, u32 *batch)
INTEL_GT_SCRATCH_FIELD_COHERENTL3_WA);
*batch++ = 0;
- *batch++ = MI_LOAD_REGISTER_IMM(1);
+ /* Gen8/9 only so no need to support relative offsets */
+ *batch++ = __MI_LOAD_REGISTER_IMM(1);
*batch++ = i915_mmio_reg_offset(GEN8_L3SQCREG4);
*batch++ = 0x40400000 | GEN8_LQSC_FLUSH_COHERENT_LINES;
@@ -2061,13 +2063,14 @@ 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(engine, count);
do {
- *batch++ = i915_mmio_reg_offset(lri->reg);
+ *batch++ = i915_get_lri_reg(engine, lri->reg);
*batch++ = lri->value;
} while (lri++, --count);
*batch++ = MI_NOOP;
@@ -2105,7 +2108,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)) {
@@ -3168,10 +3171,10 @@ static void execlists_init_reg_state(u32 *regs,
*
* Must keep consistent with virtual_update_register_offsets().
*/
- regs[CTX_LRI_HEADER_0] = MI_LOAD_REGISTER_IMM(rcs ? 14 : 11) |
+ regs[CTX_LRI_HEADER_0] = MI_LOAD_REGISTER_IMM(engine, rcs ? 14 : 11) |
MI_LRI_FORCE_POSTED;
- CTX_REG(regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(base),
+ CTX_REG(engine, regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(base),
_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT) |
_MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH));
if (INTEL_GEN(engine->i915) < 11) {
@@ -3179,22 +3182,23 @@ static void execlists_init_reg_state(u32 *regs,
_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT |
CTX_CTRL_RS_CTX_ENABLE);
}
- CTX_REG(regs, CTX_RING_HEAD, RING_HEAD(base), 0);
- CTX_REG(regs, CTX_RING_TAIL, RING_TAIL(base), 0);
- CTX_REG(regs, CTX_RING_BUFFER_START, RING_START(base), 0);
- CTX_REG(regs, CTX_RING_BUFFER_CONTROL, RING_CTL(base),
+ CTX_REG(engine, regs, CTX_RING_HEAD, RING_HEAD(base), 0);
+ CTX_REG(engine, regs, CTX_RING_TAIL, RING_TAIL(base), 0);
+ CTX_REG(engine, regs, CTX_RING_BUFFER_START, RING_START(base), 0);
+ CTX_REG(engine, regs, CTX_RING_BUFFER_CONTROL, RING_CTL(base),
RING_CTL_SIZE(ring->size) | RING_VALID);
- CTX_REG(regs, CTX_BB_HEAD_U, RING_BBADDR_UDW(base), 0);
- CTX_REG(regs, CTX_BB_HEAD_L, RING_BBADDR(base), 0);
- CTX_REG(regs, CTX_BB_STATE, RING_BBSTATE(base), RING_BB_PPGTT);
- CTX_REG(regs, CTX_SECOND_BB_HEAD_U, RING_SBBADDR_UDW(base), 0);
- CTX_REG(regs, CTX_SECOND_BB_HEAD_L, RING_SBBADDR(base), 0);
- CTX_REG(regs, CTX_SECOND_BB_STATE, RING_SBBSTATE(base), 0);
+ CTX_REG(engine, regs, CTX_BB_HEAD_U, RING_BBADDR_UDW(base), 0);
+ CTX_REG(engine, regs, CTX_BB_HEAD_L, RING_BBADDR(base), 0);
+ CTX_REG(engine, regs, CTX_BB_STATE, RING_BBSTATE(base), RING_BB_PPGTT);
+ CTX_REG(engine, regs, CTX_SECOND_BB_HEAD_U, RING_SBBADDR_UDW(base), 0);
+ CTX_REG(engine, regs, CTX_SECOND_BB_HEAD_L, RING_SBBADDR(base), 0);
+ CTX_REG(engine, regs, CTX_SECOND_BB_STATE, RING_SBBSTATE(base), 0);
if (rcs) {
struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
- CTX_REG(regs, CTX_RCS_INDIRECT_CTX, RING_INDIRECT_CTX(base), 0);
- CTX_REG(regs, CTX_RCS_INDIRECT_CTX_OFFSET,
+ CTX_REG(engine, regs, CTX_RCS_INDIRECT_CTX,
+ RING_INDIRECT_CTX(base), 0);
+ CTX_REG(engine, regs, CTX_RCS_INDIRECT_CTX_OFFSET,
RING_INDIRECT_CTX_OFFSET(base), 0);
if (wa_ctx->indirect_ctx.size) {
u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
@@ -3207,7 +3211,8 @@ static void execlists_init_reg_state(u32 *regs,
intel_lr_indirect_ctx_offset(engine) << 6;
}
- CTX_REG(regs, CTX_BB_PER_CTX_PTR, RING_BB_PER_CTX_PTR(base), 0);
+ CTX_REG(engine, regs, CTX_BB_PER_CTX_PTR,
+ RING_BB_PER_CTX_PTR(base), 0);
if (wa_ctx->per_ctx.size) {
u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
@@ -3216,18 +3221,19 @@ static void execlists_init_reg_state(u32 *regs,
}
}
- regs[CTX_LRI_HEADER_1] = MI_LOAD_REGISTER_IMM(9) | MI_LRI_FORCE_POSTED;
+ regs[CTX_LRI_HEADER_1] = MI_LOAD_REGISTER_IMM(engine, 9) |
+ MI_LRI_FORCE_POSTED;
- CTX_REG(regs, CTX_CTX_TIMESTAMP, RING_CTX_TIMESTAMP(base), 0);
+ CTX_REG(engine, regs, CTX_CTX_TIMESTAMP, RING_CTX_TIMESTAMP(base), 0);
/* PDP values well be assigned later if needed */
- CTX_REG(regs, CTX_PDP3_UDW, GEN8_RING_PDP_UDW(base, 3), 0);
- CTX_REG(regs, CTX_PDP3_LDW, GEN8_RING_PDP_LDW(base, 3), 0);
- CTX_REG(regs, CTX_PDP2_UDW, GEN8_RING_PDP_UDW(base, 2), 0);
- CTX_REG(regs, CTX_PDP2_LDW, GEN8_RING_PDP_LDW(base, 2), 0);
- CTX_REG(regs, CTX_PDP1_UDW, GEN8_RING_PDP_UDW(base, 1), 0);
- CTX_REG(regs, CTX_PDP1_LDW, GEN8_RING_PDP_LDW(base, 1), 0);
- CTX_REG(regs, CTX_PDP0_UDW, GEN8_RING_PDP_UDW(base, 0), 0);
- CTX_REG(regs, CTX_PDP0_LDW, GEN8_RING_PDP_LDW(base, 0), 0);
+ CTX_REG(engine, regs, CTX_PDP3_UDW, GEN8_RING_PDP_UDW(base, 3), 0);
+ CTX_REG(engine, regs, CTX_PDP3_LDW, GEN8_RING_PDP_LDW(base, 3), 0);
+ CTX_REG(engine, regs, CTX_PDP2_UDW, GEN8_RING_PDP_UDW(base, 2), 0);
+ CTX_REG(engine, regs, CTX_PDP2_LDW, GEN8_RING_PDP_LDW(base, 2), 0);
+ CTX_REG(engine, regs, CTX_PDP1_UDW, GEN8_RING_PDP_UDW(base, 1), 0);
+ CTX_REG(engine, regs, CTX_PDP1_LDW, GEN8_RING_PDP_LDW(base, 1), 0);
+ CTX_REG(engine, regs, CTX_PDP0_UDW, GEN8_RING_PDP_UDW(base, 0), 0);
+ CTX_REG(engine, regs, CTX_PDP0_LDW, GEN8_RING_PDP_LDW(base, 0), 0);
if (i915_vm_is_4lvl(&ppgtt->vm)) {
/* 64b PPGTT (48bit canonical)
@@ -3243,8 +3249,9 @@ static void execlists_init_reg_state(u32 *regs,
}
if (rcs) {
- regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1);
- CTX_REG(regs, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE, 0);
+ regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(engine, 1);
+ CTX_REG(engine, regs, CTX_R_PWR_CLK_STATE,
+ GEN8_R_PWR_CLK_STATE, 0);
}
regs[CTX_END] = MI_BATCH_BUFFER_END;
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
index b8f20ad71169..e56a27697b6f 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
@@ -39,10 +39,10 @@
#define CTX_R_PWR_CLK_STATE 0x42
#define CTX_END 0x44
-#define CTX_REG(reg_state, pos, reg, val) do { \
+#define CTX_REG(engine, reg_state, pos, reg, val) do { \
u32 *reg_state__ = (reg_state); \
const u32 pos__ = (pos); \
- (reg_state__)[(pos__) + 0] = i915_mmio_reg_offset(reg); \
+ (reg_state__)[(pos__) + 0] = i915_get_lri_reg((engine), (reg)); \
(reg_state__)[(pos__) + 1] = (val); \
} while (0)
diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c b/drivers/gpu/drm/i915/gt/intel_mocs.c
index 728704bbbe18..ba29a0bdae28 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,18 +453,20 @@ 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(rq->engine, table->n_entries);
for (index = 0; index < table->size; index++) {
u32 value = get_entry_control(table, index);
- *cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
+ *cs++ = i915_get_lri_reg(rq->engine,
+ mocs_register(engine, index));
*cs++ = value;
}
/* All remaining entries are also unused */
for (; index < table->n_entries; index++) {
- *cs++ = i915_mmio_reg_offset(mocs_register(engine, index));
+ *cs++ = i915_get_lri_reg(rq->engine,
+ mocs_register(engine, index));
*cs++ = unused_value;
}
@@ -514,7 +513,11 @@ static int emit_mocs_l3cc_table(struct i915_request *rq,
if (IS_ERR(cs))
return PTR_ERR(cs);
- *cs++ = MI_LOAD_REGISTER_IMM(table->n_entries / 2);
+ /*
+ * GEN9_LNCFCMOCS is not engine relative, therefore there is no
+ * need for relative addressing?
+ */
+ *cs++ = __MI_LOAD_REGISTER_IMM(table->n_entries / 2);
for (i = 0; i < table->size / 2; i++) {
u16 low = get_entry_l3cc(table, 2 * i);
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index 601c16239fdf..58997953ea9a 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -1534,12 +1534,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(engine, 1);
+ *cs++ = i915_get_lri_reg(engine, RING_PP_DIR_DCLV(engine->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(engine, 1);
+ *cs++ = i915_get_lri_reg(engine, RING_PP_DIR_BASE(engine->mmio_base));
*cs++ = px_base(ppgtt->pd)->ggtt_offset << 10;
intel_ring_advance(rq, cs);
@@ -1608,7 +1609,11 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
if (num_engines) {
struct intel_engine_cs *signaller;
- *cs++ = MI_LOAD_REGISTER_IMM(num_engines);
+ /*
+ * Must use absolute engine address as the register
+ * write is targeting a different engine.
+ */
+ *cs++ = __MI_LOAD_REGISTER_IMM(num_engines);
for_each_engine(signaller, i915, id) {
if (signaller == engine)
continue;
@@ -1662,7 +1667,11 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
struct intel_engine_cs *signaller;
i915_reg_t last_reg = {}; /* keep gcc quiet */
- *cs++ = MI_LOAD_REGISTER_IMM(num_engines);
+ /*
+ * Must use absolute engine address as the register
+ * write is targeting a different engine.
+ */
+ *cs++ = __MI_LOAD_REGISTER_IMM(num_engines);
for_each_engine(signaller, i915, id) {
if (signaller == engine)
continue;
@@ -1707,9 +1716,9 @@ 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(rq->engine, GEN7_L3LOG_SIZE/4);
for (i = 0; i < GEN7_L3LOG_SIZE/4; i++) {
- *cs++ = i915_mmio_reg_offset(GEN7_L3LOG(slice, i));
+ *cs++ = i915_get_lri_reg(rq->engine, GEN7_L3LOG(slice, i));
*cs++ = remap_info[i];
}
*cs++ = MI_NOOP;
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 126ab3667919..44369798ecc5 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -638,9 +638,9 @@ int intel_engine_emit_ctx_wa(struct i915_request *rq)
if (IS_ERR(cs))
return PTR_ERR(cs);
- *cs++ = MI_LOAD_REGISTER_IMM(wal->count);
+ *cs++ = MI_LOAD_REGISTER_IMM(rq->engine, wal->count);
for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
- *cs++ = i915_mmio_reg_offset(wa->reg);
+ *cs++ = i915_get_lri_reg(rq->engine, wa->reg);
*cs++ = wa->val;
}
*cs++ = MI_NOOP;
diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index d06d68ac2a3b..c31ea02ab91c 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -480,6 +480,8 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
for (i = 0; i < engine->whitelist.count; i++) {
u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+ u32 regLRI = i915_get_lri_reg(engine,
+ engine->whitelist.list[i].reg);
u64 addr = scratch->node.start;
struct i915_request *rq;
u32 srm, lrm, rsvd;
@@ -515,8 +517,8 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
idx = 1;
for (v = 0; v < ARRAY_SIZE(values); v++) {
/* LRI garbage */
- *cs++ = MI_LOAD_REGISTER_IMM(1);
- *cs++ = reg;
+ *cs++ = MI_LOAD_REGISTER_IMM(engine, 1);
+ *cs++ = regLRI;
*cs++ = values[v];
/* SRM result */
@@ -528,8 +530,8 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
}
for (v = 0; v < ARRAY_SIZE(values); v++) {
/* LRI garbage */
- *cs++ = MI_LOAD_REGISTER_IMM(1);
- *cs++ = reg;
+ *cs++ = MI_LOAD_REGISTER_IMM(engine, 1);
+ *cs++ = regLRI;
*cs++ = ~values[v];
/* SRM result */
@@ -823,9 +825,10 @@ static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
goto err_batch;
}
- *cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
+ *cs++ = MI_LOAD_REGISTER_IMM(engine, whitelist_writable_count(engine));
for (i = 0; i < engine->whitelist.count; i++) {
- u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+ u32 reg = i915_get_lri_reg(engine,
+ engine->whitelist.list[i].reg);
if (ro_register(reg))
continue;
diff --git a/drivers/gpu/drm/i915/gvt/mmio_context.c b/drivers/gpu/drm/i915/gvt/mmio_context.c
index 4208e40445b1..e44e1d14285c 100644
--- a/drivers/gpu/drm/i915/gvt/mmio_context.c
+++ b/drivers/gpu/drm/i915/gvt/mmio_context.c
@@ -210,14 +210,14 @@ restore_context_mmio_for_inhibit(struct intel_vgpu *vgpu,
if (IS_ERR(cs))
return PTR_ERR(cs);
- *cs++ = MI_LOAD_REGISTER_IMM(count);
+ *cs++ = MI_LOAD_REGISTER_IMM(req->engine, count);
for (mmio = gvt->engine_mmio_list.mmio;
i915_mmio_reg_valid(mmio->reg); mmio++) {
if (mmio->ring_id != ring_id ||
!mmio->in_context)
continue;
- *cs++ = i915_mmio_reg_offset(mmio->reg);
+ *cs++ = i915_get_lri_reg(req->engine, mmio->reg);
*cs++ = vgpu_vreg_t(vgpu, mmio->reg) |
(mmio->mask << 16);
gvt_dbg_core("add lri reg pair 0x%x:0x%x in inhibit ctx, vgpu:%d, rind_id:%d\n",
@@ -245,7 +245,11 @@ restore_render_mocs_control_for_inhibit(struct intel_vgpu *vgpu,
if (IS_ERR(cs))
return PTR_ERR(cs);
- *cs++ = MI_LOAD_REGISTER_IMM(GEN9_MOCS_SIZE);
+ /*
+ * GEN9_GFX_MOCS is not engine relative, therefore there is no
+ * need for relative addressing.
+ */
+ *cs++ = __MI_LOAD_REGISTER_IMM(GEN9_MOCS_SIZE);
for (index = 0; index < GEN9_MOCS_SIZE; index++) {
*cs++ = i915_mmio_reg_offset(GEN9_GFX_MOCS(index));
@@ -272,7 +276,11 @@ restore_render_mocs_l3cc_for_inhibit(struct intel_vgpu *vgpu,
if (IS_ERR(cs))
return PTR_ERR(cs);
- *cs++ = MI_LOAD_REGISTER_IMM(GEN9_MOCS_SIZE / 2);
+ /*
+ * GEN9_LNCFCMOCS is not engine relative, therefore there is no
+ * need for relative addressing.
+ */
+ *cs++ = __MI_LOAD_REGISTER_IMM(GEN9_MOCS_SIZE / 2);
for (index = 0; index < GEN9_MOCS_SIZE / 2; index++) {
*cs++ = i915_mmio_reg_offset(GEN9_LNCFCMOCS(index));
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 24555102e198..15ba08ab8180 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -222,7 +222,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
CMD( MI_SUSPEND_FLUSH, SMI, F, 1, S ),
CMD( MI_SEMAPHORE_MBOX, SMI, !F, 0xFF, R ),
CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
- CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
+ CMD( __MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
.reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 } ),
CMD( MI_STORE_REGISTER_MEM, SMI, F, 3, W | B,
.reg = { .offset = 1, .mask = 0x007FFFFC },
@@ -1183,8 +1183,8 @@ static bool check_cmd(const struct intel_engine_cs *engine,
return false;
}
- if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1) &&
- (offset + 2 > length ||
+ if (desc->cmd.value == __MI_LOAD_REGISTER_IMM(1)
+ && (offset + 2 > length ||
(cmd[offset + 1] & reg->mask) != reg->value)) {
DRM_DEBUG_DRIVER("CMD: Rejected LRI to masked register 0x%08X\n",
reg_addr);
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index e42b86827d6b..83abdda05ba2 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1692,17 +1692,23 @@ gen8_update_reg_state_unlocked(struct i915_perf_stream *stream,
};
int i;
- CTX_REG(reg_state, ctx_oactxctrl, GEN8_OACTXCONTROL,
+ /*
+ * NB: The LRI instruction is generated by the hardware.
+ * Should we read it in and assert that the offset flag is set?
+ */
+
+ CTX_REG(ce->engine, reg_state, ctx_oactxctrl, GEN8_OACTXCONTROL,
(stream->period_exponent << GEN8_OA_TIMER_PERIOD_SHIFT) |
(stream->periodic ? GEN8_OA_TIMER_ENABLE : 0) |
GEN8_OA_COUNTER_RESUME);
for (i = 0; i < ARRAY_SIZE(flex_regs); i++) {
- CTX_REG(reg_state, ctx_flexeu0 + i * 2, flex_regs[i],
+ CTX_REG(ce->engine, reg_state, ctx_flexeu0 + i * 2,
+ flex_regs[i],
oa_config_flex_reg(oa_config, flex_regs[i]));
}
- CTX_REG(reg_state,
+ CTX_REG(ce->engine, reg_state,
CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
intel_sseu_make_rpcs(i915, &ce->sseu));
}
@@ -1751,9 +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(rq->engine, count);
do {
- *cs++ = i915_mmio_reg_offset(flex->reg);
+ *cs++ = i915_get_lri_reg(rq->engine, flex->reg);
*cs++ = flex->value;
} while (flex++, --count);
*cs++ = MI_NOOP;
--
2.21.0.5.gaeb582a983
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/i915: Engine relative MMIO for Gen12
2019-08-22 18:02 [PATCH 0/2] drm/i915: Engine relative MMIO John.C.Harrison
2019-08-22 18:02 ` [PATCH 1/2] " John.C.Harrison
@ 2019-08-22 18:02 ` John.C.Harrison
2019-08-22 20:44 ` Daniele Ceraolo Spurio
2019-08-22 18:21 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7) Patchwork
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: John.C.Harrison @ 2019-08-22 18:02 UTC (permalink / raw)
To: Intel-GFX
From: John Harrison <John.C.Harrison@Intel.com>
Gen12 introduces a completely new and different scheme for
implementing engine relative MMIO accesses - MI_LRI_MMIO_REMAP. This
requires using the base address of instance zero of the relevant
engine class. And then, it is only valid if the register in
question falls within a certain range as specified by a table.
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
CC: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 185 ++++++++++++++++++-
drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 +
drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 9 +-
drivers/gpu/drm/i915/i915_perf.c | 3 +-
4 files changed, 195 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index efe1c377d797..a65e8ccd9d8d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -236,6 +236,127 @@ static u32 __engine_mmio_base(struct drm_i915_private *i915,
return bases[i].base;
}
+static void lri_init_remap_base(struct intel_engine_cs *engine)
+{
+ struct intel_engine_cs *remap_engine;
+
+ engine->lri_mmio_base = 0;
+
+ if (INTEL_GEN(engine->i915) < 12)
+ return;
+
+ remap_engine = engine->i915->gt.engine_class[engine->class][0];
+ GEM_BUG_ON(!remap_engine);
+
+ engine->lri_mmio_base = remap_engine->mmio_base;
+}
+
+static void lri_add_range(struct intel_engine_cs *engine, u32 min, u32 max)
+{
+ GEM_BUG_ON(engine->lri_num_ranges >= INTEL_MAX_LRI_RANGES);
+
+ engine->lri_ranges[engine->lri_num_ranges].min = min;
+ engine->lri_ranges[engine->lri_num_ranges].max = max;
+ engine->lri_num_ranges++;
+}
+
+static void lri_init_remap_ranges(struct intel_engine_cs *engine)
+{
+ bool has_aux_tables = true; /* Removed after TGL? */
+ u32 offset;
+
+ engine->lri_num_ranges = 0;
+
+ if (INTEL_GEN(engine->i915) < 12)
+ return;
+
+ switch (engine->class) {
+ case RENDER_CLASS:
+ /* Hardware Front End */
+ lri_add_range(engine, 0x000 + engine->mmio_base,
+ 0x7FF + engine->mmio_base);
+
+ /* TRTT */
+ lri_add_range(engine, 0x4400, 0x441F);
+
+ /* Aux Tables - REMOVEDBY(GEN:HAS:1406929672) */
+ if (has_aux_tables)
+ lri_add_range(engine, 0x4200, 0x420F);
+ break;
+
+ case VIDEO_DECODE_CLASS:
+ lri_add_range(engine, 0x0000 + engine->mmio_base,
+ 0x3FFF + engine->mmio_base);
+
+ /* TRTT */
+ offset = ((engine->instance & 0x1) * 0x20) +
+ ((engine->instance >> 1) * 0x100);
+ lri_add_range(engine, 0x4420 + offset, 0x443F + offset);
+
+ /* Aux Tables - REMOVEDBY(GEN:HAS:1406929672) */
+ if (has_aux_tables) {
+ switch (engine->instance) {
+ case 0:
+ lri_add_range(engine, 0x4210, 0x421F);
+ break;
+
+ case 1:
+ lri_add_range(engine, 0x4220, 0x422F);
+ break;
+
+ case 2:
+ lri_add_range(engine, 0x4290, 0x429F);
+ break;
+
+ case 3:
+ lri_add_range(engine, 0x42A0, 0x42AF);
+ break;
+
+ default:
+ break;
+ }
+ }
+ break;
+
+ case VIDEO_ENHANCEMENT_CLASS:
+ lri_add_range(engine, 0x0000 + engine->mmio_base,
+ 0x3FFF + engine->mmio_base);
+
+ /* TRTT */
+ offset = engine->instance * 0x100;
+ lri_add_range(engine, 0x4460 + offset, 0x447F + offset);
+
+ /* Aux Tables - REMOVEDBY(GEN:HAS:1406929672) */
+ if (has_aux_tables) {
+ switch (engine->instance) {
+ case 0:
+ lri_add_range(engine, 0x4230, 0x423F);
+ break;
+
+ case 1:
+ lri_add_range(engine, 0x42B0, 0x42BF);
+ break;
+
+ case 2:
+ lri_add_range(engine, 0x4290, 0x429F);
+ break;
+
+ case 3:
+ // Same address as instance 1???
+ lri_add_range(engine, 0x42B0, 0x42BF);
+ break;
+
+ default:
+ break;
+ }
+ }
+ break;
+
+ default:
+ break;
+ }
+}
+
static u32 i915_get_lri_cmd_legacy(const struct intel_engine_cs *engine,
u32 word_count)
{
@@ -249,6 +370,27 @@ static u32 i915_get_lri_cmd_add_offset(const struct intel_engine_cs *engine,
MI_LRI_ADD_CS_MMIO_START_GEN11;
}
+static u32 i915_get_lri_cmd_remap(const struct intel_engine_cs *engine,
+ u32 word_count)
+{
+ u32 word;
+
+ word = __MI_LOAD_REGISTER_IMM(word_count);
+
+ /* if (lri_is_reg_in_remap_table(engine, reg)) ??? */
+ word |= MI_LRI_MMIO_REMAP_GEN12;
+
+ /*
+ * NB: To gate this on the reg address will require knowing
+ * all reg addresses in advance. This is not currently the
+ * case as some LRI commands are built from multiple sources.
+ * Also, what if some regs require remap and some do not?
+ * The LRI command would need to be split into multiple pieces.
+ */
+
+ return word;
+}
+
static bool i915_engine_has_relative_lri(const struct intel_engine_cs *engine)
{
if (INTEL_GEN(engine->i915) < 11)
@@ -262,18 +404,53 @@ static bool i915_engine_has_relative_lri(const struct intel_engine_cs *engine)
static void lri_init(struct intel_engine_cs *engine)
{
- if (i915_engine_has_relative_lri(engine))
- engine->get_lri_cmd = i915_get_lri_cmd_add_offset;
- else
+ if (i915_engine_has_relative_lri(engine)) {
+ if (INTEL_GEN(engine->i915) < 12)
+ engine->get_lri_cmd = i915_get_lri_cmd_add_offset;
+ else {
+ engine->get_lri_cmd = i915_get_lri_cmd_remap;
+
+ lri_init_remap_base(engine);
+ lri_init_remap_ranges(engine);
+ }
+ } else
engine->get_lri_cmd = i915_get_lri_cmd_legacy;
}
+static bool lri_is_reg_in_remap_table(const struct intel_engine_cs *engine,
+ i915_reg_t reg)
+{
+ int i;
+ u32 offset = i915_mmio_reg_offset(reg);
+
+ for (i = 0; i < engine->lri_num_ranges; i++) {
+ if (offset < engine->lri_ranges[i].min)
+ continue;
+
+ if (offset > engine->lri_ranges[i].max)
+ continue;
+
+ return true;
+ }
+
+ return false;
+}
+
u32 i915_get_lri_reg(const struct intel_engine_cs *engine, i915_reg_t reg)
{
if (!i915_engine_has_relative_lri(engine))
return i915_mmio_reg_offset(reg);
- return i915_mmio_reg_offset(reg) - engine->mmio_base;
+ if (INTEL_GEN(engine->i915) < 12)
+ return i915_mmio_reg_offset(reg) - engine->mmio_base;
+
+ if (!WARN_ON(lri_is_reg_in_remap_table(engine, reg))) {
+ /* Is this meant to happen? */
+ return i915_mmio_reg_offset(reg);
+ }
+
+ return i915_mmio_reg_offset(reg) - engine->mmio_base +
+ engine->lri_mmio_base;
}
static void __sprint_engine_name(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 7ca6c86a33f6..1e26f668e73b 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -306,6 +306,13 @@ struct intel_engine_cs {
u32 context_size;
u32 mmio_base;
+#define INTEL_MAX_LRI_RANGES 3
+ struct lri_range {
+ u32 min, max;
+ } lri_ranges[INTEL_MAX_LRI_RANGES];
+ u32 lri_num_ranges;
+ u32 lri_mmio_base;
+
u32 (*get_lri_cmd)(const struct intel_engine_cs *engine,
u32 word_count);
diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
index eaa019df0ce7..0ee62a61d7b5 100644
--- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
+++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
@@ -130,14 +130,15 @@
* 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
+ * - Newer hardware supports engine relative addressing but using multiple
+ * incompatible schemes. 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)
#define MI_LRI_FORCE_POSTED (1<<12)
+#define MI_LRI_MMIO_REMAP_GEN12 (1<<17)
#define MI_LRI_ADD_CS_MMIO_START_GEN11 (1<<19)
#define MI_STORE_REGISTER_MEM MI_INSTR(0x24, 1)
#define MI_STORE_REGISTER_MEM_GEN8 MI_INSTR(0x24, 2)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 83abdda05ba2..f88642209283 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1694,7 +1694,8 @@ gen8_update_reg_state_unlocked(struct i915_perf_stream *stream,
/*
* NB: The LRI instruction is generated by the hardware.
- * Should we read it in and assert that the offset flag is set?
+ * Should we read it in and assert that the appropriate
+ * offset flag is set?
*/
CTX_REG(ce->engine, reg_state, ctx_oactxctrl, GEN8_OACTXCONTROL,
--
2.21.0.5.gaeb582a983
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7)
2019-08-22 18:02 [PATCH 0/2] drm/i915: Engine relative MMIO John.C.Harrison
2019-08-22 18:02 ` [PATCH 1/2] " John.C.Harrison
2019-08-22 18:02 ` [PATCH 2/2] drm/i915: Engine relative MMIO for Gen12 John.C.Harrison
@ 2019-08-22 18:21 ` Patchwork
2019-09-04 19:33 ` Rodrigo Vivi
2019-08-22 18:45 ` ✓ Fi.CI.BAT: success " Patchwork
2019-08-23 13:14 ` ✓ Fi.CI.IGT: " Patchwork
4 siblings, 1 reply; 9+ messages in thread
From: Patchwork @ 2019-08-22 18:21 UTC (permalink / raw)
To: John.C.Harrison; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Engine relative MMIO (rev7)
URL : https://patchwork.freedesktop.org/series/57117/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
2eab059bc87e drm/i915: Engine relative MMIO
-:108: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
+#define MI_LOAD_REGISTER_IMM(engine, count) \
+ (engine)->get_lri_cmd((engine), (count))
-:108: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'engine' - possible side-effects?
#108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
+#define MI_LOAD_REGISTER_IMM(engine, count) \
+ (engine)->get_lri_cmd((engine), (count))
-:203: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
#203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
+#define __MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
^
-:203: CHECK:SPACING: spaces preferred around that '-' (ctx:VxV)
#203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
+#define __MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
^
-:205: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#205: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
+#define MI_LRI_ADD_CS_MMIO_START_GEN11 (1<<19)
^
-:491: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#491: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:1719:
+ *cs++ = MI_LOAD_REGISTER_IMM(rq->engine, GEN7_L3LOG_SIZE/4);
^
-:522: CHECK:CAMELCASE: Avoid CamelCase: <regLRI>
#522: FILE: drivers/gpu/drm/i915/gt/selftest_workarounds.c:483:
+ u32 regLRI = i915_get_lri_reg(engine,
-:618: ERROR:SPACING: space prohibited after that open parenthesis '('
#618: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:225:
+ CMD( __MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
-:619: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#619: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:226:
+ CMD( __MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
.reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 } ),
-:629: CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
#629: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:1187:
+ if (desc->cmd.value == __MI_LOAD_REGISTER_IMM(1)
+ && (offset + 2 > length ||
total: 2 errors, 0 warnings, 8 checks, 531 lines checked
901081d701fe drm/i915: Engine relative MMIO for Gen12
-:182: CHECK:BRACES: braces {} should be used on all arms of this statement
#182: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:407:
+ if (i915_engine_has_relative_lri(engine)) {
[...]
+ } else
[...]
-:183: CHECK:BRACES: braces {} should be used on all arms of this statement
#183: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:408:
+ if (INTEL_GEN(engine->i915) < 12)
[...]
+ else {
[...]
-:185: CHECK:BRACES: Unbalanced braces around else statement
#185: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:410:
+ else {
-:191: CHECK:BRACES: Unbalanced braces around else statement
#191: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:416:
+ } else
-:271: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#271: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
+#define MI_LRI_MMIO_REMAP_GEN12 (1<<17)
^
total: 0 errors, 0 warnings, 5 checks, 252 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Engine relative MMIO (rev7)
2019-08-22 18:02 [PATCH 0/2] drm/i915: Engine relative MMIO John.C.Harrison
` (2 preceding siblings ...)
2019-08-22 18:21 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7) Patchwork
@ 2019-08-22 18:45 ` Patchwork
2019-08-23 13:14 ` ✓ Fi.CI.IGT: " Patchwork
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2019-08-22 18:45 UTC (permalink / raw)
To: John.C.Harrison; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Engine relative MMIO (rev7)
URL : https://patchwork.freedesktop.org/series/57117/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6767 -> Patchwork_14145
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/
Known issues
------------
Here are the changes found in Patchwork_14145 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_create@basic-files:
- fi-bxt-dsi: [PASS][1] -> [INCOMPLETE][2] ([fdo#103927])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/fi-bxt-dsi/igt@gem_ctx_create@basic-files.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/fi-bxt-dsi/igt@gem_ctx_create@basic-files.html
* igt@gem_ctx_switch@rcs0:
- fi-apl-guc: [PASS][3] -> [INCOMPLETE][4] ([fdo#103927])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/fi-apl-guc/igt@gem_ctx_switch@rcs0.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/fi-apl-guc/igt@gem_ctx_switch@rcs0.html
* igt@gem_exec_suspend@basic-s4-devices:
- fi-blb-e6850: [PASS][5] -> [INCOMPLETE][6] ([fdo#107718])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
* igt@kms_chamelium@hdmi-edid-read:
- fi-icl-u2: [PASS][7] -> [FAIL][8] ([fdo#109483])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/fi-icl-u2/igt@kms_chamelium@hdmi-edid-read.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/fi-icl-u2/igt@kms_chamelium@hdmi-edid-read.html
#### Possible fixes ####
* igt@core_auth@basic-auth:
- fi-icl-u3: [DMESG-WARN][9] ([fdo#107724]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/fi-icl-u3/igt@core_auth@basic-auth.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/fi-icl-u3/igt@core_auth@basic-auth.html
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
[fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
Participating hosts (55 -> 47)
------------------------------
Missing (8): fi-kbl-soraka fi-ilk-m540 fi-tgl-u fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_6767 -> Patchwork_14145
CI-20190529: 20190529
CI_DRM_6767: 3e978f97d4682186cc9734adbe834865e5eb9aca @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5148: 50390dd7adaccae21cafa85b866c17606cec94c3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_14145: 901081d701fe1db0fd725ba9b5469ef2ed63c426 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
901081d701fe drm/i915: Engine relative MMIO for Gen12
2eab059bc87e drm/i915: Engine relative MMIO
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: Engine relative MMIO for Gen12
2019-08-22 18:02 ` [PATCH 2/2] drm/i915: Engine relative MMIO for Gen12 John.C.Harrison
@ 2019-08-22 20:44 ` Daniele Ceraolo Spurio
0 siblings, 0 replies; 9+ messages in thread
From: Daniele Ceraolo Spurio @ 2019-08-22 20:44 UTC (permalink / raw)
To: John.C.Harrison, Intel-GFX
On 8/22/19 11:02 AM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Gen12 introduces a completely new and different scheme for
> implementing engine relative MMIO accesses - MI_LRI_MMIO_REMAP. This
> requires using the base address of instance zero of the relevant
> engine class. And then, it is only valid if the register in
> question falls within a certain range as specified by a table.
>
Bspec: 45606
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> CC: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 185 ++++++++++++++++++-
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 7 +
> drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 9 +-
> drivers/gpu/drm/i915/i915_perf.c | 3 +-
> 4 files changed, 195 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index efe1c377d797..a65e8ccd9d8d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -236,6 +236,127 @@ static u32 __engine_mmio_base(struct drm_i915_private *i915,
> return bases[i].base;
> }
>
> +static void lri_init_remap_base(struct intel_engine_cs *engine)
> +{
> + struct intel_engine_cs *remap_engine;
> +
> + engine->lri_mmio_base = 0;
> +
> + if (INTEL_GEN(engine->i915) < 12)
> + return;
> +
> + remap_engine = engine->i915->gt.engine_class[engine->class][0];
you can just use engine->gt. But anyway instance 0 is not guaranteed to
exist as it might've been fused off. if we add a static table of first
instance per class we can then fetch it directly from the info table. e.g.
static const enum first_instance[] = {
[RENDER_CLASS] = RCS0,
...
}
u32 id;
GEM_BUG_ON(engine->class > ARRAY_SIZE(first_instance));
id = first_instance[engine->class];
engine->lri_mmio_base =
__engine_mmio_base(i915, intel_engines[id].mmio_bases);
> + GEM_BUG_ON(!remap_engine);
> +
> + engine->lri_mmio_base = remap_engine->mmio_base; > +}
> +
> +static void lri_add_range(struct intel_engine_cs *engine, u32 min, u32 max)
I think it'd be cleaner at the call point if we go with base and size
instead of min and max.
> +{
> + GEM_BUG_ON(engine->lri_num_ranges >= INTEL_MAX_LRI_RANGES);
> +
> + engine->lri_ranges[engine->lri_num_ranges].min = min;
> + engine->lri_ranges[engine->lri_num_ranges].max = max;
> + engine->lri_num_ranges++;
> +}
> +
> +static void lri_init_remap_ranges(struct intel_engine_cs *engine)
> +{
> + bool has_aux_tables = true; /* Removed after TGL? */
This should really be a device flag. But do we actually need to write
any of the registers in the aux tables from the kernel? if not, maybe we
can get away not setting these in the ranges, adding a nice comment
about it in case we get failures later on. That way we'll also be able
to keep lri_num_ranges fixed at 2 and just use a define for it (we can
use a variable local to this function for lri_add_range)
> + u32 offset;
> +
> + engine->lri_num_ranges = 0;
> +
> + if (INTEL_GEN(engine->i915) < 12)
> + return;
> +
> + switch (engine->class) {
> + case RENDER_CLASS:
> + /* Hardware Front End */
> + lri_add_range(engine, 0x000 + engine->mmio_base,
> + 0x7FF + engine->mmio_base);
> +
> + /* TRTT */
> + lri_add_range(engine, 0x4400, 0x441F);
> +
> + /* Aux Tables - REMOVEDBY(GEN:HAS:1406929672) */
> + if (has_aux_tables)
> + lri_add_range(engine, 0x4200, 0x420F);
> + break;
matches
> +
> + case VIDEO_DECODE_CLASS:
> + lri_add_range(engine, 0x0000 + engine->mmio_base,
> + 0x3FFF + engine->mmio_base);
> +
> + /* TRTT */
> + offset = ((engine->instance & 0x1) * 0x20) +
> + ((engine->instance >> 1) * 0x100);
> + lri_add_range(engine, 0x4420 + offset, 0x443F + offset);
Matches. A bit convoluted, but I can't suggest a better option. Maybe a
comment?
> +
> + /* Aux Tables - REMOVEDBY(GEN:HAS:1406929672) */
> + if (has_aux_tables) {
> + switch (engine->instance) {
> + case 0:
> + lri_add_range(engine, 0x4210, 0x421F);
> + break;
> +
> + case 1:
> + lri_add_range(engine, 0x4220, 0x422F);
> + break;
> +
> + case 2:
> + lri_add_range(engine, 0x4290, 0x429F);
> + break;
> +
> + case 3:
> + lri_add_range(engine, 0x42A0, 0x42AF);
> + break;
> +
> + default:
> + break;
> + }
> + }
> + break;
> +
> + case VIDEO_ENHANCEMENT_CLASS:
> + lri_add_range(engine, 0x0000 + engine->mmio_base,
> + 0x3FFF + engine->mmio_base);
> +
> + /* TRTT */
> + offset = engine->instance * 0x100;
> + lri_add_range(engine, 0x4460 + offset, 0x447F + offset);
matches
> +
> + /* Aux Tables - REMOVEDBY(GEN:HAS:1406929672) */
> + if (has_aux_tables) {
> + switch (engine->instance) {
> + case 0:
> + lri_add_range(engine, 0x4230, 0x423F);
> + break;
> +
> + case 1:
> + lri_add_range(engine, 0x42B0, 0x42BF);
> + break;
> +
> + case 2:
> + lri_add_range(engine, 0x4290, 0x429F);
> + break;
> +
> + case 3:
> + // Same address as instance 1???
> + lri_add_range(engine, 0x42B0, 0x42BF);
> + break;
> +
We do not have this many VECS engines defined.
Also, can we do the check on the updated offset? that way we only need
to record and validate the offsets from instance 0.
> + default:
> + break;
> + }
> + }
> + break;
> +
> + default:
> + break;
> + }
> +}
> +
> static u32 i915_get_lri_cmd_legacy(const struct intel_engine_cs *engine,
> u32 word_count)
> {
> @@ -249,6 +370,27 @@ static u32 i915_get_lri_cmd_add_offset(const struct intel_engine_cs *engine,
> MI_LRI_ADD_CS_MMIO_START_GEN11;
> }
>
> +static u32 i915_get_lri_cmd_remap(const struct intel_engine_cs *engine,
> + u32 word_count)
> +{
> + u32 word;
> +
> + word = __MI_LOAD_REGISTER_IMM(word_count);
> +
> + /* if (lri_is_reg_in_remap_table(engine, reg)) ??? */
> + word |= MI_LRI_MMIO_REMAP_GEN12;
> +
> + /*
> + * NB: To gate this on the reg address will require knowing
> + * all reg addresses in advance. This is not currently the
> + * case as some LRI commands are built from multiple sources.
> + * Also, what if some regs require remap and some do not?
> + * The LRI command would need to be split into multiple pieces.
> + */
All the register in the engine ranges (i.e. the ones repeated for all
engines) are covered, so we should expect that all registers require
remap. If we want to write a register that it's in the global range then
we should use __MI_LOAD_REGISTER_IMM directly. Therefore, I would just do:
return __MI_LOAD_REGISTER_IMM(word_count) | MI_LRI_MMIO_REMAP_GEN12;
> +
> + return word;
> +}
> +
> static bool i915_engine_has_relative_lri(const struct intel_engine_cs *engine)
> {
> if (INTEL_GEN(engine->i915) < 11)
> @@ -262,18 +404,53 @@ static bool i915_engine_has_relative_lri(const struct intel_engine_cs *engine)
>
> static void lri_init(struct intel_engine_cs *engine)
> {
> - if (i915_engine_has_relative_lri(engine))
> - engine->get_lri_cmd = i915_get_lri_cmd_add_offset;
> - else
> + if (i915_engine_has_relative_lri(engine)) {
> + if (INTEL_GEN(engine->i915) < 12)
> + engine->get_lri_cmd = i915_get_lri_cmd_add_offset;
> + else {
> + engine->get_lri_cmd = i915_get_lri_cmd_remap;
the only difference in the cmd between the various modes is the flag we
set, can't we just save that instead of having a full-blown virtual
function?
> +
> + lri_init_remap_base(engine);
> + lri_init_remap_ranges(engine);
> + }
> + } else
> engine->get_lri_cmd = i915_get_lri_cmd_legacy;
> }
>
> +static bool lri_is_reg_in_remap_table(const struct intel_engine_cs *engine,
> + i915_reg_t reg)
> +{
> + int i;
> + u32 offset = i915_mmio_reg_offset(reg);
> +
> + for (i = 0; i < engine->lri_num_ranges; i++) {
> + if (offset < engine->lri_ranges[i].min)
> + continue;
> +
> + if (offset > engine->lri_ranges[i].max)
> + continue;
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> u32 i915_get_lri_reg(const struct intel_engine_cs *engine, i915_reg_t reg)
> {
> if (!i915_engine_has_relative_lri(engine))
> return i915_mmio_reg_offset(reg);
>
> - return i915_mmio_reg_offset(reg) - engine->mmio_base;
> + if (INTEL_GEN(engine->i915) < 12)
> + return i915_mmio_reg_offset(reg) - engine->mmio_base;
> +
> + if (!WARN_ON(lri_is_reg_in_remap_table(engine, reg))) {
> + /* Is this meant to happen? */
No, so just go with:
GEM_DEBUG_BUG_ON(!lri_is_reg_in_remap_table(engine, reg));
and make all the remap table code conditionally compiled based on
CONFIG_DRM_I915_DEBUG_GEM.
> + return i915_mmio_reg_offset(reg);
> + }
> +
> + return i915_mmio_reg_offset(reg) - engine->mmio_base +
> + engine->lri_mmio_base;
This doesn't work. The reason why we have the new mechanism is that the
register for different instances are no at the same delta from the MMIO
base. E.g. the TRTT registers you have above.
> }
if we set engine->lri_mmio_base appropriately on all platforms, can't we
instead just use that instead of engine->mmio_base when emitting the
registers with the relative LRI? to keep the check we can do something like:
#define lri_reg(engine__, reg__) \
({ \
GEM_DEBUG_BUG_ON(INTEL_GEN(engine->i915) >= 12 && \
!lri_is_reg_in_remap_table(engine__, reg__)); \
reg__; \
})
Daniele
>
> static void __sprint_engine_name(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 7ca6c86a33f6..1e26f668e73b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -306,6 +306,13 @@ struct intel_engine_cs {
> u32 context_size;
> u32 mmio_base;
>
> +#define INTEL_MAX_LRI_RANGES 3
> + struct lri_range {
> + u32 min, max;
> + } lri_ranges[INTEL_MAX_LRI_RANGES];
> + u32 lri_num_ranges;
> + u32 lri_mmio_base;
> +
> u32 (*get_lri_cmd)(const struct intel_engine_cs *engine,
> u32 word_count);
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> index eaa019df0ce7..0ee62a61d7b5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> @@ -130,14 +130,15 @@
> * 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
> + * - Newer hardware supports engine relative addressing but using multiple
> + * incompatible schemes. 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)
> #define MI_LRI_FORCE_POSTED (1<<12)
> +#define MI_LRI_MMIO_REMAP_GEN12 (1<<17)
> #define MI_LRI_ADD_CS_MMIO_START_GEN11 (1<<19)
> #define MI_STORE_REGISTER_MEM MI_INSTR(0x24, 1)
> #define MI_STORE_REGISTER_MEM_GEN8 MI_INSTR(0x24, 2)
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 83abdda05ba2..f88642209283 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1694,7 +1694,8 @@ gen8_update_reg_state_unlocked(struct i915_perf_stream *stream,
>
> /*
> * NB: The LRI instruction is generated by the hardware.
> - * Should we read it in and assert that the offset flag is set?
> + * Should we read it in and assert that the appropriate
> + * offset flag is set?
> */
>
> CTX_REG(ce->engine, reg_state, ctx_oactxctrl, GEN8_OACTXCONTROL,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Engine relative MMIO (rev7)
2019-08-22 18:02 [PATCH 0/2] drm/i915: Engine relative MMIO John.C.Harrison
` (3 preceding siblings ...)
2019-08-22 18:45 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2019-08-23 13:14 ` Patchwork
4 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2019-08-23 13:14 UTC (permalink / raw)
To: John.C.Harrison; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Engine relative MMIO (rev7)
URL : https://patchwork.freedesktop.org/series/57117/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6767_full -> Patchwork_14145_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_14145_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_schedule@fifo-bsd1:
- shard-iclb: [PASS][1] -> [SKIP][2] ([fdo#109276]) +18 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb1/igt@gem_exec_schedule@fifo-bsd1.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb6/igt@gem_exec_schedule@fifo-bsd1.html
* igt@gem_exec_schedule@preempt-queue-bsd:
- shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#111325]) +5 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb6/igt@gem_exec_schedule@preempt-queue-bsd.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd.html
* igt@i915_suspend@fence-restore-untiled:
- shard-skl: [PASS][5] -> [INCOMPLETE][6] ([fdo#104108])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-skl3/igt@i915_suspend@fence-restore-untiled.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-skl8/igt@i915_suspend@fence-restore-untiled.html
* igt@kms_cursor_crc@pipe-a-cursor-256x85-offscreen:
- shard-iclb: [PASS][7] -> [INCOMPLETE][8] ([fdo#107713])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb2/igt@kms_cursor_crc@pipe-a-cursor-256x85-offscreen.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb7/igt@kms_cursor_crc@pipe-a-cursor-256x85-offscreen.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible:
- shard-skl: [PASS][9] -> [FAIL][10] ([fdo#105363])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-skl5/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
* igt@kms_flip@flip-vs-suspend-interruptible:
- shard-apl: [PASS][11] -> [DMESG-WARN][12] ([fdo#108566]) +3 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-apl5/igt@kms_flip@flip-vs-suspend-interruptible.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
- shard-iclb: [PASS][13] -> [FAIL][14] ([fdo#103167]) +2 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
* igt@kms_plane_lowres@pipe-a-tiling-y:
- shard-iclb: [PASS][15] -> [FAIL][16] ([fdo#103166])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb3/igt@kms_plane_lowres@pipe-a-tiling-y.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb1/igt@kms_plane_lowres@pipe-a-tiling-y.html
* igt@kms_psr@psr2_cursor_plane_onoff:
- shard-iclb: [PASS][17] -> [SKIP][18] ([fdo#109441]) +2 similar issues
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb7/igt@kms_psr@psr2_cursor_plane_onoff.html
* igt@kms_setmode@basic:
- shard-apl: [PASS][19] -> [FAIL][20] ([fdo#99912])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-apl4/igt@kms_setmode@basic.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-apl7/igt@kms_setmode@basic.html
* igt@perf_pmu@enable-race-bcs0:
- shard-apl: [PASS][21] -> [INCOMPLETE][22] ([fdo#103927])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-apl4/igt@perf_pmu@enable-race-bcs0.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-apl4/igt@perf_pmu@enable-race-bcs0.html
#### Possible fixes ####
* igt@gem_ctx_isolation@vcs0-s3:
- shard-skl: [INCOMPLETE][23] ([fdo#104108]) -> [PASS][24] +1 similar issue
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-skl5/igt@gem_ctx_isolation@vcs0-s3.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-skl8/igt@gem_ctx_isolation@vcs0-s3.html
* igt@gem_exec_schedule@preempt-other-chain-bsd:
- shard-iclb: [SKIP][25] ([fdo#111325]) -> [PASS][26] +4 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb1/igt@gem_exec_schedule@preempt-other-chain-bsd.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb6/igt@gem_exec_schedule@preempt-other-chain-bsd.html
* igt@gem_exec_schedule@preempt-queue-bsd1:
- shard-iclb: [SKIP][27] ([fdo#109276]) -> [PASS][28] +16 similar issues
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb8/igt@gem_exec_schedule@preempt-queue-bsd1.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb4/igt@gem_exec_schedule@preempt-queue-bsd1.html
* igt@gem_workarounds@suspend-resume-context:
- shard-apl: [DMESG-WARN][29] ([fdo#108566]) -> [PASS][30] +5 similar issues
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-apl2/igt@gem_workarounds@suspend-resume-context.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-apl2/igt@gem_workarounds@suspend-resume-context.html
* igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
- shard-hsw: [FAIL][31] ([fdo#105767]) -> [PASS][32]
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-hsw2/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-hsw1/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
* igt@kms_flip@flip-vs-suspend:
- shard-skl: [INCOMPLETE][33] ([fdo#109507]) -> [PASS][34]
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-skl4/igt@kms_flip@flip-vs-suspend.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-skl7/igt@kms_flip@flip-vs-suspend.html
- shard-snb: [INCOMPLETE][35] ([fdo#105411]) -> [PASS][36]
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-snb1/igt@kms_flip@flip-vs-suspend.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-snb7/igt@kms_flip@flip-vs-suspend.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render:
- shard-iclb: [FAIL][37] ([fdo#103167]) -> [PASS][38] +7 similar issues
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render.html
* igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
- shard-skl: [FAIL][39] ([fdo#108145] / [fdo#110403]) -> [PASS][40]
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-skl10/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-skl4/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
* igt@kms_plane_lowres@pipe-a-tiling-x:
- shard-iclb: [FAIL][41] ([fdo#103166]) -> [PASS][42]
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb6/igt@kms_plane_lowres@pipe-a-tiling-x.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb4/igt@kms_plane_lowres@pipe-a-tiling-x.html
* igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
- shard-skl: [DMESG-WARN][43] ([fdo#106885]) -> [PASS][44]
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-skl6/igt@kms_plane_multiple@atomic-pipe-a-tiling-yf.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-skl7/igt@kms_plane_multiple@atomic-pipe-a-tiling-yf.html
* igt@kms_psr@psr2_primary_blt:
- shard-iclb: [SKIP][45] ([fdo#109441]) -> [PASS][46] +1 similar issue
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb8/igt@kms_psr@psr2_primary_blt.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb2/igt@kms_psr@psr2_primary_blt.html
* igt@perf@blocking:
- shard-skl: [FAIL][47] ([fdo#110728]) -> [PASS][48]
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-skl9/igt@perf@blocking.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-skl3/igt@perf@blocking.html
#### Warnings ####
* igt@gem_ctx_isolation@vcs1-nonpriv:
- shard-iclb: [SKIP][49] ([fdo#109276]) -> [FAIL][50] ([fdo#111329])
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb6/igt@gem_ctx_isolation@vcs1-nonpriv.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb4/igt@gem_ctx_isolation@vcs1-nonpriv.html
* igt@gem_mocs_settings@mocs-reset-bsd2:
- shard-iclb: [SKIP][51] ([fdo#109276]) -> [FAIL][52] ([fdo#111330])
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb6/igt@gem_mocs_settings@mocs-reset-bsd2.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb1/igt@gem_mocs_settings@mocs-reset-bsd2.html
* igt@gem_mocs_settings@mocs-settings-bsd2:
- shard-iclb: [FAIL][53] ([fdo#111330]) -> [SKIP][54] ([fdo#109276])
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6767/shard-iclb2/igt@gem_mocs_settings@mocs-settings-bsd2.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/shard-iclb5/igt@gem_mocs_settings@mocs-settings-bsd2.html
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
[fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
[fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
[fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
[fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
[fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
[fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
[fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (9 -> 9)
------------------------------
No changes in participating hosts
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_6767 -> Patchwork_14145
CI-20190529: 20190529
CI_DRM_6767: 3e978f97d4682186cc9734adbe834865e5eb9aca @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5148: 50390dd7adaccae21cafa85b866c17606cec94c3 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_14145: 901081d701fe1db0fd725ba9b5469ef2ed63c426 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14145/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7)
2019-08-22 18:21 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7) Patchwork
@ 2019-09-04 19:33 ` Rodrigo Vivi
2019-09-05 23:42 ` John Harrison
0 siblings, 1 reply; 9+ messages in thread
From: Rodrigo Vivi @ 2019-09-04 19:33 UTC (permalink / raw)
To: intel-gfx
On Thu, Aug 22, 2019 at 06:21:23PM -0000, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915: Engine relative MMIO (rev7)
> URL : https://patchwork.freedesktop.org/series/57117/
> State : warning
>
> == Summary ==
>
> $ dim checkpatch origin/drm-tip
> 2eab059bc87e drm/i915: Engine relative MMIO
> -:108: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
> #108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
> +#define MI_LOAD_REGISTER_IMM(engine, count) \
> + (engine)->get_lri_cmd((engine), (count))
>
> -:108: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'engine' - possible side-effects?
> #108: FILE: drivers/gpu/drm/i915/gt/intel_engine.h:514:
> +#define MI_LOAD_REGISTER_IMM(engine, count) \
> + (engine)->get_lri_cmd((engine), (count))
>
> -:203: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
> +#define __MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
> ^
>
> -:203: CHECK:SPACING: spaces preferred around that '-' (ctx:VxV)
> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
> +#define __MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
> ^
>
> -:205: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> #205: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
> +#define MI_LRI_ADD_CS_MMIO_START_GEN11 (1<<19)
> ^
>
> -:491: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
> #491: FILE: drivers/gpu/drm/i915/gt/intel_ringbuffer.c:1719:
> + *cs++ = MI_LOAD_REGISTER_IMM(rq->engine, GEN7_L3LOG_SIZE/4);
> ^
>
> -:522: CHECK:CAMELCASE: Avoid CamelCase: <regLRI>
> #522: FILE: drivers/gpu/drm/i915/gt/selftest_workarounds.c:483:
> + u32 regLRI = i915_get_lri_reg(engine,
>
> -:618: ERROR:SPACING: space prohibited after that open parenthesis '('
> #618: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:225:
> + CMD( __MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
>
> -:619: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
> #619: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:226:
> + CMD( __MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
> .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 } ),
>
> -:629: CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
> #629: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:1187:
> + if (desc->cmd.value == __MI_LOAD_REGISTER_IMM(1)
> + && (offset + 2 > length ||
>
> total: 2 errors, 0 warnings, 8 checks, 531 lines checked
> 901081d701fe drm/i915: Engine relative MMIO for Gen12
> -:182: CHECK:BRACES: braces {} should be used on all arms of this statement
> #182: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:407:
> + if (i915_engine_has_relative_lri(engine)) {
> [...]
> + } else
> [...]
>
> -:183: CHECK:BRACES: braces {} should be used on all arms of this statement
> #183: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:408:
> + if (INTEL_GEN(engine->i915) < 12)
> [...]
> + else {
> [...]
>
> -:185: CHECK:BRACES: Unbalanced braces around else statement
> #185: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:410:
> + else {
>
> -:191: CHECK:BRACES: Unbalanced braces around else statement
> #191: FILE: drivers/gpu/drm/i915/gt/intel_engine_cs.c:416:
> + } else
>
> -:271: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> #271: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
> +#define MI_LRI_MMIO_REMAP_GEN12 (1<<17)
> ^
>
> total: 0 errors, 0 warnings, 5 checks, 252 lines checked
I looks like we need to fix many (most) of the issues here before proceed.
Also, did you check the naming with Tvrtko. If I remember correctly
his preference was for "MI_LOAD_REGISTER_IMM_REL for
usage on relevant (new) paths."
and don't touch the old ones.
Also I'm not sure if I like _MI_LOAD for the old and MI_LOAD for the new.
Tvrtko original's suggestion makes the distinction clean.
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7)
2019-09-04 19:33 ` Rodrigo Vivi
@ 2019-09-05 23:42 ` John Harrison
0 siblings, 0 replies; 9+ messages in thread
From: John Harrison @ 2019-09-05 23:42 UTC (permalink / raw)
To: Rodrigo Vivi, intel-gfx
On 9/4/2019 12:33, Rodrigo Vivi wrote:
> On Thu, Aug 22, 2019 at 06:21:23PM -0000, Patchwork wrote:
>> == Series Details ==
>>
>> Series: drm/i915: Engine relative MMIO (rev7)
>> URL : https://patchwork.freedesktop.org/series/57117/
>> State : warning
>>
>> == Summary ==
>>
>> $ dim checkpatch origin/drm-tip
>> 2eab059bc87e drm/i915: Engine relative MMIO
>> -:203: CHECK:SPACING: spaces preferred around that '*' (ctx:VxV)
>> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
>> +#define __MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
>> ^
>>
>> -:203: CHECK:SPACING: spaces preferred around that '-' (ctx:VxV)
>> #203: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:139:
>> +#define __MI_LOAD_REGISTER_IMM(x) MI_INSTR(0x22, 2*(x)-1)
>> ^
>>
>> -:205: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>> #205: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
>> +#define MI_LRI_ADD_CS_MMIO_START_GEN11 (1<<19)
>> ^
>>
>> -:618: ERROR:SPACING: space prohibited after that open parenthesis '('
>> #618: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:225:
>> + CMD( __MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
>>
>> -:619: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
>> #619: FILE: drivers/gpu/drm/i915/i915_cmd_parser.c:226:
>> + CMD( __MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
>> .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 } ),
>>
>> total: 2 errors, 0 warnings, 8 checks, 531 lines checked
>> 901081d701fe drm/i915: Engine relative MMIO for Gen12
>> -:271: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>> #271: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:141:
>> +#define MI_LRI_MMIO_REMAP_GEN12 (1<<17)
>> ^
>>
>> I looks like we need to fix many (most) of the issues here before proceed.
The above are all in keeping with the style of the surrounding code.
Indeed, the command parser ones are because the code is laid out in a
formatted table. Should they be changed to obey the style rules or left
as is?
>> Also, did you check the naming with Tvrtko. If I remember correctly
>> his preference was for "MI_LOAD_REGISTER_IMM_REL for
>> usage on relevant (new) paths."
>>
>> and don't touch the old ones.
>>
>> Also I'm not sure if I like _MI_LOAD for the old and MI_LOAD for the new.
>> Tvrtko original's suggestion makes the distinction clean.
I believe Tvrtko is still on vacation. However, the point is that this
is not old versus new code. It is also not about code that wants to use
some fancy new feature of indirect addressing that did not exist before.
This is about making existing code paths work on new hardware. The point
is that if someone wishes to emit an LRI instruction and they want it to
work on both Gen7 and on Gen11 then it needs to be issued differently
for each of those devices. The code that is emitting the LRI doesn't
care which device it is on. It just wants to make a write happen. So my
argument is that the obvious, default, this is the normal way to do an
LRI version should be the one that copes with the idiosyncrasies of the
hardware. Whereas, if the code writer specifically wishes to not support
Gen11, or is doing something else strange then it is not an issue that
they have to knowingly use a 'here-be-dragons' version of the LRI
instruction.
Otherwise you end up with the 'here-be-dragons' version actually being
the one needed to support hardware of any generation. Whereas the
'use-me-please' version will fail on newer devices. That situation is
just inviting people to use the wrong one and create bugs on Gen11 onward.
John.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-09-05 23:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-22 18:02 [PATCH 0/2] drm/i915: Engine relative MMIO John.C.Harrison
2019-08-22 18:02 ` [PATCH 1/2] " John.C.Harrison
2019-08-22 18:02 ` [PATCH 2/2] drm/i915: Engine relative MMIO for Gen12 John.C.Harrison
2019-08-22 20:44 ` Daniele Ceraolo Spurio
2019-08-22 18:21 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Engine relative MMIO (rev7) Patchwork
2019-09-04 19:33 ` Rodrigo Vivi
2019-09-05 23:42 ` John Harrison
2019-08-22 18:45 ` ✓ Fi.CI.BAT: success " Patchwork
2019-08-23 13:14 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.