From: Fei Yang fei.yang@intel.com
GPU hangs have been observed when multiple engines write to the same aux_inv register at the same time. To avoid this each engine should only invalidate its own auxiliary table. The function gen12_emit_flush_xcs() currently invalidate the auxiliary table for all engines because the rq->engine is not necessarily the engine eventually carrying out the request, and potentially the engine could even be a virtual one (with engine->instance being -1). With this patch, auxiliary table invalidation is done only for the engine executing the request. And the mmio address for the aux_inv register is set after the engine instance becomes certain.
Signed-off-by: Chris Wilson chris.p.wilson@intel.com Signed-off-by: Fei Yang fei.yang@intel.com --- drivers/gpu/drm/i915/gt/gen2_engine_cs.c | 9 +++ drivers/gpu/drm/i915/gt/gen6_engine_cs.c | 9 +++ drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 61 ++++++++----------- .../drm/i915/gt/intel_execlists_submission.c | 35 +++++++++++ drivers/gpu/drm/i915/i915_request.h | 2 + 5 files changed, 82 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c index 1c82caf525c3..0ec4986e4805 100644 --- a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c @@ -37,6 +37,9 @@ int gen2_emit_flush(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen2 */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -123,6 +126,9 @@ int gen4_emit_flush_rcs(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen4 rcs */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -138,6 +144,9 @@ int gen4_emit_flush_vcs(struct i915_request *rq, u32 mode) *cs++ = MI_NOOP; intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen4 vcs */ + rq->aux_inv_fixup = NULL; + return 0; }
diff --git a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c index 5e65550b4dfb..efe51c4662fe 100644 --- a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c @@ -137,6 +137,9 @@ int gen6_emit_flush_rcs(struct i915_request *rq, u32 mode) *cs++ = 0; intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen6 */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -208,6 +211,9 @@ static int mi_flush_dw(struct i915_request *rq, u32 flags)
intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen6 */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -347,6 +353,9 @@ int gen7_emit_flush_rcs(struct i915_request *rq, u32 mode) *cs++ = 0; intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen7 rcs */ + rq->aux_inv_fixup = NULL; + return 0; }
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index b1b9c3fd7bf9..b6374cf53314 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -73,6 +73,9 @@ int gen8_emit_flush_rcs(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen8 rcs */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -106,6 +109,9 @@ int gen8_emit_flush_xcs(struct i915_request *rq, u32 mode) *cs++ = 0; /* value */ intel_ring_advance(rq, cs);
+ /* hsdes: 1809175790. No fixup needed for gen8 xcs */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -157,6 +163,9 @@ int gen11_emit_flush_rcs(struct i915_request *rq, u32 mode) intel_ring_advance(rq, cs); }
+ /* hsdes: 1809175790. No fixup needed for gen11 rcs */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -165,30 +174,6 @@ static u32 preparser_disable(bool state) return MI_ARB_CHECK | 1 << 8 | state; }
-static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) -{ - static const i915_reg_t vd[] = { - GEN12_VD0_AUX_NV, - GEN12_VD1_AUX_NV, - GEN12_VD2_AUX_NV, - GEN12_VD3_AUX_NV, - }; - - static const i915_reg_t ve[] = { - GEN12_VE0_AUX_NV, - GEN12_VE1_AUX_NV, - }; - - if (engine->class == VIDEO_DECODE_CLASS) - return vd[engine->instance]; - - if (engine->class == VIDEO_ENHANCEMENT_CLASS) - return ve[engine->instance]; - - GEM_BUG_ON("unknown aux_inv reg\n"); - return INVALID_MMIO_REG; -} - static u32 *gen12_emit_aux_table_inv(const i915_reg_t inv_reg, u32 *cs) { *cs++ = MI_LOAD_REGISTER_IMM(1); @@ -274,6 +259,9 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) intel_ring_advance(rq, cs); }
+ /* hsdes: 1809175790. aux_inv has been done above, no fixup needed */ + rq->aux_inv_fixup = NULL; + return 0; }
@@ -288,7 +276,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) if (mode & EMIT_INVALIDATE) aux_inv = rq->engine->mask & ~BIT(BCS0); if (aux_inv) - cmd += 2 * hweight32(aux_inv) + 2; + cmd += 4;
cs = intel_ring_begin(rq, cmd); if (IS_ERR(cs)) @@ -319,16 +307,21 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) *cs++ = 0; /* value */
if (aux_inv) { /* hsdes: 1809175790 */ - struct intel_engine_cs *engine; - unsigned int tmp; - - *cs++ = MI_LOAD_REGISTER_IMM(hweight32(aux_inv)); - for_each_engine_masked(engine, rq->engine->gt, aux_inv, tmp) { - *cs++ = i915_mmio_reg_offset(aux_inv_reg(engine)); - *cs++ = AUX_INV; - } + /* + * We don't know which engine will eventually carry out + * this request, so the mmio aux_inv register address is + * unknown at this moment. We save the cs pointer supposed + * to hold the aux_inv address in rq->aux_inv_fixup and set + * it in execlists_dequeue() when the engine instance + * carrying out this request becomes certain + */ + *cs++ = MI_LOAD_REGISTER_IMM(1); + rq->aux_inv_fixup = cs; /* save the pointer to aux_inv */ + *cs++ = 0; /* mmio addr to be set at submission to HW */ + *cs++ = AUX_INV; *cs++ = MI_NOOP; - } + } else + rq->aux_inv_fixup = NULL;
if (mode & EMIT_INVALIDATE) *cs++ = preparser_disable(false); diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index e1470bb60f34..7e8552414275 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1258,6 +1258,34 @@ static bool completed(const struct i915_request *rq) return __i915_request_is_complete(rq); }
+static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) +{ + static const i915_reg_t vd[] = { + GEN12_VD0_AUX_NV, + GEN12_VD1_AUX_NV, + GEN12_VD2_AUX_NV, + GEN12_VD3_AUX_NV, + }; + + static const i915_reg_t ve[] = { + GEN12_VE0_AUX_NV, + GEN12_VE1_AUX_NV, + }; + + if (engine->class == VIDEO_DECODE_CLASS) { + GEM_BUG_ON(engine->instance >= ARRAY_SIZE(vd)); + return vd[engine->instance]; + } + + if (engine->class == VIDEO_ENHANCEMENT_CLASS) { + GEM_BUG_ON(engine->instance >= ARRAY_SIZE(ve)); + return ve[engine->instance]; + } + + GEM_BUG_ON("unknown aux_inv reg\n"); + return INVALID_MMIO_REG; +} + static void execlists_dequeue(struct intel_engine_cs *engine) { struct intel_engine_execlists * const execlists = &engine->execlists; @@ -1538,6 +1566,13 @@ static void execlists_dequeue(struct intel_engine_cs *engine) }
if (__i915_request_submit(rq)) { + /* hsdes: 1809175790 */ + if (rq->aux_inv_fixup) { + *rq->aux_inv_fixup = + i915_mmio_reg_offset + (aux_inv_reg(engine)); + rq->execution_mask = engine->mask; + } if (!merge) { *port++ = i915_request_get(last); last = NULL; diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index 28b1f9db5487..8e0764c43936 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -312,6 +312,8 @@ struct i915_request { struct i915_capture_list *capture_list; #endif
+ u32 *aux_inv_fixup; /* hsdes: 1809175790 */ + /** Time at which this request was emitted, in jiffies. */ unsigned long emitted_jiffies;
On Fri, 2022-03-04 at 14:14 -0800, fei.yang@intel.com wrote:
From: Fei Yang fei.yang@intel.com
GPU hangs have been observed when multiple engines write to the same aux_inv register at the same time. To avoid this each engine should only invalidate its own auxiliary table. The function gen12_emit_flush_xcs() currently invalidate the auxiliary table for all engines because the rq->engine is not necessarily the engine eventually carrying out the request, and potentially the engine could even be a virtual one (with engine->instance being -1). With this patch, auxiliary table invalidation is done only for the engine executing the request. And the mmio address for the aux_inv register is set after the engine instance becomes certain.
Signed-off-by: Chris Wilson chris.p.wilson@intel.com Signed-off-by: Fei Yang fei.yang@intel.com
drivers/gpu/drm/i915/gt/gen2_engine_cs.c | 9 +++ drivers/gpu/drm/i915/gt/gen6_engine_cs.c | 9 +++ drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 61 ++++++++--------- -- .../drm/i915/gt/intel_execlists_submission.c | 35 +++++++++++ drivers/gpu/drm/i915/i915_request.h | 2 + 5 files changed, 82 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c index 1c82caf525c3..0ec4986e4805 100644 --- a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c @@ -37,6 +37,9 @@ int gen2_emit_flush(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen2 */
- rq->aux_inv_fixup = NULL;
- return 0;
}
@@ -123,6 +126,9 @@ int gen4_emit_flush_rcs(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen4 rcs */
- rq->aux_inv_fixup = NULL;
- return 0;
}
@@ -138,6 +144,9 @@ int gen4_emit_flush_vcs(struct i915_request *rq, u32 mode) *cs++ = MI_NOOP; intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen4 vcs */
- rq->aux_inv_fixup = NULL;
- return 0;
}
diff --git a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c index 5e65550b4dfb..efe51c4662fe 100644 --- a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c @@ -137,6 +137,9 @@ int gen6_emit_flush_rcs(struct i915_request *rq, u32 mode) *cs++ = 0; intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen6 */
- rq->aux_inv_fixup = NULL;
- return 0;
}
@@ -208,6 +211,9 @@ static int mi_flush_dw(struct i915_request *rq, u32 flags)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen6 */
- rq->aux_inv_fixup = NULL;
- return 0;
}
@@ -347,6 +353,9 @@ int gen7_emit_flush_rcs(struct i915_request *rq, u32 mode) *cs++ = 0; intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen7 rcs */
- rq->aux_inv_fixup = NULL;
- return 0;
}
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index b1b9c3fd7bf9..b6374cf53314 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -73,6 +73,9 @@ int gen8_emit_flush_rcs(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen8 rcs */
- rq->aux_inv_fixup = NULL;
- return 0;
}
@@ -106,6 +109,9 @@ int gen8_emit_flush_xcs(struct i915_request *rq, u32 mode) *cs++ = 0; /* value */ intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen8 xcs */
- rq->aux_inv_fixup = NULL;
- return 0;
}
@@ -157,6 +163,9 @@ int gen11_emit_flush_rcs(struct i915_request *rq, u32 mode) intel_ring_advance(rq, cs); }
- /* hsdes: 1809175790. No fixup needed for gen11 rcs */
- rq->aux_inv_fixup = NULL;
This is a little ugly to me. Can we just set this to 0 or 0xdeadbeef by default maybe and check that value below instead of retroactively adding all of these assignments?
- return 0;
}
@@ -165,30 +174,6 @@ static u32 preparser_disable(bool state) return MI_ARB_CHECK | 1 << 8 | state; }
-static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) -{
- static const i915_reg_t vd[] = {
GEN12_VD0_AUX_NV,GEN12_VD1_AUX_NV,GEN12_VD2_AUX_NV,GEN12_VD3_AUX_NV,- };
- static const i915_reg_t ve[] = {
GEN12_VE0_AUX_NV,GEN12_VE1_AUX_NV,- };
- if (engine->class == VIDEO_DECODE_CLASS)
return vd[engine->instance];- if (engine->class == VIDEO_ENHANCEMENT_CLASS)
return ve[engine->instance];- GEM_BUG_ON("unknown aux_inv reg\n");
- return INVALID_MMIO_REG;
-}
static u32 *gen12_emit_aux_table_inv(const i915_reg_t inv_reg, u32 *cs) { *cs++ = MI_LOAD_REGISTER_IMM(1); @@ -274,6 +259,9 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) intel_ring_advance(rq, cs); }
- /* hsdes: 1809175790. aux_inv has been done above, no fixup
needed */
- rq->aux_inv_fixup = NULL;
- return 0;
}
@@ -288,7 +276,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) if (mode & EMIT_INVALIDATE) aux_inv = rq->engine->mask & ~BIT(BCS0); if (aux_inv)
cmd += 2 * hweight32(aux_inv) + 2;
cmd += 4;cs = intel_ring_begin(rq, cmd); if (IS_ERR(cs))
@@ -319,16 +307,21 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) *cs++ = 0; /* value */
if (aux_inv) { /* hsdes: 1809175790 */
struct intel_engine_cs *engine;unsigned int tmp;*cs++ = MI_LOAD_REGISTER_IMM(hweight32(aux_inv));for_each_engine_masked(engine, rq->engine->gt, aux_inv,tmp) {
*cs++ =i915_mmio_reg_offset(aux_inv_reg(engine));
*cs++ = AUX_INV;}
/** We don't know which engine will eventually carry out* this request, so the mmio aux_inv register addressis
* unknown at this moment. We save the cs pointersupposed
* to hold the aux_inv address in rq->aux_inv_fixup andset
* it in execlists_dequeue() when the engine instance* carrying out this request becomes certain*/*cs++ = MI_LOAD_REGISTER_IMM(1);rq->aux_inv_fixup = cs; /* save the pointer to aux_inv*/
*cs++ = 0; /* mmio addr to be set at submission to HW*/
Maybe MI_NOOP instead?
*cs++ = AUX_INV;
- }
- } else
Can you add the brackets here on the else: } else { aux_inv_fixup = NULL }
Also good to run checkpatch. I see this showing up as a warning in the checkpatch results.
rq->aux_inv_fixup = NULL;if (mode & EMIT_INVALIDATE) *cs++ = preparser_disable(false);
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index e1470bb60f34..7e8552414275 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1258,6 +1258,34 @@ static bool completed(const struct i915_request *rq) return __i915_request_is_complete(rq); }
+static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) +{
- static const i915_reg_t vd[] = {
GEN12_VD0_AUX_NV,GEN12_VD1_AUX_NV,GEN12_VD2_AUX_NV,GEN12_VD3_AUX_NV,- };
- static const i915_reg_t ve[] = {
GEN12_VE0_AUX_NV,GEN12_VE1_AUX_NV,- };
- if (engine->class == VIDEO_DECODE_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(vd));return vd[engine->instance];- }
- if (engine->class == VIDEO_ENHANCEMENT_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(ve));return ve[engine->instance];- }
- GEM_BUG_ON("unknown aux_inv reg\n");
- return INVALID_MMIO_REG;
+}
static void execlists_dequeue(struct intel_engine_cs *engine)
So in the previous implementation, this "worked" for both execlists and guc submission. But how will this work now for GuC based submission? This flow and the address of the engine is owned by the GuC.
If we are going to say this is an execlist only requirement (e.g. platforms using GuC submission don't need this workaround), you should add an if (!using guc submission) in the sequence you added to the various emit_flush() routines above.
Thanks, Stuart
{ struct intel_engine_execlists * const execlists = &engine-
execlists;
@@ -1538,6 +1566,13 @@ static void execlists_dequeue(struct intel_engine_cs *engine) }
if (__i915_request_submit(rq)) {
/* hsdes: 1809175790 */if (rq->aux_inv_fixup) {*rq->aux_inv_fixup =i915_mmio_reg_offset(aux_inv_reg(engine));
rq->execution_mask = engine-mask;
} if (!merge) { *port++ =i915_request_get(last); last = NULL; diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index 28b1f9db5487..8e0764c43936 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -312,6 +312,8 @@ struct i915_request { struct i915_capture_list *capture_list; #endif
- u32 *aux_inv_fixup; /* hsdes: 1809175790 */
- /** Time at which this request was emitted, in jiffies. */ unsigned long emitted_jiffies;
@@ -157,6 +163,9 @@ int gen11_emit_flush_rcs(struct i915_request *rq, u32 mode) intel_ring_advance(rq, cs); }
- /* hsdes: 1809175790. No fixup needed for gen11 rcs */
- rq->aux_inv_fixup = NULL;
This is a little ugly to me. Can we just set this to 0 or 0xdeadbeef by default maybe and check that value below instead of retroactively adding all of these assignments?
The problem is there are many code paths that a i915_request could be allocated, I'm not aware of a unified routine where I could initialize the pointer for all i915_requests.
- return 0;
}
/** We don't know which engine will eventually carry out* this request, so the mmio aux_inv register addressis
* unknown at this moment. We save the cs pointersupposed
* to hold the aux_inv address in rq->aux_inv_fixup andset
* it in execlists_dequeue() when the engine instance* carrying out this request becomes certain*/*cs++ = MI_LOAD_REGISTER_IMM(1);rq->aux_inv_fixup = cs; /* save the pointer to aux_inv*/
*cs++ = 0; /* mmio addr to be set at submission to HW*/
Maybe MI_NOOP instead?
This is supposed to be the mmio address field for the MI_LOAD_REGISTER_IMM instruction, setting it to 0 makes more sense?
*cs++ = AUX_INV;
- }
- } else
Can you add the brackets here on the else: } else { aux_inv_fixup = NULL }
Also good to run checkpatch. I see this showing up as a warning in the checkpatch results.
I noticed the warning, will update.
rq->aux_inv_fixup = NULL;if (mode & EMIT_INVALIDATE) *cs++ = preparser_disable(false);
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index e1470bb60f34..7e8552414275 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1258,6 +1258,34 @@ static bool completed(const struct i915_request *rq) return __i915_request_is_complete(rq); }
+static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) {
- static const i915_reg_t vd[] = {
GEN12_VD0_AUX_NV,GEN12_VD1_AUX_NV,GEN12_VD2_AUX_NV,GEN12_VD3_AUX_NV,- };
- static const i915_reg_t ve[] = {
GEN12_VE0_AUX_NV,GEN12_VE1_AUX_NV,- };
- if (engine->class == VIDEO_DECODE_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(vd));return vd[engine->instance];- }
- if (engine->class == VIDEO_ENHANCEMENT_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(ve));return ve[engine->instance];- }
- GEM_BUG_ON("unknown aux_inv reg\n");
- return INVALID_MMIO_REG;
+}
static void execlists_dequeue(struct intel_engine_cs *engine)
So in the previous implementation, this "worked" for both execlists and guc submission. But how will this work now for GuC based submission? This flow and the address of the engine is owned by the GuC.
If we are going to say this is an execlist only requirement (e.g. platforms using GuC submission don't need this workaround), you should add an if (!using guc submission) in the sequence you added to the various emit_flush() routines above.
Good point. I didn't consider GuC submission because Chrome doesn't enable GuC for TGL. But it is true that the implementation will have problem with GuC submission. I'm not sure if it's possible for i915 to know which engine will eventually carry out the request because it might be scheduled by GuC. I will need to investigate.
Thanks, Stuart
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index e1470bb60f34..7e8552414275 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1258,6 +1258,34 @@ static bool completed(const struct i915_request *rq) return __i915_request_is_complete(rq); }
+static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) {
- static const i915_reg_t vd[] = {
GEN12_VD0_AUX_NV,GEN12_VD1_AUX_NV,GEN12_VD2_AUX_NV,GEN12_VD3_AUX_NV,- };
- static const i915_reg_t ve[] = {
GEN12_VE0_AUX_NV,GEN12_VE1_AUX_NV,- };
- if (engine->class == VIDEO_DECODE_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(vd));return vd[engine->instance];- }
- if (engine->class == VIDEO_ENHANCEMENT_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(ve));return ve[engine->instance];- }
- GEM_BUG_ON("unknown aux_inv reg\n");
- return INVALID_MMIO_REG;
+}
static void execlists_dequeue(struct intel_engine_cs *engine)
So in the previous implementation, this "worked" for both execlists and guc submission. But how will this work now for GuC based submission? This flow and the address of the engine is owned by the GuC.
If we are going to say this is an execlist only requirement (e.g. platforms using GuC submission don't need this workaround), you should add an if (!using guc submission) in the sequence you added to the various emit_flush() routines above.
Good point. I didn't consider GuC submission because Chrome doesn't enable GuC for TGL. But it is true that the implementation will have problem with GuC submission. I'm not sure if it's possible for i915 to know which engine will eventually carry out the request because it might be scheduled by GuC. I will need to investigate.
I think the same can be done in intel_guc_submission.c after __i915_request_submit(rq) calls.
Thanks, Stuart
On 04/03/2022 22:14, fei.yang@intel.com wrote:
From: Fei Yang fei.yang@intel.com
GPU hangs have been observed when multiple engines write to the same aux_inv register at the same time. To avoid this each engine should only invalidate its own auxiliary table. The function gen12_emit_flush_xcs() currently invalidate the auxiliary table for all engines because the rq->engine is not necessarily the engine eventually carrying out the request, and potentially the engine could even be a virtual one (with engine->instance being -1). With this patch, auxiliary table invalidation is done only for the engine executing the request. And the mmio address for the aux_inv register is set after the engine instance becomes certain.
Signed-off-by: Chris Wilson chris.p.wilson@intel.com Signed-off-by: Fei Yang fei.yang@intel.com
drivers/gpu/drm/i915/gt/gen2_engine_cs.c | 9 +++ drivers/gpu/drm/i915/gt/gen6_engine_cs.c | 9 +++ drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 61 ++++++++----------- .../drm/i915/gt/intel_execlists_submission.c | 35 +++++++++++ drivers/gpu/drm/i915/i915_request.h | 2 + 5 files changed, 82 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c index 1c82caf525c3..0ec4986e4805 100644 --- a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c @@ -37,6 +37,9 @@ int gen2_emit_flush(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen2 */
- rq->aux_inv_fixup = NULL;
Same thing that Stuart mentioned - would it not work for instance to initialize this in __i915_request_create?
- return 0; }
@@ -123,6 +126,9 @@ int gen4_emit_flush_rcs(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen4 rcs */
- rq->aux_inv_fixup = NULL;
- return 0; }
@@ -138,6 +144,9 @@ int gen4_emit_flush_vcs(struct i915_request *rq, u32 mode) *cs++ = MI_NOOP; intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen4 vcs */
- rq->aux_inv_fixup = NULL;
- return 0; }
diff --git a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c index 5e65550b4dfb..efe51c4662fe 100644 --- a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c @@ -137,6 +137,9 @@ int gen6_emit_flush_rcs(struct i915_request *rq, u32 mode) *cs++ = 0; intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen6 */
- rq->aux_inv_fixup = NULL;
- return 0; }
@@ -208,6 +211,9 @@ static int mi_flush_dw(struct i915_request *rq, u32 flags)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen6 */
- rq->aux_inv_fixup = NULL;
- return 0; }
@@ -347,6 +353,9 @@ int gen7_emit_flush_rcs(struct i915_request *rq, u32 mode) *cs++ = 0; intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen7 rcs */
- rq->aux_inv_fixup = NULL;
- return 0; }
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index b1b9c3fd7bf9..b6374cf53314 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -73,6 +73,9 @@ int gen8_emit_flush_rcs(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen8 rcs */
- rq->aux_inv_fixup = NULL;
- return 0; }
@@ -106,6 +109,9 @@ int gen8_emit_flush_xcs(struct i915_request *rq, u32 mode) *cs++ = 0; /* value */ intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen8 xcs */
- rq->aux_inv_fixup = NULL;
- return 0; }
@@ -157,6 +163,9 @@ int gen11_emit_flush_rcs(struct i915_request *rq, u32 mode) intel_ring_advance(rq, cs); }
- /* hsdes: 1809175790. No fixup needed for gen11 rcs */
- rq->aux_inv_fixup = NULL;
- return 0; }
@@ -165,30 +174,6 @@ static u32 preparser_disable(bool state) return MI_ARB_CHECK | 1 << 8 | state; }
-static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) -{
- static const i915_reg_t vd[] = {
GEN12_VD0_AUX_NV,GEN12_VD1_AUX_NV,GEN12_VD2_AUX_NV,GEN12_VD3_AUX_NV,- };
- static const i915_reg_t ve[] = {
GEN12_VE0_AUX_NV,GEN12_VE1_AUX_NV,- };
- if (engine->class == VIDEO_DECODE_CLASS)
return vd[engine->instance];- if (engine->class == VIDEO_ENHANCEMENT_CLASS)
return ve[engine->instance];- GEM_BUG_ON("unknown aux_inv reg\n");
- return INVALID_MMIO_REG;
-}
- static u32 *gen12_emit_aux_table_inv(const i915_reg_t inv_reg, u32 *cs) { *cs++ = MI_LOAD_REGISTER_IMM(1);
@@ -274,6 +259,9 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) intel_ring_advance(rq, cs); }
- /* hsdes: 1809175790. aux_inv has been done above, no fixup needed */
- rq->aux_inv_fixup = NULL;
- return 0; }
@@ -288,7 +276,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) if (mode & EMIT_INVALIDATE) aux_inv = rq->engine->mask & ~BIT(BCS0); if (aux_inv)
cmd += 2 * hweight32(aux_inv) + 2;
cmd += 4;cs = intel_ring_begin(rq, cmd); if (IS_ERR(cs))
@@ -319,16 +307,21 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) *cs++ = 0; /* value */
if (aux_inv) { /* hsdes: 1809175790 */
struct intel_engine_cs *engine;unsigned int tmp;*cs++ = MI_LOAD_REGISTER_IMM(hweight32(aux_inv));for_each_engine_masked(engine, rq->engine->gt, aux_inv, tmp) {*cs++ = i915_mmio_reg_offset(aux_inv_reg(engine));*cs++ = AUX_INV;}
/** We don't know which engine will eventually carry out* this request, so the mmio aux_inv register address is* unknown at this moment. We save the cs pointer supposed* to hold the aux_inv address in rq->aux_inv_fixup and set* it in execlists_dequeue() when the engine instance* carrying out this request becomes certain*/*cs++ = MI_LOAD_REGISTER_IMM(1);rq->aux_inv_fixup = cs; /* save the pointer to aux_inv */*cs++ = 0; /* mmio addr to be set at submission to HW */*cs++ = AUX_INV;
- }
} else
rq->aux_inv_fixup = NULL;if (mode & EMIT_INVALIDATE) *cs++ = preparser_disable(false);
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index e1470bb60f34..7e8552414275 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -1258,6 +1258,34 @@ static bool completed(const struct i915_request *rq) return __i915_request_is_complete(rq); }
+static i915_reg_t aux_inv_reg(const struct intel_engine_cs *engine) +{
- static const i915_reg_t vd[] = {
GEN12_VD0_AUX_NV,GEN12_VD1_AUX_NV,GEN12_VD2_AUX_NV,GEN12_VD3_AUX_NV,- };
- static const i915_reg_t ve[] = {
GEN12_VE0_AUX_NV,GEN12_VE1_AUX_NV,- };
- if (engine->class == VIDEO_DECODE_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(vd));return vd[engine->instance];- }
- if (engine->class == VIDEO_ENHANCEMENT_CLASS) {
GEM_BUG_ON(engine->instance >= ARRAY_SIZE(ve));return ve[engine->instance];- }
- GEM_BUG_ON("unknown aux_inv reg\n");
- return INVALID_MMIO_REG;
+}
- static void execlists_dequeue(struct intel_engine_cs *engine) { struct intel_engine_execlists * const execlists = &engine->execlists;
@@ -1538,6 +1566,13 @@ static void execlists_dequeue(struct intel_engine_cs *engine) }
if (__i915_request_submit(rq)) {
/* hsdes: 1809175790 */if (rq->aux_inv_fixup) {*rq->aux_inv_fixup =i915_mmio_reg_offset(aux_inv_reg(engine));rq->execution_mask = engine->mask;
Execution mask assignment needs a comment explaining what, why, and what are the consequences of fixing the mask here.
Also, from the top of my head for instance I can't remember why would aux_inv_fixup handling be just here and not in the first/virtual dequeue loop (the "while ... first_virtual_engine" one)? I thought the 2nd loop is non-virtual requests, but I am probably missing something.
} if (!merge) { *port++ = i915_request_get(last); last = NULL;diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index 28b1f9db5487..8e0764c43936 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -312,6 +312,8 @@ struct i915_request { struct i915_capture_list *capture_list; #endif
- u32 *aux_inv_fixup; /* hsdes: 1809175790 */
- /** Time at which this request was emitted, in jiffies. */ unsigned long emitted_jiffies;
Question of affected platforms and how will this be handled in case of GuC? I don't see how it can without the firmware involvement. Only if i915 would nerf the virtual engine setup so each GuC virtual context gets only one engine from the set, but that would be beyond horrible. Best to involved GuC people here.
Regards,
Tvrtko
diff --git a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c index 1c82caf525c3..0ec4986e4805 100644 --- a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c @@ -37,6 +37,9 @@ int gen2_emit_flush(struct i915_request *rq, u32 mode)
intel_ring_advance(rq, cs);
- /* hsdes: 1809175790. No fixup needed for gen2 */
- rq->aux_inv_fixup = NULL;
Same thing that Stuart mentioned - would it not work for instance to initialize this in __i915_request_create?
I didn't try __i915_request_create because there is code like the following in the driver, and I'm not sure how many such allocation is there. I will give it a shot. struct measure_breadcrumb { struct i915_request rq; struct intel_ring ring; u32 cs[2048]; };
static int measure_breadcrumb_dw(struct intel_context *ce) { struct intel_engine_cs *engine = ce->engine; struct measure_breadcrumb *frame; int dw;
GEM_BUG_ON(!engine->gt->scratch);
frame = kzalloc(sizeof(*frame), GFP_KERNEL); if (!frame) return -ENOMEM;
frame->rq.engine = engine; frame->rq.context = ce; ... }
- return 0; }
dri-devel@lists.freedesktop.org
-
fei.yang＠intel.com -
Summers, Stuart -
Tvrtko Ursulin -
Yang, Fei