[Intel-gfx] [PATCH 1/2] drm/i915: Add mechanism to submit a context WA on ring submission
From: Mika Kuoppala
This patch adds framework to submit an arbitrary batchbuffer on each
context switch to clear residual state for render engine on Gen7/7.5
devices.
The idea of always emitting the context and vm setup around each request
is primary to make reset recovery easy, and not require rewriting the
ringbuffer. As each request would set up its own context, leaving it to
the HW to notice and elide no-op context switches, we could restart the
ring at any point, and reorder the requests freely.
However, to avoid emitting clear_residuals() between consecutive requests
in the ringbuffer of the same context, we do want to track the current
context in the ring. In doing so, we need to be careful to only record a
context switch when we are sure the next request will be emitted.
This security mitigation change does not trigger any performance
regression. Performance is on par with current mainline/drm-tip.
v2: Update vm_alias params to point to correct address space "vm" due to
changes made in the patch "f21613797bae98773"
v3-v4: none
Signed-off-by: Mika Kuoppala
Signed-off-by: Prathap Kumar Valsan
Signed-off-by: Akeem G Abodunrin
Cc: Chris Wilson
Cc: Balestrieri Francesco
Cc: Bloomfield Jon
Cc: Dutt Sudeep
---
.../gpu/drm/i915/gt/intel_ring_submission.c | 134 +-
1 file changed, 130 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index f70b903a98bc..593710558b99 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -1360,7 +1360,9 @@ static int load_pd_dir(struct i915_request *rq,
return rq->engine->emit_flush(rq, EMIT_FLUSH);
}
-static inline int mi_set_context(struct i915_request *rq, u32 flags)
+static inline int mi_set_context(struct i915_request *rq,
+struct intel_context *ce,
+u32 flags)
{
struct drm_i915_private *i915 = rq->i915;
struct intel_engine_cs *engine = rq->engine;
@@ -1435,7 +1437,7 @@ static inline int mi_set_context(struct i915_request *rq,
u32 flags)
*cs++ = MI_NOOP;
*cs++ = MI_SET_CONTEXT;
- *cs++ = i915_ggtt_offset(rq->context->state) | flags;
+ *cs++ = i915_ggtt_offset(ce->state) | flags;
/*
* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
* WaMiSetContext_Hang:snb,ivb,vlv
@@ -1550,13 +1552,56 @@ static int switch_mm(struct i915_request *rq, struct
i915_address_space *vm)
return rq->engine->emit_flush(rq, EMIT_INVALIDATE);
}
+static int clear_residuals(struct i915_request *rq)
+{
+ struct intel_engine_cs *engine = rq->engine;
+ int ret;
+
+ GEM_BUG_ON(!engine->kernel_context->state);
+
+ ret = switch_mm(rq, vm_alias(engine->kernel_context->vm));
+ if (ret)
+ return ret;
+
+ ret = mi_set_context(rq,
+engine->kernel_context,
+MI_MM_SPACE_GTT | MI_RESTORE_INHIBIT);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_bb_start(rq,
+ engine->wa_ctx.vma->node.start, 0,
+ 0);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_flush(rq, EMIT_FLUSH);
+ if (ret)
+ return ret;
+
+ /* Always invalidate before the next switch_mm() */
+ return engine->emit_flush(rq, EMIT_INVALIDATE);
+}
+
static int switch_context(struct i915_request *rq)
{
+ struct intel_engine_cs *engine = rq->engine;
struct intel_context *ce = rq->context;
+ void **residuals = NULL;
int ret;
GEM_BUG_ON(HAS_EXECLISTS(rq->i915));
+ if (engine->wa_ctx.vma && ce != engine->kernel_context) {
+ if (engine->wa_ctx.vma->private != ce) {
+ ret = clear_residuals(rq);
+ if (ret)
+ return ret;
+
+ residuals = >wa_ctx.vma->private;
+ }
+ }
+
ret = switch_mm(rq, vm_alias(ce->vm));
if (ret)
return ret;
@@ -1564,7 +1609,7 @@ static int switch_context(struct i915_request *rq)
if (ce->state) {
u32 flags;
- GEM_BUG_ON(rq->engine->id != RCS0);
+ GEM_BUG_ON(engine->id != RCS0);
/* For resource streamer on HSW+ and power context elsewhere */
BUILD_BUG_ON(HSW_MI_RS_SAVE_STATE_EN != MI_SAVE_EXT_STATE_EN);
@@ -1576,7 +1621,7 @@ static int switch_context(struct i915_request *rq)
else
flags |= MI_RESTORE_INHIBIT;
- ret = mi_set_context(rq, flags);
+ ret = mi_set_context(rq, ce, flags);
if (ret)
return ret;
}
@@ -1585,6 +1630,20 @@ static int
[Intel-gfx] [PATCH 1/2] drm/i915: Add mechanism to submit a context WA on ring submission
From: Mika Kuoppala
This patch adds framework to submit an arbitrary batchbuffer on each
context switch to clear residual state for render engine on Gen7/7.5
devices.
The idea of always emitting the context and vm setup around each request
is primary to make reset recovery easy, and not require rewriting the
ringbuffer. As each request would set up its own context, leaving it to
the HW to notice and elide no-op context switches, we could restart the
ring at any point, and reorder the requests freely.
However, to avoid emitting clear_residuals() between consecutive requests
in the ringbuffer of the same context, we do want to track the current
context in the ring. In doing so, we need to be careful to only record a
context switch when we are sure the next request will be emitted.
This security mitigation change does not trigger any performance
regression. Performance is on par with current mainline/drm-tip.
Signed-off-by: Mika Kuoppala
Signed-off-by: Prathap Kumar Valsan
Signed-off-by: Akeem G Abodunrin
Cc: Chris Wilson
Cc: Balestrieri Francesco
Cc: Bloomfield Jon
Cc: Dutt Sudeep
---
.../gpu/drm/i915/gt/intel_ring_submission.c | 132 +-
1 file changed, 129 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index 9aa86ba15ce7..9f3bfe499446 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -1385,7 +1385,9 @@ static int load_pd_dir(struct i915_request *rq,
return rq->engine->emit_flush(rq, EMIT_FLUSH);
}
-static inline int mi_set_context(struct i915_request *rq, u32 flags)
+static inline int mi_set_context(struct i915_request *rq,
+struct intel_context *ce,
+u32 flags)
{
struct drm_i915_private *i915 = rq->i915;
struct intel_engine_cs *engine = rq->engine;
@@ -1460,7 +1462,7 @@ static inline int mi_set_context(struct i915_request *rq,
u32 flags)
*cs++ = MI_NOOP;
*cs++ = MI_SET_CONTEXT;
- *cs++ = i915_ggtt_offset(rq->context->state) | flags;
+ *cs++ = i915_ggtt_offset(ce->state) | flags;
/*
* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
* WaMiSetContext_Hang:snb,ivb,vlv
@@ -1575,13 +1577,56 @@ static int switch_mm(struct i915_request *rq, struct
i915_address_space *vm)
return rq->engine->emit_flush(rq, EMIT_INVALIDATE);
}
+static int clear_residuals(struct i915_request *rq)
+{
+ struct intel_engine_cs *engine = rq->engine;
+ int ret;
+
+ GEM_BUG_ON(!engine->kernel_context->state);
+
+ ret = switch_mm(rq, vm_alias(engine->kernel_context));
+ if (ret)
+ return ret;
+
+ ret = mi_set_context(rq,
+engine->kernel_context,
+MI_MM_SPACE_GTT | MI_RESTORE_INHIBIT);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_bb_start(rq,
+ engine->wa_ctx.vma->node.start, 0,
+ 0);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_flush(rq, EMIT_FLUSH);
+ if (ret)
+ return ret;
+
+ /* Always invalidate before the next switch_mm() */
+ return engine->emit_flush(rq, EMIT_INVALIDATE);
+}
+
static int switch_context(struct i915_request *rq)
{
+ struct intel_engine_cs *engine = rq->engine;
struct intel_context *ce = rq->context;
+ void **residuals = NULL;
int ret;
GEM_BUG_ON(HAS_EXECLISTS(rq->i915));
+ if (engine->wa_ctx.vma && ce != engine->kernel_context) {
+ if (engine->wa_ctx.vma->private != ce) {
+ ret = clear_residuals(rq);
+ if (ret)
+ return ret;
+
+ residuals = >wa_ctx.vma->private;
+ }
+ }
+
ret = switch_mm(rq, vm_alias(ce));
if (ret)
return ret;
@@ -1601,7 +1646,7 @@ static int switch_context(struct i915_request *rq)
else
flags |= MI_RESTORE_INHIBIT;
- ret = mi_set_context(rq, flags);
+ ret = mi_set_context(rq, ce, flags);
if (ret)
return ret;
}
@@ -1610,6 +1655,20 @@ static int switch_context(struct i915_request *rq)
if (ret)
return ret;
+ /*
+* Now past the point of no return, this request _will_ be emitted.
+*
+* Or at least this preamble will be emitted, the request may be
+* interrupted prior to submitting the user payload. If so, we
+* still submit the "empty" request in order to preserve global
+* state tracking such as this, our tracking of the current
+* dirty context.
+*/
+ if (residuals) {
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Add mechanism to submit a context WA on ring submission
Mika Kuoppala writes: >Subject: Re: [PATCH 1/2] drm/i915: Add mechanism to submit a context WA >on ring submission I forgot to add RFC into patch subject. This should carry the RFC status as it is v2 on a RFC patch. This patch squashes Chris Wilson's ctx switch optimization and the development is still continuing. -Mika ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Add mechanism to submit a context WA on ring submission
This patch adds framework to submit an arbitrary batchbuffer on each
context switch to clear residual state for render engine on Gen7/7.5
devices.
The idea of always emitting the context and vm setup around each request
is primary to make reset recovery easy, and not require rewriting the
ringbuffer. As each request would set up its own context, leaving it to
the HW to notice and elide no-op context switches, we could restart the
ring at any point, and reorder the requests freely.
However, to avoid emitting clear_residuals() between consecutive requests
in the ringbuffer of the same context, we do want to track the current
context in the ring. In doing so, we need to be careful to only record a
context switch when we are sure the next request will be emitted.
v2: elide optimization patch squashed, courtesy of Chris Wilson
Signed-off-by: Mika Kuoppala
Signed-off-by: Akeem G Abodunrin
Cc: Kumar Valsan Prathap
Cc: Chris Wilson
Cc: Balestrieri Francesco
Cc: Bloomfield Jon
Cc: Dutt Sudeep
---
.../gpu/drm/i915/gt/intel_ring_submission.c | 132 +-
1 file changed, 129 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index bc44fe8e5ffa..58500032c993 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -1384,7 +1384,9 @@ static int load_pd_dir(struct i915_request *rq,
return rq->engine->emit_flush(rq, EMIT_FLUSH);
}
-static inline int mi_set_context(struct i915_request *rq, u32 flags)
+static inline int mi_set_context(struct i915_request *rq,
+struct intel_context *ce,
+u32 flags)
{
struct drm_i915_private *i915 = rq->i915;
struct intel_engine_cs *engine = rq->engine;
@@ -1459,7 +1461,7 @@ static inline int mi_set_context(struct i915_request *rq,
u32 flags)
*cs++ = MI_NOOP;
*cs++ = MI_SET_CONTEXT;
- *cs++ = i915_ggtt_offset(rq->context->state) | flags;
+ *cs++ = i915_ggtt_offset(ce->state) | flags;
/*
* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
* WaMiSetContext_Hang:snb,ivb,vlv
@@ -1574,13 +1576,56 @@ static int switch_mm(struct i915_request *rq, struct
i915_address_space *vm)
return rq->engine->emit_flush(rq, EMIT_INVALIDATE);
}
+static int clear_residuals(struct i915_request *rq)
+{
+ struct intel_engine_cs *engine = rq->engine;
+ int ret;
+
+ GEM_BUG_ON(!engine->kernel_context->state);
+
+ ret = switch_mm(rq, vm_alias(engine->kernel_context));
+ if (ret)
+ return ret;
+
+ ret = mi_set_context(rq,
+engine->kernel_context,
+MI_MM_SPACE_GTT | MI_RESTORE_INHIBIT);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_bb_start(rq,
+ engine->wa_ctx.vma->node.start, 0,
+ 0);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_flush(rq, EMIT_FLUSH);
+ if (ret)
+ return ret;
+
+ /* Always invalidate before the next switch_mm() */
+ return engine->emit_flush(rq, EMIT_INVALIDATE);
+}
+
static int switch_context(struct i915_request *rq)
{
+ struct intel_engine_cs *engine = rq->engine;
struct intel_context *ce = rq->context;
+ void **residuals = NULL;
int ret;
GEM_BUG_ON(HAS_EXECLISTS(rq->i915));
+ if (engine->wa_ctx.vma && ce != engine->kernel_context) {
+ if (engine->wa_ctx.vma->private != ce) {
+ ret = clear_residuals(rq);
+ if (ret)
+ return ret;
+
+ residuals = >wa_ctx.vma->private;
+ }
+ }
+
ret = switch_mm(rq, vm_alias(ce));
if (ret)
return ret;
@@ -1600,7 +1645,7 @@ static int switch_context(struct i915_request *rq)
else
flags |= MI_RESTORE_INHIBIT;
- ret = mi_set_context(rq, flags);
+ ret = mi_set_context(rq, ce, flags);
if (ret)
return ret;
}
@@ -1609,6 +1654,20 @@ static int switch_context(struct i915_request *rq)
if (ret)
return ret;
+ /*
+* Now past the point of no return, this request _will_ be emitted.
+*
+* Or at least this preamble will be emitted, the request may be
+* interrupted prior to submitting the user payload. If so, we
+* still submit the "empty" request in order to preserve global
+* state tracking such as this, our tracking of the current
+* dirty context.
+*/
+ if (residuals) {
+ intel_context_put(*residuals);
+ *residuals = intel_context_get(ce);
[Intel-gfx] [PATCH 1/2] drm/i915: Add mechanism to submit a context WA on ring submission
This patch adds framework to submit an arbitrary batchbuffer on each
context switch to clear residual state for render engine on Gen7/7.5
devices.
The idea of always emitting the context and vm setup around each request
is primary to make reset recovery easy, and not require rewriting the
ringbuffer. As each request would set up its own context, leaving it to
the HW to notice and elide no-op context switches, we could restart the
ring at any point, and reorder the requests freely.
However, to avoid emitting clear_residuals() between consecutive requests
in the ringbuffer of the same context, we do want to track the current
context in the ring. In doing so, we need to be careful to only record a
context switch when we are sure the next request will be emitted.
v2: elide optimization patch squashed, courtesy of Chris Wilson
Signed-off-by: Mika Kuoppala
Signed-off-by: Akeem G Abodunrin
Cc: Kumar Valsan Prathap
Cc: Chris Wilson
Cc: Balestrieri Francesco
Cc: Bloomfield Jon
Cc: Dutt Sudeep
---
.../gpu/drm/i915/gt/intel_ring_submission.c | 132 +-
1 file changed, 129 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
index bc44fe8e5ffa..58500032c993 100644
--- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
@@ -1384,7 +1384,9 @@ static int load_pd_dir(struct i915_request *rq,
return rq->engine->emit_flush(rq, EMIT_FLUSH);
}
-static inline int mi_set_context(struct i915_request *rq, u32 flags)
+static inline int mi_set_context(struct i915_request *rq,
+struct intel_context *ce,
+u32 flags)
{
struct drm_i915_private *i915 = rq->i915;
struct intel_engine_cs *engine = rq->engine;
@@ -1459,7 +1461,7 @@ static inline int mi_set_context(struct i915_request *rq,
u32 flags)
*cs++ = MI_NOOP;
*cs++ = MI_SET_CONTEXT;
- *cs++ = i915_ggtt_offset(rq->context->state) | flags;
+ *cs++ = i915_ggtt_offset(ce->state) | flags;
/*
* w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
* WaMiSetContext_Hang:snb,ivb,vlv
@@ -1574,13 +1576,56 @@ static int switch_mm(struct i915_request *rq, struct
i915_address_space *vm)
return rq->engine->emit_flush(rq, EMIT_INVALIDATE);
}
+static int clear_residuals(struct i915_request *rq)
+{
+ struct intel_engine_cs *engine = rq->engine;
+ int ret;
+
+ GEM_BUG_ON(!engine->kernel_context->state);
+
+ ret = switch_mm(rq, vm_alias(engine->kernel_context));
+ if (ret)
+ return ret;
+
+ ret = mi_set_context(rq,
+engine->kernel_context,
+MI_MM_SPACE_GTT | MI_RESTORE_INHIBIT);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_bb_start(rq,
+ engine->wa_ctx.vma->node.start, 0,
+ 0);
+ if (ret)
+ return ret;
+
+ ret = engine->emit_flush(rq, EMIT_FLUSH);
+ if (ret)
+ return ret;
+
+ /* Always invalidate before the next switch_mm() */
+ return engine->emit_flush(rq, EMIT_INVALIDATE);
+}
+
static int switch_context(struct i915_request *rq)
{
+ struct intel_engine_cs *engine = rq->engine;
struct intel_context *ce = rq->context;
+ void **residuals = NULL;
int ret;
GEM_BUG_ON(HAS_EXECLISTS(rq->i915));
+ if (engine->wa_ctx.vma && ce != engine->kernel_context) {
+ if (engine->wa_ctx.vma->private != ce) {
+ ret = clear_residuals(rq);
+ if (ret)
+ return ret;
+
+ residuals = >wa_ctx.vma->private;
+ }
+ }
+
ret = switch_mm(rq, vm_alias(ce));
if (ret)
return ret;
@@ -1600,7 +1645,7 @@ static int switch_context(struct i915_request *rq)
else
flags |= MI_RESTORE_INHIBIT;
- ret = mi_set_context(rq, flags);
+ ret = mi_set_context(rq, ce, flags);
if (ret)
return ret;
}
@@ -1609,6 +1654,20 @@ static int switch_context(struct i915_request *rq)
if (ret)
return ret;
+ /*
+* Now past the point of no return, this request _will_ be emitted.
+*
+* Or at least this preamble will be emitted, the request may be
+* interrupted prior to submitting the user payload. If so, we
+* still submit the "empty" request in order to preserve global
+* state tracking such as this, our tracking of the current
+* dirty context.
+*/
+ if (residuals) {
+ intel_context_put(*residuals);
+ *residuals = intel_context_get(ce);
