* [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 10:47 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2019-10-31 10:47 UTC (permalink / raw)
To: intel-gfx
Check that the context's ring register state still matches our
expectations prior to execution.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
3 files changed, 63 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 5f61cd128d9c..666e70931524 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
i915_request_put(rq);
}
+static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
+{
+ if (INTEL_GEN(engine->i915) >= 12)
+ return 0x60;
+ else if (INTEL_GEN(engine->i915) >= 9)
+ return 0x54;
+ else if (engine->class == RENDER_CLASS)
+ return 0x58;
+ else
+ return -1;
+}
+
+static void
+execlists_check_context(const struct intel_context *ce,
+ const struct intel_engine_cs *engine)
+{
+ const struct intel_ring *ring = ce->ring;
+ u32 *regs = ce->lrc_reg_state;
+ int x;
+
+ if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
+ pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
+ engine->name,
+ regs[CTX_RING_START],
+ i915_ggtt_offset(ring->vma));
+ regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
+ }
+
+ if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
+ (RING_CTL_SIZE(ring->size) | RING_VALID)) {
+ pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
+ engine->name,
+ regs[CTX_RING_CTL],
+ (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
+ regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
+ }
+
+ if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
+ pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
+ engine->name,
+ regs[CTX_BB_STATE],
+ RING_BB_PPGTT);
+ regs[CTX_BB_STATE] = RING_BB_PPGTT;
+ }
+
+ x = lrc_ring_mi_mode(engine);
+ if (x != -1 && regs[x + 1] & STOP_RING) {
+ pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
+ engine->name, regs[x + 1]);
+ regs[x + 1] &= ~STOP_RING;
+ regs[x + 1] |= STOP_RING << 16;
+ }
+}
+
static u64 execlists_update_context(const struct i915_request *rq)
{
struct intel_context *ce = rq->context;
@@ -1154,6 +1208,9 @@ static u64 execlists_update_context(const struct i915_request *rq)
ce->lrc_reg_state[CTX_RING_TAIL] =
intel_ring_set_tail(rq->ring, rq->tail);
+ if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
+ execlists_check_context(ce, rq->engine);
+
/*
* Make sure the context image is complete before we submit it to HW.
*
@@ -2355,7 +2412,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
- regs[CTX_RING_BUFFER_START] = i915_ggtt_offset(ring->vma);
+ regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
regs[CTX_RING_HEAD] = ring->head;
regs[CTX_RING_TAIL] = ring->tail;
@@ -2942,18 +2999,6 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
&execlists->csb_status[reset_value]);
}
-static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
-{
- if (INTEL_GEN(engine->i915) >= 12)
- return 0x60;
- else if (INTEL_GEN(engine->i915) >= 9)
- return 0x54;
- else if (engine->class == RENDER_CLASS)
- return 0x58;
- else
- return -1;
-}
-
static void __execlists_reset_reg_state(const struct intel_context *ce,
const struct intel_engine_cs *engine)
{
@@ -3881,7 +3926,7 @@ static void init_common_reg_state(u32 * const regs,
_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT |
CTX_CTRL_RS_CTX_ENABLE);
- regs[CTX_RING_BUFFER_CONTROL] = RING_CTL_SIZE(ring->size) | RING_VALID;
+ regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
regs[CTX_BB_STATE] = RING_BB_PPGTT;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
index 06ab0276e10e..08a3be65f700 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
@@ -13,8 +13,8 @@
#define CTX_CONTEXT_CONTROL (0x02 + 1)
#define CTX_RING_HEAD (0x04 + 1)
#define CTX_RING_TAIL (0x06 + 1)
-#define CTX_RING_BUFFER_START (0x08 + 1)
-#define CTX_RING_BUFFER_CONTROL (0x0a + 1)
+#define CTX_RING_START (0x08 + 1)
+#define CTX_RING_CTL (0x0a + 1)
#define CTX_BB_STATE (0x10 + 1)
#define CTX_BB_PER_CTX_PTR (0x18 + 1)
#define CTX_PDP3_UDW (0x24 + 1)
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 04d0dd6a938a..7d3c7ca811b3 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -3178,12 +3178,12 @@ static int live_lrc_fixed(void *arg)
} tbl[] = {
{
i915_mmio_reg_offset(RING_START(engine->mmio_base)),
- CTX_RING_BUFFER_START - 1,
+ CTX_RING_START - 1,
"RING_START"
},
{
i915_mmio_reg_offset(RING_CTL(engine->mmio_base)),
- CTX_RING_BUFFER_CONTROL - 1,
+ CTX_RING_CTL - 1,
"RING_CTL"
},
{
--
2.24.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Intel-gfx] [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 10:47 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2019-10-31 10:47 UTC (permalink / raw)
To: intel-gfx
Check that the context's ring register state still matches our
expectations prior to execution.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
3 files changed, 63 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 5f61cd128d9c..666e70931524 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
i915_request_put(rq);
}
+static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
+{
+ if (INTEL_GEN(engine->i915) >= 12)
+ return 0x60;
+ else if (INTEL_GEN(engine->i915) >= 9)
+ return 0x54;
+ else if (engine->class == RENDER_CLASS)
+ return 0x58;
+ else
+ return -1;
+}
+
+static void
+execlists_check_context(const struct intel_context *ce,
+ const struct intel_engine_cs *engine)
+{
+ const struct intel_ring *ring = ce->ring;
+ u32 *regs = ce->lrc_reg_state;
+ int x;
+
+ if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
+ pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
+ engine->name,
+ regs[CTX_RING_START],
+ i915_ggtt_offset(ring->vma));
+ regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
+ }
+
+ if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
+ (RING_CTL_SIZE(ring->size) | RING_VALID)) {
+ pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
+ engine->name,
+ regs[CTX_RING_CTL],
+ (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
+ regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
+ }
+
+ if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
+ pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
+ engine->name,
+ regs[CTX_BB_STATE],
+ RING_BB_PPGTT);
+ regs[CTX_BB_STATE] = RING_BB_PPGTT;
+ }
+
+ x = lrc_ring_mi_mode(engine);
+ if (x != -1 && regs[x + 1] & STOP_RING) {
+ pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
+ engine->name, regs[x + 1]);
+ regs[x + 1] &= ~STOP_RING;
+ regs[x + 1] |= STOP_RING << 16;
+ }
+}
+
static u64 execlists_update_context(const struct i915_request *rq)
{
struct intel_context *ce = rq->context;
@@ -1154,6 +1208,9 @@ static u64 execlists_update_context(const struct i915_request *rq)
ce->lrc_reg_state[CTX_RING_TAIL] =
intel_ring_set_tail(rq->ring, rq->tail);
+ if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
+ execlists_check_context(ce, rq->engine);
+
/*
* Make sure the context image is complete before we submit it to HW.
*
@@ -2355,7 +2412,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
- regs[CTX_RING_BUFFER_START] = i915_ggtt_offset(ring->vma);
+ regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
regs[CTX_RING_HEAD] = ring->head;
regs[CTX_RING_TAIL] = ring->tail;
@@ -2942,18 +2999,6 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
&execlists->csb_status[reset_value]);
}
-static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
-{
- if (INTEL_GEN(engine->i915) >= 12)
- return 0x60;
- else if (INTEL_GEN(engine->i915) >= 9)
- return 0x54;
- else if (engine->class == RENDER_CLASS)
- return 0x58;
- else
- return -1;
-}
-
static void __execlists_reset_reg_state(const struct intel_context *ce,
const struct intel_engine_cs *engine)
{
@@ -3881,7 +3926,7 @@ static void init_common_reg_state(u32 * const regs,
_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT |
CTX_CTRL_RS_CTX_ENABLE);
- regs[CTX_RING_BUFFER_CONTROL] = RING_CTL_SIZE(ring->size) | RING_VALID;
+ regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
regs[CTX_BB_STATE] = RING_BB_PPGTT;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
index 06ab0276e10e..08a3be65f700 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
+++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
@@ -13,8 +13,8 @@
#define CTX_CONTEXT_CONTROL (0x02 + 1)
#define CTX_RING_HEAD (0x04 + 1)
#define CTX_RING_TAIL (0x06 + 1)
-#define CTX_RING_BUFFER_START (0x08 + 1)
-#define CTX_RING_BUFFER_CONTROL (0x0a + 1)
+#define CTX_RING_START (0x08 + 1)
+#define CTX_RING_CTL (0x0a + 1)
#define CTX_BB_STATE (0x10 + 1)
#define CTX_BB_PER_CTX_PTR (0x18 + 1)
#define CTX_PDP3_UDW (0x24 + 1)
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 04d0dd6a938a..7d3c7ca811b3 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -3178,12 +3178,12 @@ static int live_lrc_fixed(void *arg)
} tbl[] = {
{
i915_mmio_reg_offset(RING_START(engine->mmio_base)),
- CTX_RING_BUFFER_START - 1,
+ CTX_RING_START - 1,
"RING_START"
},
{
i915_mmio_reg_offset(RING_CTL(engine->mmio_base)),
- CTX_RING_BUFFER_CONTROL - 1,
+ CTX_RING_CTL - 1,
"RING_CTL"
},
{
--
2.24.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✗ Fi.CI.BUILD: failure for drm/i915/execlists: Verify context register state before execution (rev2)
@ 2019-10-31 10:51 ` Patchwork
0 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-10-31 10:51 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/execlists: Verify context register state before execution (rev2)
URL : https://patchwork.freedesktop.org/series/68599/
State : failure
== Summary ==
Applying: drm/i915/execlists: Verify context register state before execution
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/gt/intel_lrc.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915/execlists: Verify context register state before execution
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/execlists: Verify context register state before execution (rev2)
@ 2019-10-31 10:51 ` Patchwork
0 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-10-31 10:51 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/execlists: Verify context register state before execution (rev2)
URL : https://patchwork.freedesktop.org/series/68599/
State : failure
== Summary ==
Applying: drm/i915/execlists: Verify context register state before execution
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/gt/intel_lrc.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915/execlists: Verify context register state before execution
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 14:32 ` Mika Kuoppala
0 siblings, 0 replies; 10+ messages in thread
From: Mika Kuoppala @ 2019-10-31 14:32 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Check that the context's ring register state still matches our
> expectations prior to execution.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
> drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
> drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
> 3 files changed, 63 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 5f61cd128d9c..666e70931524 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
> i915_request_put(rq);
> }
>
> +static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> +{
> + if (INTEL_GEN(engine->i915) >= 12)
> + return 0x60;
> + else if (INTEL_GEN(engine->i915) >= 9)
> + return 0x54;
> + else if (engine->class == RENDER_CLASS)
> + return 0x58;
> + else
> + return -1;
> +}
> +
> +static void
> +execlists_check_context(const struct intel_context *ce,
> + const struct intel_engine_cs *engine)
> +{
> + const struct intel_ring *ring = ce->ring;
> + u32 *regs = ce->lrc_reg_state;
> + int x;
> +
> + if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
> + pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
> + engine->name,
> + regs[CTX_RING_START],
> + i915_ggtt_offset(ring->vma));
> + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
> + }
> +
> + if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
> + (RING_CTL_SIZE(ring->size) | RING_VALID)) {
> + pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
> + engine->name,
> + regs[CTX_RING_CTL],
> + (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
> + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
We are going to submit by clearing the waits. First I thought this will
lead to disaster but the hardware works so that we clear on writing '1'.
> + }
> +
> + if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
> + pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
> + engine->name,
> + regs[CTX_BB_STATE],
> + RING_BB_PPGTT);
> + regs[CTX_BB_STATE] = RING_BB_PPGTT;
> + }
> +
> + x = lrc_ring_mi_mode(engine);
> + if (x != -1 && regs[x + 1] & STOP_RING) {
> + pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
> + engine->name, regs[x + 1]);
> + regs[x + 1] &= ~STOP_RING;
> + regs[x + 1] |= STOP_RING << 16;
Ok you want only to care about this one. Was pondering of restoring rest
of the state from previous.
Will the hardware even care on masks at this point or is the saving part
setting them accordingly?
Not affecting this patch tho, just curious.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> + }
> +}
> +
> static u64 execlists_update_context(const struct i915_request *rq)
> {
> struct intel_context *ce = rq->context;
> @@ -1154,6 +1208,9 @@ static u64 execlists_update_context(const struct i915_request *rq)
> ce->lrc_reg_state[CTX_RING_TAIL] =
> intel_ring_set_tail(rq->ring, rq->tail);
>
> + if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> + execlists_check_context(ce, rq->engine);
> +
> /*
> * Make sure the context image is complete before we submit it to HW.
> *
> @@ -2355,7 +2412,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
> GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
>
> - regs[CTX_RING_BUFFER_START] = i915_ggtt_offset(ring->vma);
> + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
> regs[CTX_RING_HEAD] = ring->head;
> regs[CTX_RING_TAIL] = ring->tail;
>
> @@ -2942,18 +2999,6 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
> &execlists->csb_status[reset_value]);
> }
>
> -static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> -{
> - if (INTEL_GEN(engine->i915) >= 12)
> - return 0x60;
> - else if (INTEL_GEN(engine->i915) >= 9)
> - return 0x54;
> - else if (engine->class == RENDER_CLASS)
> - return 0x58;
> - else
> - return -1;
> -}
> -
> static void __execlists_reset_reg_state(const struct intel_context *ce,
> const struct intel_engine_cs *engine)
> {
> @@ -3881,7 +3926,7 @@ static void init_common_reg_state(u32 * const regs,
> _MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT |
> CTX_CTRL_RS_CTX_ENABLE);
>
> - regs[CTX_RING_BUFFER_CONTROL] = RING_CTL_SIZE(ring->size) | RING_VALID;
> + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
> regs[CTX_BB_STATE] = RING_BB_PPGTT;
> }
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> index 06ab0276e10e..08a3be65f700 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> @@ -13,8 +13,8 @@
> #define CTX_CONTEXT_CONTROL (0x02 + 1)
> #define CTX_RING_HEAD (0x04 + 1)
> #define CTX_RING_TAIL (0x06 + 1)
> -#define CTX_RING_BUFFER_START (0x08 + 1)
> -#define CTX_RING_BUFFER_CONTROL (0x0a + 1)
> +#define CTX_RING_START (0x08 + 1)
> +#define CTX_RING_CTL (0x0a + 1)
> #define CTX_BB_STATE (0x10 + 1)
> #define CTX_BB_PER_CTX_PTR (0x18 + 1)
> #define CTX_PDP3_UDW (0x24 + 1)
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 04d0dd6a938a..7d3c7ca811b3 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -3178,12 +3178,12 @@ static int live_lrc_fixed(void *arg)
> } tbl[] = {
> {
> i915_mmio_reg_offset(RING_START(engine->mmio_base)),
> - CTX_RING_BUFFER_START - 1,
> + CTX_RING_START - 1,
> "RING_START"
> },
> {
> i915_mmio_reg_offset(RING_CTL(engine->mmio_base)),
> - CTX_RING_BUFFER_CONTROL - 1,
> + CTX_RING_CTL - 1,
> "RING_CTL"
> },
> {
> --
> 2.24.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 14:32 ` Mika Kuoppala
0 siblings, 0 replies; 10+ messages in thread
From: Mika Kuoppala @ 2019-10-31 14:32 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Check that the context's ring register state still matches our
> expectations prior to execution.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
> drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
> drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
> 3 files changed, 63 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 5f61cd128d9c..666e70931524 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
> i915_request_put(rq);
> }
>
> +static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> +{
> + if (INTEL_GEN(engine->i915) >= 12)
> + return 0x60;
> + else if (INTEL_GEN(engine->i915) >= 9)
> + return 0x54;
> + else if (engine->class == RENDER_CLASS)
> + return 0x58;
> + else
> + return -1;
> +}
> +
> +static void
> +execlists_check_context(const struct intel_context *ce,
> + const struct intel_engine_cs *engine)
> +{
> + const struct intel_ring *ring = ce->ring;
> + u32 *regs = ce->lrc_reg_state;
> + int x;
> +
> + if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
> + pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
> + engine->name,
> + regs[CTX_RING_START],
> + i915_ggtt_offset(ring->vma));
> + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
> + }
> +
> + if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
> + (RING_CTL_SIZE(ring->size) | RING_VALID)) {
> + pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
> + engine->name,
> + regs[CTX_RING_CTL],
> + (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
> + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
We are going to submit by clearing the waits. First I thought this will
lead to disaster but the hardware works so that we clear on writing '1'.
> + }
> +
> + if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
> + pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
> + engine->name,
> + regs[CTX_BB_STATE],
> + RING_BB_PPGTT);
> + regs[CTX_BB_STATE] = RING_BB_PPGTT;
> + }
> +
> + x = lrc_ring_mi_mode(engine);
> + if (x != -1 && regs[x + 1] & STOP_RING) {
> + pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
> + engine->name, regs[x + 1]);
> + regs[x + 1] &= ~STOP_RING;
> + regs[x + 1] |= STOP_RING << 16;
Ok you want only to care about this one. Was pondering of restoring rest
of the state from previous.
Will the hardware even care on masks at this point or is the saving part
setting them accordingly?
Not affecting this patch tho, just curious.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> + }
> +}
> +
> static u64 execlists_update_context(const struct i915_request *rq)
> {
> struct intel_context *ce = rq->context;
> @@ -1154,6 +1208,9 @@ static u64 execlists_update_context(const struct i915_request *rq)
> ce->lrc_reg_state[CTX_RING_TAIL] =
> intel_ring_set_tail(rq->ring, rq->tail);
>
> + if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> + execlists_check_context(ce, rq->engine);
> +
> /*
> * Make sure the context image is complete before we submit it to HW.
> *
> @@ -2355,7 +2412,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
> GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
> GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
>
> - regs[CTX_RING_BUFFER_START] = i915_ggtt_offset(ring->vma);
> + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
> regs[CTX_RING_HEAD] = ring->head;
> regs[CTX_RING_TAIL] = ring->tail;
>
> @@ -2942,18 +2999,6 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
> &execlists->csb_status[reset_value]);
> }
>
> -static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> -{
> - if (INTEL_GEN(engine->i915) >= 12)
> - return 0x60;
> - else if (INTEL_GEN(engine->i915) >= 9)
> - return 0x54;
> - else if (engine->class == RENDER_CLASS)
> - return 0x58;
> - else
> - return -1;
> -}
> -
> static void __execlists_reset_reg_state(const struct intel_context *ce,
> const struct intel_engine_cs *engine)
> {
> @@ -3881,7 +3926,7 @@ static void init_common_reg_state(u32 * const regs,
> _MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT |
> CTX_CTRL_RS_CTX_ENABLE);
>
> - regs[CTX_RING_BUFFER_CONTROL] = RING_CTL_SIZE(ring->size) | RING_VALID;
> + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
> regs[CTX_BB_STATE] = RING_BB_PPGTT;
> }
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> index 06ab0276e10e..08a3be65f700 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> @@ -13,8 +13,8 @@
> #define CTX_CONTEXT_CONTROL (0x02 + 1)
> #define CTX_RING_HEAD (0x04 + 1)
> #define CTX_RING_TAIL (0x06 + 1)
> -#define CTX_RING_BUFFER_START (0x08 + 1)
> -#define CTX_RING_BUFFER_CONTROL (0x0a + 1)
> +#define CTX_RING_START (0x08 + 1)
> +#define CTX_RING_CTL (0x0a + 1)
> #define CTX_BB_STATE (0x10 + 1)
> #define CTX_BB_PER_CTX_PTR (0x18 + 1)
> #define CTX_PDP3_UDW (0x24 + 1)
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 04d0dd6a938a..7d3c7ca811b3 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -3178,12 +3178,12 @@ static int live_lrc_fixed(void *arg)
> } tbl[] = {
> {
> i915_mmio_reg_offset(RING_START(engine->mmio_base)),
> - CTX_RING_BUFFER_START - 1,
> + CTX_RING_START - 1,
> "RING_START"
> },
> {
> i915_mmio_reg_offset(RING_CTL(engine->mmio_base)),
> - CTX_RING_BUFFER_CONTROL - 1,
> + CTX_RING_CTL - 1,
> "RING_CTL"
> },
> {
> --
> 2.24.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 14:40 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2019-10-31 14:40 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Mika Kuoppala (2019-10-31 14:32:05)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Check that the context's ring register state still matches our
> > expectations prior to execution.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
> > drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
> > drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
> > 3 files changed, 63 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 5f61cd128d9c..666e70931524 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
> > i915_request_put(rq);
> > }
> >
> > +static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> > +{
> > + if (INTEL_GEN(engine->i915) >= 12)
> > + return 0x60;
> > + else if (INTEL_GEN(engine->i915) >= 9)
> > + return 0x54;
> > + else if (engine->class == RENDER_CLASS)
> > + return 0x58;
> > + else
> > + return -1;
> > +}
> > +
> > +static void
> > +execlists_check_context(const struct intel_context *ce,
> > + const struct intel_engine_cs *engine)
> > +{
> > + const struct intel_ring *ring = ce->ring;
> > + u32 *regs = ce->lrc_reg_state;
> > + int x;
> > +
> > + if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
> > + engine->name,
> > + regs[CTX_RING_START],
> > + i915_ggtt_offset(ring->vma));
> > + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
> > + }
> > +
> > + if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
> > + (RING_CTL_SIZE(ring->size) | RING_VALID)) {
> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
> > + engine->name,
> > + regs[CTX_RING_CTL],
> > + (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
> > + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
>
> We are going to submit by clearing the waits. First I thought this will
> lead to disaster but the hardware works so that we clear on writing '1'.
Afaik, they are only indicator bits. So the HW ignores those bits when
we write to the register.
> > + if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
> > + pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
> > + engine->name,
> > + regs[CTX_BB_STATE],
> > + RING_BB_PPGTT);
> > + regs[CTX_BB_STATE] = RING_BB_PPGTT;
> > + }
> > +
> > + x = lrc_ring_mi_mode(engine);
> > + if (x != -1 && regs[x + 1] & STOP_RING) {
> > + pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
> > + engine->name, regs[x + 1]);
> > + regs[x + 1] &= ~STOP_RING;
> > + regs[x + 1] |= STOP_RING << 16;
>
> Ok you want only to care about this one. Was pondering of restoring rest
> of the state from previous.
It was mostly in the spirit of "now that we are here, we might as well
fix it up". It's mainly the paranoia in trying to ascertain if we are
feeding garbage into the GPU. There's probably a lot more we can check,
but the ring registers are essential :)
> Will the hardware even care on masks at this point or is the saving part
> setting them accordingly?
Aiui, it uses the masks on the implicit LRI in the context restore to
control where or not to actually apply the bits. Not that I have checked
to see what the HW is doing (we will see if it ever flags an error), but
memory says from elsewere it uses 0xffffxxxx.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 14:40 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2019-10-31 14:40 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx
Quoting Mika Kuoppala (2019-10-31 14:32:05)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > Check that the context's ring register state still matches our
> > expectations prior to execution.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
> > drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
> > drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
> > 3 files changed, 63 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 5f61cd128d9c..666e70931524 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
> > i915_request_put(rq);
> > }
> >
> > +static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> > +{
> > + if (INTEL_GEN(engine->i915) >= 12)
> > + return 0x60;
> > + else if (INTEL_GEN(engine->i915) >= 9)
> > + return 0x54;
> > + else if (engine->class == RENDER_CLASS)
> > + return 0x58;
> > + else
> > + return -1;
> > +}
> > +
> > +static void
> > +execlists_check_context(const struct intel_context *ce,
> > + const struct intel_engine_cs *engine)
> > +{
> > + const struct intel_ring *ring = ce->ring;
> > + u32 *regs = ce->lrc_reg_state;
> > + int x;
> > +
> > + if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
> > + engine->name,
> > + regs[CTX_RING_START],
> > + i915_ggtt_offset(ring->vma));
> > + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
> > + }
> > +
> > + if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
> > + (RING_CTL_SIZE(ring->size) | RING_VALID)) {
> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
> > + engine->name,
> > + regs[CTX_RING_CTL],
> > + (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
> > + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
>
> We are going to submit by clearing the waits. First I thought this will
> lead to disaster but the hardware works so that we clear on writing '1'.
Afaik, they are only indicator bits. So the HW ignores those bits when
we write to the register.
> > + if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
> > + pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
> > + engine->name,
> > + regs[CTX_BB_STATE],
> > + RING_BB_PPGTT);
> > + regs[CTX_BB_STATE] = RING_BB_PPGTT;
> > + }
> > +
> > + x = lrc_ring_mi_mode(engine);
> > + if (x != -1 && regs[x + 1] & STOP_RING) {
> > + pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
> > + engine->name, regs[x + 1]);
> > + regs[x + 1] &= ~STOP_RING;
> > + regs[x + 1] |= STOP_RING << 16;
>
> Ok you want only to care about this one. Was pondering of restoring rest
> of the state from previous.
It was mostly in the spirit of "now that we are here, we might as well
fix it up". It's mainly the paranoia in trying to ascertain if we are
feeding garbage into the GPU. There's probably a lot more we can check,
but the ring registers are essential :)
> Will the hardware even care on masks at this point or is the saving part
> setting them accordingly?
Aiui, it uses the masks on the implicit LRI in the context restore to
control where or not to actually apply the bits. Not that I have checked
to see what the HW is doing (we will see if it ever flags an error), but
memory says from elsewere it uses 0xffffxxxx.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 14:52 ` Mika Kuoppala
0 siblings, 0 replies; 10+ messages in thread
From: Mika Kuoppala @ 2019-10-31 14:52 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2019-10-31 14:32:05)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>> > Check that the context's ring register state still matches our
>> > expectations prior to execution.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> > drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
>> > drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
>> > drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
>> > 3 files changed, 63 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > index 5f61cd128d9c..666e70931524 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > @@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
>> > i915_request_put(rq);
>> > }
>> >
>> > +static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
>> > +{
>> > + if (INTEL_GEN(engine->i915) >= 12)
>> > + return 0x60;
>> > + else if (INTEL_GEN(engine->i915) >= 9)
>> > + return 0x54;
>> > + else if (engine->class == RENDER_CLASS)
>> > + return 0x58;
>> > + else
>> > + return -1;
>> > +}
>> > +
>> > +static void
>> > +execlists_check_context(const struct intel_context *ce,
>> > + const struct intel_engine_cs *engine)
>> > +{
>> > + const struct intel_ring *ring = ce->ring;
>> > + u32 *regs = ce->lrc_reg_state;
>> > + int x;
>> > +
>> > + if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
>> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
>> > + engine->name,
>> > + regs[CTX_RING_START],
>> > + i915_ggtt_offset(ring->vma));
>> > + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
>> > + }
>> > +
>> > + if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
>> > + (RING_CTL_SIZE(ring->size) | RING_VALID)) {
>> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
>> > + engine->name,
>> > + regs[CTX_RING_CTL],
>> > + (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
>> > + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
>>
>> We are going to submit by clearing the waits. First I thought this will
>> lead to disaster but the hardware works so that we clear on writing '1'.
>
> Afaik, they are only indicator bits. So the HW ignores those bits when
> we write to the register.
>
I did check on reviewing. For Gen12 it states that: writing 0 == no
effect, writing 1 == clear.
-Mika
>> > + if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
>> > + pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
>> > + engine->name,
>> > + regs[CTX_BB_STATE],
>> > + RING_BB_PPGTT);
>> > + regs[CTX_BB_STATE] = RING_BB_PPGTT;
>> > + }
>> > +
>> > + x = lrc_ring_mi_mode(engine);
>> > + if (x != -1 && regs[x + 1] & STOP_RING) {
>> > + pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
>> > + engine->name, regs[x + 1]);
>> > + regs[x + 1] &= ~STOP_RING;
>> > + regs[x + 1] |= STOP_RING << 16;
>>
>> Ok you want only to care about this one. Was pondering of restoring rest
>> of the state from previous.
>
> It was mostly in the spirit of "now that we are here, we might as well
> fix it up". It's mainly the paranoia in trying to ascertain if we are
> feeding garbage into the GPU. There's probably a lot more we can check,
> but the ring registers are essential :)
>
>> Will the hardware even care on masks at this point or is the saving part
>> setting them accordingly?
>
> Aiui, it uses the masks on the implicit LRI in the context restore to
> control where or not to actually apply the bits. Not that I have checked
> to see what the HW is doing (we will see if it ever flags an error), but
> memory says from elsewere it uses 0xffffxxxx.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/execlists: Verify context register state before execution
@ 2019-10-31 14:52 ` Mika Kuoppala
0 siblings, 0 replies; 10+ messages in thread
From: Mika Kuoppala @ 2019-10-31 14:52 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
Chris Wilson <chris@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2019-10-31 14:32:05)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>> > Check that the context's ring register state still matches our
>> > expectations prior to execution.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> > drivers/gpu/drm/i915/gt/intel_lrc.c | 73 ++++++++++++++++++++-----
>> > drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 4 +-
>> > drivers/gpu/drm/i915/gt/selftest_lrc.c | 4 +-
>> > 3 files changed, 63 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > index 5f61cd128d9c..666e70931524 100644
>> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>> > @@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
>> > i915_request_put(rq);
>> > }
>> >
>> > +static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
>> > +{
>> > + if (INTEL_GEN(engine->i915) >= 12)
>> > + return 0x60;
>> > + else if (INTEL_GEN(engine->i915) >= 9)
>> > + return 0x54;
>> > + else if (engine->class == RENDER_CLASS)
>> > + return 0x58;
>> > + else
>> > + return -1;
>> > +}
>> > +
>> > +static void
>> > +execlists_check_context(const struct intel_context *ce,
>> > + const struct intel_engine_cs *engine)
>> > +{
>> > + const struct intel_ring *ring = ce->ring;
>> > + u32 *regs = ce->lrc_reg_state;
>> > + int x;
>> > +
>> > + if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
>> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
>> > + engine->name,
>> > + regs[CTX_RING_START],
>> > + i915_ggtt_offset(ring->vma));
>> > + regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
>> > + }
>> > +
>> > + if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
>> > + (RING_CTL_SIZE(ring->size) | RING_VALID)) {
>> > + pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
>> > + engine->name,
>> > + regs[CTX_RING_CTL],
>> > + (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
>> > + regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
>>
>> We are going to submit by clearing the waits. First I thought this will
>> lead to disaster but the hardware works so that we clear on writing '1'.
>
> Afaik, they are only indicator bits. So the HW ignores those bits when
> we write to the register.
>
I did check on reviewing. For Gen12 it states that: writing 0 == no
effect, writing 1 == clear.
-Mika
>> > + if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
>> > + pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
>> > + engine->name,
>> > + regs[CTX_BB_STATE],
>> > + RING_BB_PPGTT);
>> > + regs[CTX_BB_STATE] = RING_BB_PPGTT;
>> > + }
>> > +
>> > + x = lrc_ring_mi_mode(engine);
>> > + if (x != -1 && regs[x + 1] & STOP_RING) {
>> > + pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
>> > + engine->name, regs[x + 1]);
>> > + regs[x + 1] &= ~STOP_RING;
>> > + regs[x + 1] |= STOP_RING << 16;
>>
>> Ok you want only to care about this one. Was pondering of restoring rest
>> of the state from previous.
>
> It was mostly in the spirit of "now that we are here, we might as well
> fix it up". It's mainly the paranoia in trying to ascertain if we are
> feeding garbage into the GPU. There's probably a lot more we can check,
> but the ring registers are essential :)
>
>> Will the hardware even care on masks at this point or is the saving part
>> setting them accordingly?
>
> Aiui, it uses the masks on the implicit LRI in the context restore to
> control where or not to actually apply the bits. Not that I have checked
> to see what the HW is doing (we will see if it ever flags an error), but
> memory says from elsewere it uses 0xffffxxxx.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-31 14:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 10:47 [PATCH] drm/i915/execlists: Verify context register state before execution Chris Wilson
2019-10-31 10:47 ` [Intel-gfx] " Chris Wilson
2019-10-31 10:51 ` ✗ Fi.CI.BUILD: failure for drm/i915/execlists: Verify context register state before execution (rev2) Patchwork
2019-10-31 10:51 ` [Intel-gfx] " Patchwork
2019-10-31 14:32 ` [PATCH] drm/i915/execlists: Verify context register state before execution Mika Kuoppala
2019-10-31 14:32 ` [Intel-gfx] " Mika Kuoppala
2019-10-31 14:40 ` Chris Wilson
2019-10-31 14:40 ` [Intel-gfx] " Chris Wilson
2019-10-31 14:52 ` Mika Kuoppala
2019-10-31 14:52 ` [Intel-gfx] " Mika Kuoppala
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.