On 18/06/2019 14:43, John Harrison wrote:
On 6/17/2019 23:42, Tvrtko Ursulin wrote:
On 18/06/2019 02:01, John.C.Harrison@xxxxxxxxx wrote:
From: "Robert M. Fosha" <robert.m.fosha@xxxxxxxxx>
Updates the live_workarounds selftest to handle whitelisted
registers that are flagged as read only.
Signed-off-by: Robert M. Fosha <robert.m.fosha@xxxxxxxxx>
Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
.../gpu/drm/i915/gt/selftest_workarounds.c | 43 +++++++++++++++++--
1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index c8d335d63f9c..eb6d3aa2c8cc 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs
*engine, u32 reg)
return false;
}
+static bool ro_register(u32 reg)
+{
+ if (reg & RING_FORCE_TO_NONPRIV_RD)
+ return true;
+
+ return false;
+}
+
+static int whitelist_writable_count(struct intel_engine_cs *engine)
+{
+ int count = engine->whitelist.count;
+ int i;
+
+ for (i = 0; i < engine->whitelist.count; i++) {
+ u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+ if (ro_register(reg))
+ count--;
+ }
+
+ return count;
+}
+
static int check_dirty_whitelist(struct i915_gem_context *ctx,
struct intel_engine_cs *engine)
{
@@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct
i915_gem_context *ctx,
if (wo_register(engine, reg))
continue;
+ if (ro_register(reg))
+ continue;
+
srm = MI_STORE_REGISTER_MEM;
lrm = MI_LOAD_REGISTER_MEM;
if (INTEL_GEN(ctx->i915) >= 8)
@@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct
i915_gem_context *ctx,
for (i = 0; i < engine->whitelist.count; i++) {
u64 offset = results->node.start + sizeof(u32) * i;
+ u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+ /* Clear RD only and WR only flags */
+ reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
*cs++ = srm;
- *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+ *cs++ = reg;
*cs++ = lower_32_bits(offset);
*cs++ = upper_32_bits(offset);
}
@@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct
i915_gem_context *ctx,
goto err_batch;
}
- *cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
+ *cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
for (i = 0; i < engine->whitelist.count; i++) {
- *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+ u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+ if (ro_register(reg))
+ continue;
+
Are we not able to test the read-only property at all?
I am sure it would be possible to make such work. But can that wait
until the next round when we add support for ranges? And write only
access too if any registers actually use that and there is a way to test
that it really does do something?
I believe Robert was looking at getting something going but it wasn't
immediately working and we urgently need to get the HUC whitelist
updates merged to hit the next release window. So right now, it is
sufficient to say that the user land media driver works with these
changes and therefore the whitelisting must be working. The kernel
selftest is just a belt and braces check on top of that and therefore
can wait until later.
I don't agree that it is just belt and braces but in fact a way to check
that the thing really is read-only like it says on the tin. Lesson here
is that history keeps repeating panics which could have been avoided by
running the pre-existing tests during development.
However since I don't think there is any risk to driver or system
stability, even if the read-only property happened to be broken, which
it probably isn't, you can have a grumpy:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
