[PATCH v2 1/4] drm/i915: Verify workarounds immediately after application

[PATCH v2 1/4] drm/i915: Verify workarounds immediately after application

[Chris Wilson]

Immediately after writing the workaround, verify that it stuck in the
register.

References: https://bugs.freedesktop.org/show_bug.cgi?id=108954
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_workarounds.c | 32 +++++++++++++-----------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
index ccaf63679435..1c54b5030807 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -913,6 +913,20 @@ wal_get_fw_for_rmw(struct intel_uncore *uncore, const struct i915_wa_list *wal)
 	return fw;
 }
 
+static bool
+wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
+{
+	if ((cur ^ wa->val) & wa->mask) {
+		DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
+			  name, from, i915_mmio_reg_offset(wa->reg), cur,
+			  cur & wa->mask, wa->val, wa->mask);
+
+		return false;
+	}
+
+	return true;
+}
+
 static void
 wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
 {
@@ -931,6 +945,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
 
 	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
 		intel_uncore_rmw_fw(uncore, wa->reg, wa->mask, wa->val);
+		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
+			wa_verify(wa,
+				  intel_uncore_read_fw(uncore, wa->reg),
+				  wal->name, "applictation");
 	}
 
 	intel_uncore_forcewake_put__locked(uncore, fw);
@@ -942,20 +960,6 @@ void intel_gt_apply_workarounds(struct drm_i915_private *i915)
 	wa_list_apply(&i915->uncore, &i915->gt_wa_list);
 }
 
-static bool
-wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
-{
-	if ((cur ^ wa->val) & wa->mask) {
-		DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
-			  name, from, i915_mmio_reg_offset(wa->reg), cur,
-			  cur & wa->mask, wa->val, wa->mask);
-
-		return false;
-	}
-
-	return true;
-}
-
 static bool wa_list_verify(struct intel_uncore *uncore,
 			   const struct i915_wa_list *wal,
 			   const char *from)
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/4] drm/i915: Verify the engine workarounds stick on application

[Chris Wilson]

Read the engine workarounds back using the GPU after loading the initial
context state to verify that we are setting them correctly, and bail if
it fails.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c               |   6 +
 drivers/gpu/drm/i915/intel_workarounds.c      | 120 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_workarounds.h      |   2 +
 .../drm/i915/selftests/intel_workarounds.c    |  53 +-------
 4 files changed, 134 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0a818a60ad31..95ae69753e91 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4717,6 +4717,12 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 		i915_request_add(rq);
 		if (err)
 			goto err_active;
+
+		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) &&
+		    intel_engine_verify_workarounds(engine, "load")) {
+			err = -EIO;
+			goto err_active;
+		}
 	}
 
 	/* Flush the default context image to memory, and enable powersaving. */
diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
index 1c54b5030807..db99f2e676bb 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -1259,6 +1259,126 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
 	wa_list_apply(engine->uncore, &engine->wa_list);
 }
 
+static struct i915_vma *
+create_scratch(struct i915_address_space *vm, int count)
+{
+	struct drm_i915_gem_object *obj;
+	struct i915_vma *vma;
+	unsigned int size;
+	int err;
+
+	size = round_up(count * 4, PAGE_SIZE);
+	obj = i915_gem_object_create_internal(vm->i915, size);
+	if (IS_ERR(obj))
+		return ERR_CAST(obj);
+
+	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
+
+	vma = i915_vma_instance(obj, vm, NULL);
+	if (IS_ERR(vma)) {
+		err = PTR_ERR(vma);
+		goto err_obj;
+	}
+
+	err = i915_vma_pin(vma, 0, 0,
+			   i915_vma_is_ggtt(vma) ? PIN_GLOBAL : PIN_USER);
+	if (err)
+		goto err_obj;
+
+	return vma;
+
+err_obj:
+	i915_gem_object_put(obj);
+	return ERR_PTR(err);
+}
+
+static int
+wa_list_srm(struct i915_request *rq,
+	    const struct i915_wa_list *wal,
+	    struct i915_vma *vma)
+{
+	const struct i915_wa *wa;
+	u32 srm, *cs;
+	int i;
+
+	srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
+	if (INTEL_GEN(rq->i915) >= 8)
+		srm++;
+
+	cs = intel_ring_begin(rq, 4 * wal->count);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
+		*cs++ = srm;
+		*cs++ = i915_mmio_reg_offset(wa->reg);
+		*cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
+		*cs++ = 0;
+	}
+	intel_ring_advance(rq, cs);
+
+	return 0;
+}
+
+static int engine_wa_list_verify(struct intel_engine_cs *engine,
+				 const struct i915_wa_list * const wal,
+				 const char *from)
+{
+	const struct i915_wa *wa;
+	struct i915_request *rq;
+	struct i915_vma *vma;
+	unsigned int i;
+	u32 *results;
+	int err;
+
+	if (!wal->count)
+		return 0;
+
+	vma = create_scratch(&engine->i915->ggtt.vm, wal->count);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	rq = i915_request_alloc(engine, engine->kernel_context->gem_context);
+	if (IS_ERR(rq)) {
+		err = PTR_ERR(rq);
+		goto err_vma;
+	}
+
+	err = wa_list_srm(rq, wal, vma);
+	if (err)
+		goto err_vma;
+
+	i915_request_add(rq);
+	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0) {
+		err = -ETIME;
+		goto err_vma;
+	}
+
+	results = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+	if (IS_ERR(results)) {
+		err = PTR_ERR(results);
+		goto err_vma;
+	}
+
+	err = 0;
+	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
+		if (!wa_verify(wa, results[i], wal->name, from))
+			err = -ENXIO;
+
+	i915_gem_object_unpin_map(vma->obj);
+
+err_vma:
+	i915_vma_unpin(vma);
+	i915_vma_put(vma);
+	return err;
+}
+
+int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
+				    const char *from)
+{
+	return engine_wa_list_verify(engine, &engine->wa_list, from);
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/intel_workarounds.c"
 #endif
diff --git a/drivers/gpu/drm/i915/intel_workarounds.h b/drivers/gpu/drm/i915/intel_workarounds.h
index 34eee5ec511e..fdf7ebb90f28 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.h
+++ b/drivers/gpu/drm/i915/intel_workarounds.h
@@ -30,5 +30,7 @@ void intel_engine_apply_whitelist(struct intel_engine_cs *engine);
 
 void intel_engine_init_workarounds(struct intel_engine_cs *engine);
 void intel_engine_apply_workarounds(struct intel_engine_cs *engine);
+int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
+				    const char *from);
 
 #endif
diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
index 567b6f8dae86..a363748a7a4f 100644
--- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
@@ -340,49 +340,6 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine,
 	return err;
 }
 
-static struct i915_vma *create_scratch(struct i915_gem_context *ctx)
-{
-	struct drm_i915_gem_object *obj;
-	struct i915_vma *vma;
-	void *ptr;
-	int err;
-
-	obj = i915_gem_object_create_internal(ctx->i915, PAGE_SIZE);
-	if (IS_ERR(obj))
-		return ERR_CAST(obj);
-
-	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
-
-	ptr = i915_gem_object_pin_map(obj, I915_MAP_WB);
-	if (IS_ERR(ptr)) {
-		err = PTR_ERR(ptr);
-		goto err_obj;
-	}
-	memset(ptr, 0xc5, PAGE_SIZE);
-	i915_gem_object_flush_map(obj);
-	i915_gem_object_unpin_map(obj);
-
-	vma = i915_vma_instance(obj, &ctx->ppgtt->vm, NULL);
-	if (IS_ERR(vma)) {
-		err = PTR_ERR(vma);
-		goto err_obj;
-	}
-
-	err = i915_vma_pin(vma, 0, 0, PIN_USER);
-	if (err)
-		goto err_obj;
-
-	err = i915_gem_object_set_to_cpu_domain(obj, false);
-	if (err)
-		goto err_obj;
-
-	return vma;
-
-err_obj:
-	i915_gem_object_put(obj);
-	return ERR_PTR(err);
-}
-
 static struct i915_vma *create_batch(struct i915_gem_context *ctx)
 {
 	struct drm_i915_gem_object *obj;
@@ -475,7 +432,7 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
 	int err = 0, i, v;
 	u32 *cs, *results;
 
-	scratch = create_scratch(ctx);
+	scratch = create_scratch(&ctx->ppgtt->vm, 2 * ARRAY_SIZE(values) + 1);
 	if (IS_ERR(scratch))
 		return PTR_ERR(scratch);
 
@@ -752,9 +709,11 @@ static bool verify_gt_engine_wa(struct drm_i915_private *i915,
 
 	ok &= wa_list_verify(&i915->uncore, &lists->gt_wa_list, str);
 
-	for_each_engine(engine, i915, id)
-		ok &= wa_list_verify(engine->uncore,
-				     &lists->engine[id].wa_list, str);
+	for_each_engine(engine, i915, id) {
+		ok &= engine_wa_list_verify(engine,
+					    &lists->engine[id].wa_list,
+					    str) == 0;
+	}
 
 	return ok;
 }
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/4] drm/i915: Make workaround verification *optional*

[Chris Wilson]

Sometimes the HW doesn't even play fair, and completely forgets about
register writes. Skip verifying known troublemakers.

References: https://bugs.freedesktop.org/show_bug.cgi?id=108954
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_workarounds.c      | 40 ++++++++++++++-----
 .../gpu/drm/i915/intel_workarounds_types.h    |  7 ++--
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
index db99f2e676bb..ba58be05f58c 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -122,6 +122,7 @@ static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
 			wal->wa_count++;
 			wa_->val |= wa->val;
 			wa_->mask |= wa->mask;
+			wa_->read |= wa->read;
 			return;
 		}
 	}
@@ -146,9 +147,10 @@ wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
 		   u32 val)
 {
 	struct i915_wa wa = {
-		.reg = reg,
+		.reg  = reg,
 		.mask = mask,
-		.val = val
+		.val  = val,
+		.read = mask,
 	};
 
 	_wa_add(wal, &wa);
@@ -172,6 +174,19 @@ wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
 	wa_write_masked_or(wal, reg, val, val);
 }
 
+static void
+ignore_wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask, u32 val)
+{
+	struct i915_wa wa = {
+		.reg  = reg,
+		.mask = mask,
+		.val  = val,
+		/* Bonkers HW, skip verifying */
+	};
+
+	_wa_add(wal, &wa);
+}
+
 #define WA_SET_BIT_MASKED(addr, mask) \
 	wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_ENABLE(mask))
 
@@ -916,10 +931,11 @@ wal_get_fw_for_rmw(struct intel_uncore *uncore, const struct i915_wa_list *wal)
 static bool
 wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
 {
-	if ((cur ^ wa->val) & wa->mask) {
+	if ((cur ^ wa->val) & wa->read) {
 		DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
-			  name, from, i915_mmio_reg_offset(wa->reg), cur,
-			  cur & wa->mask, wa->val, wa->mask);
+			  name, from, i915_mmio_reg_offset(wa->reg),
+			  cur, cur & wa->read,
+			  wa->val, wa->mask);
 
 		return false;
 	}
@@ -1122,9 +1138,10 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 			     _3D_CHICKEN3_AA_LINE_QUALITY_FIX_ENABLE);
 
 		/* WaPipelineFlushCoherentLines:icl */
-		wa_write_or(wal,
-			    GEN8_L3SQCREG4,
-			    GEN8_LQSC_FLUSH_COHERENT_LINES);
+		ignore_wa_write_or(wal,
+				   GEN8_L3SQCREG4,
+				   GEN8_LQSC_FLUSH_COHERENT_LINES,
+				   GEN8_LQSC_FLUSH_COHERENT_LINES);
 
 		/*
 		 * Wa_1405543622:icl
@@ -1151,9 +1168,10 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
 		 * Wa_1405733216:icl
 		 * Formerly known as WaDisableCleanEvicts
 		 */
-		wa_write_or(wal,
-			    GEN8_L3SQCREG4,
-			    GEN11_LQSC_CLEAN_EVICT_DISABLE);
+		ignore_wa_write_or(wal,
+				   GEN8_L3SQCREG4,
+				   GEN11_LQSC_CLEAN_EVICT_DISABLE,
+				   GEN11_LQSC_CLEAN_EVICT_DISABLE);
 
 		/* WaForwardProgressSoftReset:icl */
 		wa_write_or(wal,
diff --git a/drivers/gpu/drm/i915/intel_workarounds_types.h b/drivers/gpu/drm/i915/intel_workarounds_types.h
index 30918da180ff..42ac1fb99572 100644
--- a/drivers/gpu/drm/i915/intel_workarounds_types.h
+++ b/drivers/gpu/drm/i915/intel_workarounds_types.h
@@ -12,9 +12,10 @@
 #include "i915_reg.h"
 
 struct i915_wa {
-	i915_reg_t	  reg;
-	u32		  mask;
-	u32		  val;
+	i915_reg_t	reg;
+	u32		mask;
+	u32		val;
+	u32		read;
 };
 
 struct i915_wa_list {
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/4] drm/i915/selftests: Verify whitelist of context registers

[Chris Wilson]

The RING_NONPRIV allows us to add registers to a whitelist that allows
userspace to modify them. Ideally such registers should be safe and
saved within the context such that they do not impact system behaviour
for other users. This selftest verifies that those registers we do add
are (a) then writable by userspace and (b) only affect a single client.

Opens:
- Is GEN9_SLICE_COMMON_ECO_CHICKEN1 really write-only?

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 .../drm/i915/selftests/intel_workarounds.c    | 320 ++++++++++++++++++
 1 file changed, 320 insertions(+)

diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
index a363748a7a4f..56799f3538f9 100644
--- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
@@ -700,6 +700,325 @@ static int live_reset_whitelist(void *arg)
 	return err;
 }
 
+static int read_whitelisted_registers(struct i915_gem_context *ctx,
+				      struct intel_engine_cs *engine,
+				      struct i915_vma *results)
+{
+	intel_wakeref_t wakeref;
+	struct i915_request *rq;
+	u32 srm, *cs;
+	int err, i;
+
+	rq = ERR_PTR(-ENODEV);
+	with_intel_runtime_pm(engine->i915, wakeref)
+		rq = i915_request_alloc(engine, ctx);
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
+
+	err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
+	if (err)
+		goto err_req;
+
+	srm = MI_STORE_REGISTER_MEM;
+	if (INTEL_GEN(ctx->i915) >= 8)
+		srm++;
+
+	cs = intel_ring_begin(rq, 4 * engine->whitelist.count);
+	if (IS_ERR(cs)) {
+		err = PTR_ERR(cs);
+		goto err_req;
+	}
+
+	for (i = 0; i < engine->whitelist.count; i++) {
+		u64 offset = results->node.start + sizeof(u32) * i;
+
+		*cs++ = srm;
+		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = lower_32_bits(offset);
+		*cs++ = upper_32_bits(offset);
+	}
+	intel_ring_advance(rq, cs);
+
+err_req:
+	i915_request_add(rq);
+
+	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
+		err = -EIO;
+
+	return err;
+}
+
+static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
+				       struct intel_engine_cs *engine)
+{
+	intel_wakeref_t wakeref;
+	struct i915_request *rq;
+	struct i915_vma *batch;
+	int i, err;
+	u32 *cs;
+
+	batch = create_batch(ctx);
+	if (IS_ERR(batch))
+		return PTR_ERR(batch);
+
+	cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC);
+	if (IS_ERR(cs)) {
+		err = PTR_ERR(cs);
+		goto err_batch;
+	}
+
+	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
+	for (i = 0; i < engine->whitelist.count; i++) {
+		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = 0xffffffff;
+	}
+	*cs++ = MI_BATCH_BUFFER_END;
+
+	i915_gem_object_flush_map(batch->obj);
+	i915_gem_chipset_flush(ctx->i915);
+
+	rq = ERR_PTR(-ENODEV);
+	with_intel_runtime_pm(engine->i915, wakeref)
+		rq = i915_request_alloc(engine, ctx);
+	if (IS_ERR(rq))
+		goto err_unpin;
+
+	if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
+		err = engine->emit_init_breadcrumb(rq);
+		if (err)
+			goto err_request;
+	}
+
+	err = engine->emit_bb_start(rq, batch->node.start, 0, 0);
+
+err_request:
+	i915_request_add(rq);
+	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
+		err = -EIO;
+
+err_unpin:
+	i915_gem_object_unpin_map(batch->obj);
+err_batch:
+	i915_vma_unpin_and_release(&batch, 0);
+	return err;
+}
+
+static bool pardon_reg(struct drm_i915_private *i915, i915_reg_t reg)
+{
+	/* Alas, we must pardon some whitelists */
+	static const struct {
+		i915_reg_t reg;
+		unsigned long gen_mask;
+	} pardon[] = {
+		{ GEN9_CTX_PREEMPT_REG, INTEL_GEN_MASK(9, 9) },
+		{ GEN8_L3SQCREG4, INTEL_GEN_MASK(9, 9) },
+	};
+	u32 offset = i915_mmio_reg_offset(reg);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pardon); i++) {
+		if (INTEL_INFO(i915)->gen_mask & pardon[i].gen_mask &&
+		    i915_mmio_reg_offset(pardon[i].reg) == offset)
+			return true;
+	}
+
+	return false;
+}
+
+static int eq_whitelisted_registers(struct i915_vma *A,
+				    struct i915_vma *B,
+				    struct intel_engine_cs *engine)
+{
+	u32 *a, *b;
+	int i, err;
+
+	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
+	if (IS_ERR(a))
+		return PTR_ERR(a);
+
+	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
+	if (IS_ERR(b)) {
+		err = PTR_ERR(b);
+		goto err_a;
+	}
+
+	err = 0;
+	for (i = 0; i < engine->whitelist.count; i++) {
+		if (a[i] != b[i] &&
+		    !pardon_reg(engine->i915, engine->whitelist.list[i].reg)) {
+			pr_err("[%d] Whitelisted register 0x%4x not context saved: A=%08x, B=%08x\n",
+			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg),
+			       a[i], b[i]);
+			err = -EINVAL;
+		}
+	}
+
+	i915_gem_object_unpin_map(B->obj);
+err_a:
+	i915_gem_object_unpin_map(A->obj);
+	return err;
+}
+
+static bool writeonly_reg(struct drm_i915_private *i915, i915_reg_t reg)
+{
+	static const struct {
+		i915_reg_t reg;
+		unsigned long gen_mask;
+	} wo[] = {
+		{ GEN9_SLICE_COMMON_ECO_CHICKEN1, INTEL_GEN_MASK(9, 9) },
+	};
+	u32 offset = i915_mmio_reg_offset(reg);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(wo); i++) {
+		if (INTEL_INFO(i915)->gen_mask & wo[i].gen_mask &&
+		    i915_mmio_reg_offset(wo[i].reg) == offset)
+			return true;
+	}
+
+	return false;
+}
+
+static int neq_whitelisted_registers(struct i915_vma *A,
+				     struct i915_vma *B,
+				     struct intel_engine_cs *engine)
+{
+	u32 *a, *b;
+	int i, err;
+
+	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
+	if (IS_ERR(a))
+		return PTR_ERR(a);
+
+	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
+	if (IS_ERR(b)) {
+		err = PTR_ERR(b);
+		goto err_a;
+	}
+
+	err = 0;
+	for (i = 0; i < engine->whitelist.count; i++) {
+		if (a[i] == b[i] &&
+		    !writeonly_reg(engine->i915, engine->whitelist.list[i])) {
+			pr_err("[%d] Whitelist register 0x%4x:%08x was unwritable\n",
+			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg), a[i]);
+			err = -EINVAL;
+		}
+	}
+
+	i915_gem_object_unpin_map(B->obj);
+err_a:
+	i915_gem_object_unpin_map(A->obj);
+	return err;
+}
+
+static int live_isolated_whitelist(void *arg)
+{
+	struct drm_i915_private *i915 = arg;
+	struct {
+		struct i915_gem_context *ctx;
+		struct i915_vma *scratch[2];
+	} client[2] = {};
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int i, err = 0;
+
+	/*
+	 * Check that a write into a whitelist register works, but
+	 * invisible to a second context.
+	 */
+
+	if (!intel_engines_has_context_isolation(i915))
+		return 0;
+
+	if (!i915->kernel_context->ppgtt)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(client); i++) {
+		struct i915_gem_context *c;
+
+		c = kernel_context(i915);
+		if (IS_ERR(c)) {
+			err = PTR_ERR(c);
+			goto err;
+		}
+
+		client[i].scratch[0] = create_scratch(&c->ppgtt->vm, 1024);
+		if (IS_ERR(client[i].scratch[0])) {
+			err = PTR_ERR(client[i].scratch[0]);
+			kernel_context_close(c);
+			goto err;
+		}
+
+		client[i].scratch[1] = create_scratch(&c->ppgtt->vm, 1024);
+		if (IS_ERR(client[i].scratch[1])) {
+			err = PTR_ERR(client[i].scratch[1]);
+			i915_vma_unpin_and_release(&client[i].scratch[0], 0);
+			kernel_context_close(c);
+			goto err;
+		}
+
+		client[i].ctx = c;
+	}
+
+	for_each_engine(engine, i915, id) {
+		if (!engine->whitelist.count)
+			continue;
+
+		/* Read default values */
+		err = read_whitelisted_registers(client[0].ctx, engine,
+						 client[0].scratch[0]);
+		if (err)
+			goto err;
+
+		/* Try to overwrite registers (should only affect ctx0) */
+		err = scrub_whitelisted_registers(client[0].ctx, engine);
+		if (err)
+			goto err;
+
+		/* Read values from ctx1, we expect these to be defaults */
+		err = read_whitelisted_registers(client[1].ctx, engine,
+						 client[1].scratch[0]);
+		if (err)
+			goto err;
+
+		/* Verify that both reads return the same default values */
+		err = eq_whitelisted_registers(client[0].scratch[0],
+					       client[1].scratch[0],
+					       engine);
+		if (err)
+			goto err;
+
+		/* Read back the updated values in ctx0 */
+		err = read_whitelisted_registers(client[0].ctx, engine,
+						 client[0].scratch[1]);
+		if (err)
+			goto err;
+
+		/* User should be granted privilege to overwhite regs */
+		err = neq_whitelisted_registers(client[0].scratch[0],
+						client[0].scratch[1],
+						engine);
+		if (err)
+			goto err;
+	}
+
+err:
+	for (i = 0; i < ARRAY_SIZE(client); i++) {
+		if (!client[i].ctx)
+			break;
+
+		i915_vma_unpin_and_release(&client[i].scratch[1], 0);
+		i915_vma_unpin_and_release(&client[i].scratch[0], 0);
+		kernel_context_close(client[i].ctx);
+	}
+
+	if (igt_flush_test(i915, I915_WAIT_LOCKED))
+		err = -EIO;
+
+	return err;
+}
+
 static bool verify_gt_engine_wa(struct drm_i915_private *i915,
 				struct wa_lists *lists, const char *str)
 {
@@ -844,6 +1163,7 @@ int intel_workarounds_live_selftests(struct drm_i915_private *i915)
 	static const struct i915_subtest tests[] = {
 		SUBTEST(live_dirty_whitelist),
 		SUBTEST(live_reset_whitelist),
+		SUBTEST(live_isolated_whitelist),
 		SUBTEST(live_gpu_reset_gt_engine_workarounds),
 		SUBTEST(live_engine_reset_gt_engine_workarounds),
 	};
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/selftests: Verify whitelist of context registers

[Chris Wilson]

The RING_NONPRIV allows us to add registers to a whitelist that allows
userspace to modify them. Ideally such registers should be safe and
saved within the context such that they do not impact system behaviour
for other users. This selftest verifies that those registers we do add
are (a) then writable by userspace and (b) only affect a single client.

Opens:
- Is GEN9_SLICE_COMMON_ECO_CHICKEN1 really write-only?

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
Compile fix for writeonly_reg()
---
 .../drm/i915/selftests/intel_workarounds.c    | 320 ++++++++++++++++++
 1 file changed, 320 insertions(+)

diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
index a363748a7a4f..f40e0937ec96 100644
--- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
@@ -700,6 +700,325 @@ static int live_reset_whitelist(void *arg)
 	return err;
 }
 
+static int read_whitelisted_registers(struct i915_gem_context *ctx,
+				      struct intel_engine_cs *engine,
+				      struct i915_vma *results)
+{
+	intel_wakeref_t wakeref;
+	struct i915_request *rq;
+	u32 srm, *cs;
+	int err, i;
+
+	rq = ERR_PTR(-ENODEV);
+	with_intel_runtime_pm(engine->i915, wakeref)
+		rq = i915_request_alloc(engine, ctx);
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
+
+	err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
+	if (err)
+		goto err_req;
+
+	srm = MI_STORE_REGISTER_MEM;
+	if (INTEL_GEN(ctx->i915) >= 8)
+		srm++;
+
+	cs = intel_ring_begin(rq, 4 * engine->whitelist.count);
+	if (IS_ERR(cs)) {
+		err = PTR_ERR(cs);
+		goto err_req;
+	}
+
+	for (i = 0; i < engine->whitelist.count; i++) {
+		u64 offset = results->node.start + sizeof(u32) * i;
+
+		*cs++ = srm;
+		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = lower_32_bits(offset);
+		*cs++ = upper_32_bits(offset);
+	}
+	intel_ring_advance(rq, cs);
+
+err_req:
+	i915_request_add(rq);
+
+	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
+		err = -EIO;
+
+	return err;
+}
+
+static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
+				       struct intel_engine_cs *engine)
+{
+	intel_wakeref_t wakeref;
+	struct i915_request *rq;
+	struct i915_vma *batch;
+	int i, err;
+	u32 *cs;
+
+	batch = create_batch(ctx);
+	if (IS_ERR(batch))
+		return PTR_ERR(batch);
+
+	cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC);
+	if (IS_ERR(cs)) {
+		err = PTR_ERR(cs);
+		goto err_batch;
+	}
+
+	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
+	for (i = 0; i < engine->whitelist.count; i++) {
+		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = 0xffffffff;
+	}
+	*cs++ = MI_BATCH_BUFFER_END;
+
+	i915_gem_object_flush_map(batch->obj);
+	i915_gem_chipset_flush(ctx->i915);
+
+	rq = ERR_PTR(-ENODEV);
+	with_intel_runtime_pm(engine->i915, wakeref)
+		rq = i915_request_alloc(engine, ctx);
+	if (IS_ERR(rq))
+		goto err_unpin;
+
+	if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
+		err = engine->emit_init_breadcrumb(rq);
+		if (err)
+			goto err_request;
+	}
+
+	err = engine->emit_bb_start(rq, batch->node.start, 0, 0);
+
+err_request:
+	i915_request_add(rq);
+	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
+		err = -EIO;
+
+err_unpin:
+	i915_gem_object_unpin_map(batch->obj);
+err_batch:
+	i915_vma_unpin_and_release(&batch, 0);
+	return err;
+}
+
+static bool pardon_reg(struct drm_i915_private *i915, i915_reg_t reg)
+{
+	/* Alas, we must pardon some whitelists */
+	static const struct {
+		i915_reg_t reg;
+		unsigned long gen_mask;
+	} pardon[] = {
+		{ GEN9_CTX_PREEMPT_REG, INTEL_GEN_MASK(9, 9) },
+		{ GEN8_L3SQCREG4, INTEL_GEN_MASK(9, 9) },
+	};
+	u32 offset = i915_mmio_reg_offset(reg);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(pardon); i++) {
+		if (INTEL_INFO(i915)->gen_mask & pardon[i].gen_mask &&
+		    i915_mmio_reg_offset(pardon[i].reg) == offset)
+			return true;
+	}
+
+	return false;
+}
+
+static int eq_whitelisted_registers(struct i915_vma *A,
+				    struct i915_vma *B,
+				    struct intel_engine_cs *engine)
+{
+	u32 *a, *b;
+	int i, err;
+
+	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
+	if (IS_ERR(a))
+		return PTR_ERR(a);
+
+	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
+	if (IS_ERR(b)) {
+		err = PTR_ERR(b);
+		goto err_a;
+	}
+
+	err = 0;
+	for (i = 0; i < engine->whitelist.count; i++) {
+		if (a[i] != b[i] &&
+		    !pardon_reg(engine->i915, engine->whitelist.list[i].reg)) {
+			pr_err("[%d] Whitelisted register 0x%4x not context saved: A=%08x, B=%08x\n",
+			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg),
+			       a[i], b[i]);
+			err = -EINVAL;
+		}
+	}
+
+	i915_gem_object_unpin_map(B->obj);
+err_a:
+	i915_gem_object_unpin_map(A->obj);
+	return err;
+}
+
+static bool writeonly_reg(struct drm_i915_private *i915, i915_reg_t reg)
+{
+	static const struct {
+		i915_reg_t reg;
+		unsigned long gen_mask;
+	} wo[] = {
+		{ GEN9_SLICE_COMMON_ECO_CHICKEN1, INTEL_GEN_MASK(9, 9) },
+	};
+	u32 offset = i915_mmio_reg_offset(reg);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(wo); i++) {
+		if (INTEL_INFO(i915)->gen_mask & wo[i].gen_mask &&
+		    i915_mmio_reg_offset(wo[i].reg) == offset)
+			return true;
+	}
+
+	return false;
+}
+
+static int neq_whitelisted_registers(struct i915_vma *A,
+				     struct i915_vma *B,
+				     struct intel_engine_cs *engine)
+{
+	u32 *a, *b;
+	int i, err;
+
+	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
+	if (IS_ERR(a))
+		return PTR_ERR(a);
+
+	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
+	if (IS_ERR(b)) {
+		err = PTR_ERR(b);
+		goto err_a;
+	}
+
+	err = 0;
+	for (i = 0; i < engine->whitelist.count; i++) {
+		if (a[i] == b[i] &&
+		    !writeonly_reg(engine->i915, engine->whitelist.list[i].reg)) {
+			pr_err("[%d] Whitelist register 0x%4x:%08x was unwritable\n",
+			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg), a[i]);
+			err = -EINVAL;
+		}
+	}
+
+	i915_gem_object_unpin_map(B->obj);
+err_a:
+	i915_gem_object_unpin_map(A->obj);
+	return err;
+}
+
+static int live_isolated_whitelist(void *arg)
+{
+	struct drm_i915_private *i915 = arg;
+	struct {
+		struct i915_gem_context *ctx;
+		struct i915_vma *scratch[2];
+	} client[2] = {};
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int i, err = 0;
+
+	/*
+	 * Check that a write into a whitelist register works, but
+	 * invisible to a second context.
+	 */
+
+	if (!intel_engines_has_context_isolation(i915))
+		return 0;
+
+	if (!i915->kernel_context->ppgtt)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(client); i++) {
+		struct i915_gem_context *c;
+
+		c = kernel_context(i915);
+		if (IS_ERR(c)) {
+			err = PTR_ERR(c);
+			goto err;
+		}
+
+		client[i].scratch[0] = create_scratch(&c->ppgtt->vm, 1024);
+		if (IS_ERR(client[i].scratch[0])) {
+			err = PTR_ERR(client[i].scratch[0]);
+			kernel_context_close(c);
+			goto err;
+		}
+
+		client[i].scratch[1] = create_scratch(&c->ppgtt->vm, 1024);
+		if (IS_ERR(client[i].scratch[1])) {
+			err = PTR_ERR(client[i].scratch[1]);
+			i915_vma_unpin_and_release(&client[i].scratch[0], 0);
+			kernel_context_close(c);
+			goto err;
+		}
+
+		client[i].ctx = c;
+	}
+
+	for_each_engine(engine, i915, id) {
+		if (!engine->whitelist.count)
+			continue;
+
+		/* Read default values */
+		err = read_whitelisted_registers(client[0].ctx, engine,
+						 client[0].scratch[0]);
+		if (err)
+			goto err;
+
+		/* Try to overwrite registers (should only affect ctx0) */
+		err = scrub_whitelisted_registers(client[0].ctx, engine);
+		if (err)
+			goto err;
+
+		/* Read values from ctx1, we expect these to be defaults */
+		err = read_whitelisted_registers(client[1].ctx, engine,
+						 client[1].scratch[0]);
+		if (err)
+			goto err;
+
+		/* Verify that both reads return the same default values */
+		err = eq_whitelisted_registers(client[0].scratch[0],
+					       client[1].scratch[0],
+					       engine);
+		if (err)
+			goto err;
+
+		/* Read back the updated values in ctx0 */
+		err = read_whitelisted_registers(client[0].ctx, engine,
+						 client[0].scratch[1]);
+		if (err)
+			goto err;
+
+		/* User should be granted privilege to overwhite regs */
+		err = neq_whitelisted_registers(client[0].scratch[0],
+						client[0].scratch[1],
+						engine);
+		if (err)
+			goto err;
+	}
+
+err:
+	for (i = 0; i < ARRAY_SIZE(client); i++) {
+		if (!client[i].ctx)
+			break;
+
+		i915_vma_unpin_and_release(&client[i].scratch[1], 0);
+		i915_vma_unpin_and_release(&client[i].scratch[0], 0);
+		kernel_context_close(client[i].ctx);
+	}
+
+	if (igt_flush_test(i915, I915_WAIT_LOCKED))
+		err = -EIO;
+
+	return err;
+}
+
 static bool verify_gt_engine_wa(struct drm_i915_private *i915,
 				struct wa_lists *lists, const char *str)
 {
@@ -844,6 +1163,7 @@ int intel_workarounds_live_selftests(struct drm_i915_private *i915)
 	static const struct i915_subtest tests[] = {
 		SUBTEST(live_dirty_whitelist),
 		SUBTEST(live_reset_whitelist),
+		SUBTEST(live_isolated_whitelist),
 		SUBTEST(live_gpu_reset_gt_engine_workarounds),
 		SUBTEST(live_engine_reset_gt_engine_workarounds),
 	};
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/4] drm/i915: Verify workarounds immediately after application

[Tvrtko Ursulin]

On 16/04/2019 09:10, Chris Wilson wrote:
> Immediately after writing the workaround, verify that it stuck in the
> register.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=108954
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_workarounds.c | 32 +++++++++++++-----------
>   1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> index ccaf63679435..1c54b5030807 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -913,6 +913,20 @@ wal_get_fw_for_rmw(struct intel_uncore *uncore, const struct i915_wa_list *wal)
>   	return fw;
>   }
>   
> +static bool
> +wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> +{
> +	if ((cur ^ wa->val) & wa->mask) {
> +		DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
> +			  name, from, i915_mmio_reg_offset(wa->reg), cur,
> +			  cur & wa->mask, wa->val, wa->mask);
> +
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>   static void
>   wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
>   {
> @@ -931,6 +945,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
>   
>   	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
>   		intel_uncore_rmw_fw(uncore, wa->reg, wa->mask, wa->val);
> +		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> +			wa_verify(wa,
> +				  intel_uncore_read_fw(uncore, wa->reg),
> +				  wal->name, "applictation");

application

>   	}
>   
>   	intel_uncore_forcewake_put__locked(uncore, fw);
> @@ -942,20 +960,6 @@ void intel_gt_apply_workarounds(struct drm_i915_private *i915)
>   	wa_list_apply(&i915->uncore, &i915->gt_wa_list);
>   }
>   
> -static bool
> -wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> -{
> -	if ((cur ^ wa->val) & wa->mask) {
> -		DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
> -			  name, from, i915_mmio_reg_offset(wa->reg), cur,
> -			  cur & wa->mask, wa->val, wa->mask);
> -
> -		return false;
> -	}
> -
> -	return true;
> -}
> -
>   static bool wa_list_verify(struct intel_uncore *uncore,
>   			   const struct i915_wa_list *wal,
>   			   const char *from)
> 

With the typo fixed:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915: Verify the engine workarounds stick on application

[Tvrtko Ursulin]

On 16/04/2019 09:10, Chris Wilson wrote:
> Read the engine workarounds back using the GPU after loading the initial
> context state to verify that we are setting them correctly, and bail if
> it fails.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c               |   6 +
>   drivers/gpu/drm/i915/intel_workarounds.c      | 120 ++++++++++++++++++
>   drivers/gpu/drm/i915/intel_workarounds.h      |   2 +
>   .../drm/i915/selftests/intel_workarounds.c    |  53 +-------
>   4 files changed, 134 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0a818a60ad31..95ae69753e91 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4717,6 +4717,12 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   		i915_request_add(rq);
>   		if (err)
>   			goto err_active;
> +
> +		if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) &&
> +		    intel_engine_verify_workarounds(engine, "load")) {
> +			err = -EIO;
> +			goto err_active;

The two submission use different contexts timelines so strictly speaking 
should be somehow explicitly serialized.

> +		}
>   	}
>   
>   	/* Flush the default context image to memory, and enable powersaving. */
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> index 1c54b5030807..db99f2e676bb 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -1259,6 +1259,126 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
>   	wa_list_apply(engine->uncore, &engine->wa_list);
>   }
>   
> +static struct i915_vma *
> +create_scratch(struct i915_address_space *vm, int count)
> +{
> +	struct drm_i915_gem_object *obj;
> +	struct i915_vma *vma;
> +	unsigned int size;
> +	int err;
> +
> +	size = round_up(count * 4, PAGE_SIZE);

sizeof(u32) if you want.

> +	obj = i915_gem_object_create_internal(vm->i915, size);
> +	if (IS_ERR(obj))
> +		return ERR_CAST(obj);
> +
> +	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
> +
> +	vma = i915_vma_instance(obj, vm, NULL);
> +	if (IS_ERR(vma)) {
> +		err = PTR_ERR(vma);
> +		goto err_obj;
> +	}
> +
> +	err = i915_vma_pin(vma, 0, 0,
> +			   i915_vma_is_ggtt(vma) ? PIN_GLOBAL : PIN_USER);
> +	if (err)
> +		goto err_obj;
> +
> +	return vma;
> +
> +err_obj:
> +	i915_gem_object_put(obj);
> +	return ERR_PTR(err);
> +}
> +
> +static int
> +wa_list_srm(struct i915_request *rq,
> +	    const struct i915_wa_list *wal,
> +	    struct i915_vma *vma)
> +{
> +	const struct i915_wa *wa;
> +	u32 srm, *cs;
> +	int i;

A little bit of inconsistency between type of i here and in 
engine_wa_list_verify. Up to you.

> +
> +	srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
> +	if (INTEL_GEN(rq->i915) >= 8)
> +		srm++;
> +
> +	cs = intel_ring_begin(rq, 4 * wal->count);
> +	if (IS_ERR(cs))
> +		return PTR_ERR(cs);
> +
> +	for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +		*cs++ = srm;
> +		*cs++ = i915_mmio_reg_offset(wa->reg);
> +		*cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
> +		*cs++ = 0;
> +	}
> +	intel_ring_advance(rq, cs);
> +
> +	return 0;
> +}
> +
> +static int engine_wa_list_verify(struct intel_engine_cs *engine,
> +				 const struct i915_wa_list * const wal,
> +				 const char *from)
> +{
> +	const struct i915_wa *wa;
> +	struct i915_request *rq;
> +	struct i915_vma *vma;
> +	unsigned int i;
> +	u32 *results;
> +	int err;
> +
> +	if (!wal->count)
> +		return 0;
> +
> +	vma = create_scratch(&engine->i915->ggtt.vm, wal->count);
> +	if (IS_ERR(vma))
> +		return PTR_ERR(vma);
> +
> +	rq = i915_request_alloc(engine, engine->kernel_context->gem_context);
> +	if (IS_ERR(rq)) {
> +		err = PTR_ERR(rq);
> +		goto err_vma;
> +	}
> +
> +	err = wa_list_srm(rq, wal, vma);
> +	if (err)
> +		goto err_vma;
> +
> +	i915_request_add(rq);
> +	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0) {

Wouldn't:

err = i915_request_wait(...
if (err < 0 || !i915_request_completed())

be more correct?

> +		err = -ETIME;
> +		goto err_vma;
> +	}
> +
> +	results = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> +	if (IS_ERR(results)) {
> +		err = PTR_ERR(results);
> +		goto err_vma;
> +	}
> +
> +	err = 0;
> +	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> +		if (!wa_verify(wa, results[i], wal->name, from))
> +			err = -ENXIO;
> +
> +	i915_gem_object_unpin_map(vma->obj);
> +
> +err_vma:
> +	i915_vma_unpin(vma);
> +	i915_vma_put(vma);
> +	return err;
> +}
> +
> +int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
> +				    const char *from)
> +{
> +	return engine_wa_list_verify(engine, &engine->wa_list, from);
> +}
> +
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>   #include "selftests/intel_workarounds.c"
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.h b/drivers/gpu/drm/i915/intel_workarounds.h
> index 34eee5ec511e..fdf7ebb90f28 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.h
> +++ b/drivers/gpu/drm/i915/intel_workarounds.h
> @@ -30,5 +30,7 @@ void intel_engine_apply_whitelist(struct intel_engine_cs *engine);
>   
>   void intel_engine_init_workarounds(struct intel_engine_cs *engine);
>   void intel_engine_apply_workarounds(struct intel_engine_cs *engine);
> +int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
> +				    const char *from);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> index 567b6f8dae86..a363748a7a4f 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> @@ -340,49 +340,6 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine,
>   	return err;
>   }
>   
> -static struct i915_vma *create_scratch(struct i915_gem_context *ctx)
> -{
> -	struct drm_i915_gem_object *obj;
> -	struct i915_vma *vma;
> -	void *ptr;
> -	int err;
> -
> -	obj = i915_gem_object_create_internal(ctx->i915, PAGE_SIZE);
> -	if (IS_ERR(obj))
> -		return ERR_CAST(obj);
> -
> -	i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
> -
> -	ptr = i915_gem_object_pin_map(obj, I915_MAP_WB);
> -	if (IS_ERR(ptr)) {
> -		err = PTR_ERR(ptr);
> -		goto err_obj;
> -	}
> -	memset(ptr, 0xc5, PAGE_SIZE);
> -	i915_gem_object_flush_map(obj);
> -	i915_gem_object_unpin_map(obj);
> -
> -	vma = i915_vma_instance(obj, &ctx->ppgtt->vm, NULL);
> -	if (IS_ERR(vma)) {
> -		err = PTR_ERR(vma);
> -		goto err_obj;
> -	}
> -
> -	err = i915_vma_pin(vma, 0, 0, PIN_USER);
> -	if (err)
> -		goto err_obj;
> -
> -	err = i915_gem_object_set_to_cpu_domain(obj, false);
> -	if (err)
> -		goto err_obj;
> -
> -	return vma;
> -
> -err_obj:
> -	i915_gem_object_put(obj);
> -	return ERR_PTR(err);
> -}
> -
>   static struct i915_vma *create_batch(struct i915_gem_context *ctx)
>   {
>   	struct drm_i915_gem_object *obj;
> @@ -475,7 +432,7 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   	int err = 0, i, v;
>   	u32 *cs, *results;
>   
> -	scratch = create_scratch(ctx);
> +	scratch = create_scratch(&ctx->ppgtt->vm, 2 * ARRAY_SIZE(values) + 1);
>   	if (IS_ERR(scratch))
>   		return PTR_ERR(scratch);
>   
> @@ -752,9 +709,11 @@ static bool verify_gt_engine_wa(struct drm_i915_private *i915,
>   
>   	ok &= wa_list_verify(&i915->uncore, &lists->gt_wa_list, str);
>   
> -	for_each_engine(engine, i915, id)
> -		ok &= wa_list_verify(engine->uncore,
> -				     &lists->engine[id].wa_list, str);
> +	for_each_engine(engine, i915, id) {
> +		ok &= engine_wa_list_verify(engine,
> +					    &lists->engine[id].wa_list,
> +					    str) == 0;
> +	}
>   
>   	return ok;
>   }
> 


Okay so MMIO verification happens immediately after application and with 
SRM from selftests.

What about the context workarounds? With the SRM infrastructure those 
could be verified easily as well I think.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/4] drm/i915: Make workaround verification *optional*

[Tvrtko Ursulin]

On 16/04/2019 09:10, Chris Wilson wrote:
> Sometimes the HW doesn't even play fair, and completely forgets about
> register writes. Skip verifying known troublemakers.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=108954
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_workarounds.c      | 40 ++++++++++++++-----
>   .../gpu/drm/i915/intel_workarounds_types.h    |  7 ++--
>   2 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> index db99f2e676bb..ba58be05f58c 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -122,6 +122,7 @@ static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
>   			wal->wa_count++;
>   			wa_->val |= wa->val;
>   			wa_->mask |= wa->mask;
> +			wa_->read |= wa->read;
>   			return;
>   		}
>   	}
> @@ -146,9 +147,10 @@ wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
>   		   u32 val)
>   {
>   	struct i915_wa wa = {
> -		.reg = reg,
> +		.reg  = reg,
>   		.mask = mask,
> -		.val = val
> +		.val  = val,
> +		.read = mask,
>   	};
>   
>   	_wa_add(wal, &wa);
> @@ -172,6 +174,19 @@ wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
>   	wa_write_masked_or(wal, reg, val, val);
>   }
>   
> +static void
> +ignore_wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask, u32 val)
> +{
> +	struct i915_wa wa = {
> +		.reg  = reg,
> +		.mask = mask,
> +		.val  = val,
> +		/* Bonkers HW, skip verifying */
> +	};
> +
> +	_wa_add(wal, &wa);
> +}
> +
>   #define WA_SET_BIT_MASKED(addr, mask) \
>   	wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_ENABLE(mask))
>   
> @@ -916,10 +931,11 @@ wal_get_fw_for_rmw(struct intel_uncore *uncore, const struct i915_wa_list *wal)
>   static bool
>   wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
>   {
> -	if ((cur ^ wa->val) & wa->mask) {
> +	if ((cur ^ wa->val) & wa->read) {
>   		DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
> -			  name, from, i915_mmio_reg_offset(wa->reg), cur,
> -			  cur & wa->mask, wa->val, wa->mask);
> +			  name, from, i915_mmio_reg_offset(wa->reg),
> +			  cur, cur & wa->read,
> +			  wa->val, wa->mask);
>   
>   		return false;
>   	}
> @@ -1122,9 +1138,10 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>   			     _3D_CHICKEN3_AA_LINE_QUALITY_FIX_ENABLE);
>   
>   		/* WaPipelineFlushCoherentLines:icl */
> -		wa_write_or(wal,
> -			    GEN8_L3SQCREG4,
> -			    GEN8_LQSC_FLUSH_COHERENT_LINES);
> +		ignore_wa_write_or(wal,
> +				   GEN8_L3SQCREG4,
> +				   GEN8_LQSC_FLUSH_COHERENT_LINES,
> +				   GEN8_LQSC_FLUSH_COHERENT_LINES);
>   
>   		/*
>   		 * Wa_1405543622:icl
> @@ -1151,9 +1168,10 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>   		 * Wa_1405733216:icl
>   		 * Formerly known as WaDisableCleanEvicts
>   		 */
> -		wa_write_or(wal,
> -			    GEN8_L3SQCREG4,
> -			    GEN11_LQSC_CLEAN_EVICT_DISABLE);
> +		ignore_wa_write_or(wal,
> +				   GEN8_L3SQCREG4,
> +				   GEN11_LQSC_CLEAN_EVICT_DISABLE,
> +				   GEN11_LQSC_CLEAN_EVICT_DISABLE);
>   
>   		/* WaForwardProgressSoftReset:icl */
>   		wa_write_or(wal,
> diff --git a/drivers/gpu/drm/i915/intel_workarounds_types.h b/drivers/gpu/drm/i915/intel_workarounds_types.h
> index 30918da180ff..42ac1fb99572 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds_types.h
> +++ b/drivers/gpu/drm/i915/intel_workarounds_types.h
> @@ -12,9 +12,10 @@
>   #include "i915_reg.h"
>   
>   struct i915_wa {
> -	i915_reg_t	  reg;
> -	u32		  mask;
> -	u32		  val;
> +	i915_reg_t	reg;
> +	u32		mask;
> +	u32		val;
> +	u32		read;
>   };
>   
>   struct i915_wa_list {
> 

The sporadic nature of failures worries me here. What is better:

a) Stop verifying from the driver and potentially lose a workaround 
and/or never figure out what is actually going on.

b) Have CI buglog track this as a known issue "forever"? Can't our 
automated tooling handle this?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915: Verify the engine workarounds stick on application

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-16 10:43:43)
> 
> On 16/04/2019 09:10, Chris Wilson wrote:
> > Read the engine workarounds back using the GPU after loading the initial
> > context state to verify that we are setting them correctly, and bail if
> > it fails.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c               |   6 +
> >   drivers/gpu/drm/i915/intel_workarounds.c      | 120 ++++++++++++++++++
> >   drivers/gpu/drm/i915/intel_workarounds.h      |   2 +
> >   .../drm/i915/selftests/intel_workarounds.c    |  53 +-------
> >   4 files changed, 134 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0a818a60ad31..95ae69753e91 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4717,6 +4717,12 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >               i915_request_add(rq);
> >               if (err)
> >                       goto err_active;
> > +
> > +             if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) &&
> > +                 intel_engine_verify_workarounds(engine, "load")) {
> > +                     err = -EIO;
> > +                     goto err_active;
> 
> The two submission use different contexts timelines so strictly speaking 
> should be somehow explicitly serialized.

Yes, we are reading from the kernel context. Neither of these contexts
will overwrite the existing register state on first load, and there is
nothing to prevent fifo here. But it doesn't matter which order the read
is done, as the w/a are set by mmio, not engine->init_context().
Although I've been contemplating using init_context in case LRI work
better.

It could be in a separate loop beforehand, the only catch is that we
expect to leave this function with the GT idle, so it's best done before
we park.

> > diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> > index 1c54b5030807..db99f2e676bb 100644
> > --- a/drivers/gpu/drm/i915/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> > @@ -1259,6 +1259,126 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> >       wa_list_apply(engine->uncore, &engine->wa_list);
> >   }
> >   
> > +static struct i915_vma *
> > +create_scratch(struct i915_address_space *vm, int count)
> > +{
> > +     struct drm_i915_gem_object *obj;
> > +     struct i915_vma *vma;
> > +     unsigned int size;
> > +     int err;
> > +
> > +     size = round_up(count * 4, PAGE_SIZE);
> 
> sizeof(u32) if you want.
> 
> > +     obj = i915_gem_object_create_internal(vm->i915, size);
> > +     if (IS_ERR(obj))
> > +             return ERR_CAST(obj);
> > +
> > +     i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
> > +
> > +     vma = i915_vma_instance(obj, vm, NULL);
> > +     if (IS_ERR(vma)) {
> > +             err = PTR_ERR(vma);
> > +             goto err_obj;
> > +     }
> > +
> > +     err = i915_vma_pin(vma, 0, 0,
> > +                        i915_vma_is_ggtt(vma) ? PIN_GLOBAL : PIN_USER);
> > +     if (err)
> > +             goto err_obj;
> > +
> > +     return vma;
> > +
> > +err_obj:
> > +     i915_gem_object_put(obj);
> > +     return ERR_PTR(err);
> > +}
> > +
> > +static int
> > +wa_list_srm(struct i915_request *rq,
> > +         const struct i915_wa_list *wal,
> > +         struct i915_vma *vma)
> > +{
> > +     const struct i915_wa *wa;
> > +     u32 srm, *cs;
> > +     int i;
> 
> A little bit of inconsistency between type of i here and in 
> engine_wa_list_verify. Up to you.
> 
> > +
> > +     srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
> > +     if (INTEL_GEN(rq->i915) >= 8)
> > +             srm++;
> > +
> > +     cs = intel_ring_begin(rq, 4 * wal->count);
> > +     if (IS_ERR(cs))
> > +             return PTR_ERR(cs);
> > +
> > +     for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> > +             *cs++ = srm;
> > +             *cs++ = i915_mmio_reg_offset(wa->reg);
> > +             *cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
> > +             *cs++ = 0;
> > +     }
> > +     intel_ring_advance(rq, cs);
> > +
> > +     return 0;
> > +}
> > +
> > +static int engine_wa_list_verify(struct intel_engine_cs *engine,
> > +                              const struct i915_wa_list * const wal,
> > +                              const char *from)
> > +{
> > +     const struct i915_wa *wa;
> > +     struct i915_request *rq;
> > +     struct i915_vma *vma;
> > +     unsigned int i;
> > +     u32 *results;
> > +     int err;
> > +
> > +     if (!wal->count)
> > +             return 0;
> > +
> > +     vma = create_scratch(&engine->i915->ggtt.vm, wal->count);
> > +     if (IS_ERR(vma))
> > +             return PTR_ERR(vma);
> > +
> > +     rq = i915_request_alloc(engine, engine->kernel_context->gem_context);
> > +     if (IS_ERR(rq)) {
> > +             err = PTR_ERR(rq);
> > +             goto err_vma;
> > +     }
> > +
> > +     err = wa_list_srm(rq, wal, vma);
> > +     if (err)
> > +             goto err_vma;
> > +
> > +     i915_request_add(rq);
> > +     if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0) {
> 
> Wouldn't:
> 
> err = i915_request_wait(...
> if (err < 0 || !i915_request_completed())
> 
> be more correct?

It can't return for any other reason as it's an uninterruptible wait, so
it times out, or the request is completed.

If you want to consider that hangcheck could have kicked in the 0.2ms
and cancelled the request, then we could need to check
request->fence.error.

> 
> Okay so MMIO verification happens immediately after application and with 
> SRM from selftests.
> 
> What about the context workarounds? With the SRM infrastructure those 
> could be verified easily as well I think.

They can, one problem at at time :) I thought the context workarounds
were listed and checked by gem_workarounds. Aye, but only ctx_wa:

static int i915_wa_registers(struct seq_file *m, void *unused)
{
        struct drm_i915_private *i915 = node_to_i915(m->private);
        const struct i915_wa_list *wal = &i915->engine[RCS0]->ctx_wa_list;
        struct i915_wa *wa;
        unsigned int i;

        seq_printf(m, "Workarounds applied: %u\n", wal->count);
        for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
                seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n",
                           i915_mmio_reg_offset(wa->reg), wa->val, wa->mask);

        return 0;
}


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915: Verify the engine workarounds stick on application

[Tvrtko Ursulin]

On 16/04/2019 10:57, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-04-16 10:43:43)
>>
>> On 16/04/2019 09:10, Chris Wilson wrote:
>>> Read the engine workarounds back using the GPU after loading the initial
>>> context state to verify that we are setting them correctly, and bail if
>>> it fails.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_gem.c               |   6 +
>>>    drivers/gpu/drm/i915/intel_workarounds.c      | 120 ++++++++++++++++++
>>>    drivers/gpu/drm/i915/intel_workarounds.h      |   2 +
>>>    .../drm/i915/selftests/intel_workarounds.c    |  53 +-------
>>>    4 files changed, 134 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 0a818a60ad31..95ae69753e91 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -4717,6 +4717,12 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>>                i915_request_add(rq);
>>>                if (err)
>>>                        goto err_active;
>>> +
>>> +             if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) &&
>>> +                 intel_engine_verify_workarounds(engine, "load")) {
>>> +                     err = -EIO;
>>> +                     goto err_active;
>>
>> The two submission use different contexts timelines so strictly speaking
>> should be somehow explicitly serialized.
> 
> Yes, we are reading from the kernel context. Neither of these contexts
> will overwrite the existing register state on first load, and there is
> nothing to prevent fifo here. But it doesn't matter which order the read
> is done, as the w/a are set by mmio, not engine->init_context().
> Although I've been contemplating using init_context in case LRI work
> better.

Using init_context for what, engine wa/?

> 
> It could be in a separate loop beforehand, the only catch is that we
> expect to leave this function with the GT idle, so it's best done before
> we park.

Yep since the placement in the ->init_context loop confused me I think a 
separate loop would indeed be better.

__i915_gem_restart_engines looks like the place, to follow 
engine->init_hw which applies the engine workarounds.

>>> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
>>> index 1c54b5030807..db99f2e676bb 100644
>>> --- a/drivers/gpu/drm/i915/intel_workarounds.c
>>> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
>>> @@ -1259,6 +1259,126 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
>>>        wa_list_apply(engine->uncore, &engine->wa_list);
>>>    }
>>>    
>>> +static struct i915_vma *
>>> +create_scratch(struct i915_address_space *vm, int count)
>>> +{
>>> +     struct drm_i915_gem_object *obj;
>>> +     struct i915_vma *vma;
>>> +     unsigned int size;
>>> +     int err;
>>> +
>>> +     size = round_up(count * 4, PAGE_SIZE);
>>
>> sizeof(u32) if you want.
>>
>>> +     obj = i915_gem_object_create_internal(vm->i915, size);
>>> +     if (IS_ERR(obj))
>>> +             return ERR_CAST(obj);
>>> +
>>> +     i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
>>> +
>>> +     vma = i915_vma_instance(obj, vm, NULL);
>>> +     if (IS_ERR(vma)) {
>>> +             err = PTR_ERR(vma);
>>> +             goto err_obj;
>>> +     }
>>> +
>>> +     err = i915_vma_pin(vma, 0, 0,
>>> +                        i915_vma_is_ggtt(vma) ? PIN_GLOBAL : PIN_USER);
>>> +     if (err)
>>> +             goto err_obj;
>>> +
>>> +     return vma;
>>> +
>>> +err_obj:
>>> +     i915_gem_object_put(obj);
>>> +     return ERR_PTR(err);
>>> +}
>>> +
>>> +static int
>>> +wa_list_srm(struct i915_request *rq,
>>> +         const struct i915_wa_list *wal,
>>> +         struct i915_vma *vma)
>>> +{
>>> +     const struct i915_wa *wa;
>>> +     u32 srm, *cs;
>>> +     int i;
>>
>> A little bit of inconsistency between type of i here and in
>> engine_wa_list_verify. Up to you.
>>
>>> +
>>> +     srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
>>> +     if (INTEL_GEN(rq->i915) >= 8)
>>> +             srm++;
>>> +
>>> +     cs = intel_ring_begin(rq, 4 * wal->count);
>>> +     if (IS_ERR(cs))
>>> +             return PTR_ERR(cs);
>>> +
>>> +     for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
>>> +             *cs++ = srm;
>>> +             *cs++ = i915_mmio_reg_offset(wa->reg);
>>> +             *cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
>>> +             *cs++ = 0;
>>> +     }
>>> +     intel_ring_advance(rq, cs);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int engine_wa_list_verify(struct intel_engine_cs *engine,
>>> +                              const struct i915_wa_list * const wal,
>>> +                              const char *from)
>>> +{
>>> +     const struct i915_wa *wa;
>>> +     struct i915_request *rq;
>>> +     struct i915_vma *vma;
>>> +     unsigned int i;
>>> +     u32 *results;
>>> +     int err;
>>> +
>>> +     if (!wal->count)
>>> +             return 0;
>>> +
>>> +     vma = create_scratch(&engine->i915->ggtt.vm, wal->count);
>>> +     if (IS_ERR(vma))
>>> +             return PTR_ERR(vma);
>>> +
>>> +     rq = i915_request_alloc(engine, engine->kernel_context->gem_context);
>>> +     if (IS_ERR(rq)) {
>>> +             err = PTR_ERR(rq);
>>> +             goto err_vma;
>>> +     }
>>> +
>>> +     err = wa_list_srm(rq, wal, vma);
>>> +     if (err)
>>> +             goto err_vma;
>>> +
>>> +     i915_request_add(rq);
>>> +     if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0) {
>>
>> Wouldn't:
>>
>> err = i915_request_wait(...
>> if (err < 0 || !i915_request_completed())
>>
>> be more correct?
> 
> It can't return for any other reason as it's an uninterruptible wait, so
> it times out, or the request is completed.
> 
> If you want to consider that hangcheck could have kicked in the 0.2ms
> and cancelled the request, then we could need to check
> request->fence.error.

I was going by the comment for i915_request_wait which says "Returns the 
remaining time (in jiffies) if the request completed, which may be zero 
or -ETIME if the request is unfinished after the timeout expires.". So 
two timeout conditions. Anyways, I know it is highly hypothetical if that.

> 
>>
>> Okay so MMIO verification happens immediately after application and with
>> SRM from selftests.
>>
>> What about the context workarounds? With the SRM infrastructure those
>> could be verified easily as well I think.
> 
> They can, one problem at at time :) I thought the context workarounds
> were listed and checked by gem_workarounds. Aye, but only ctx_wa:
> 
> static int i915_wa_registers(struct seq_file *m, void *unused)
> {
>          struct drm_i915_private *i915 = node_to_i915(m->private);
>          const struct i915_wa_list *wal = &i915->engine[RCS0]->ctx_wa_list;
>          struct i915_wa *wa;
>          unsigned int i;
> 
>          seq_printf(m, "Workarounds applied: %u\n", wal->count);
>          for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
>                  seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n",
>                             i915_mmio_reg_offset(wa->reg), wa->val, wa->mask);
> 
>          return 0;
> }
> 
> 
> 

Yes one thing at a time is fine. :)

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915: Verify the engine workarounds stick on application

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-16 11:36:12)
> 
> On 16/04/2019 10:57, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-04-16 10:43:43)
> >>
> >> On 16/04/2019 09:10, Chris Wilson wrote:
> >>> Read the engine workarounds back using the GPU after loading the initial
> >>> context state to verify that we are setting them correctly, and bail if
> >>> it fails.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_gem.c               |   6 +
> >>>    drivers/gpu/drm/i915/intel_workarounds.c      | 120 ++++++++++++++++++
> >>>    drivers/gpu/drm/i915/intel_workarounds.h      |   2 +
> >>>    .../drm/i915/selftests/intel_workarounds.c    |  53 +-------
> >>>    4 files changed, 134 insertions(+), 47 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>> index 0a818a60ad31..95ae69753e91 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >>> @@ -4717,6 +4717,12 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>                i915_request_add(rq);
> >>>                if (err)
> >>>                        goto err_active;
> >>> +
> >>> +             if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) &&
> >>> +                 intel_engine_verify_workarounds(engine, "load")) {
> >>> +                     err = -EIO;
> >>> +                     goto err_active;
> >>
> >> The two submission use different contexts timelines so strictly speaking
> >> should be somehow explicitly serialized.
> > 
> > Yes, we are reading from the kernel context. Neither of these contexts
> > will overwrite the existing register state on first load, and there is
> > nothing to prevent fifo here. But it doesn't matter which order the read
> > is done, as the w/a are set by mmio, not engine->init_context().
> > Although I've been contemplating using init_context in case LRI work
> > better.
> 
> Using init_context for what, engine wa/?

Yeah, just thinking of other ways to try and hammer icl-2/3 (or
whichever fail today) into submission.

> > It could be in a separate loop beforehand, the only catch is that we
> > expect to leave this function with the GT idle, so it's best done before
> > we park.
> 
> Yep since the placement in the ->init_context loop confused me I think a 
> separate loop would indeed be better.
> 
> __i915_gem_restart_engines looks like the place, to follow 
> engine->init_hw which applies the engine workarounds.

Hmm. That may restart with a full engine and a probe take an
unnecessarily, if not dangerously, long time.

load_power_context() perhaps. Bleugh, even on resume, we may have
restarted persistent batches in the near future.

So far I've talked myself into that we only know the system is idle at
init, and when we park the engines. We could squeeze a sanity check into
the parking.

> >>> +     i915_request_add(rq);
> >>> +     if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0) {
> >>
> >> Wouldn't:
> >>
> >> err = i915_request_wait(...
> >> if (err < 0 || !i915_request_completed())
> >>
> >> be more correct?
> > 
> > It can't return for any other reason as it's an uninterruptible wait, so
> > it times out, or the request is completed.
> > 
> > If you want to consider that hangcheck could have kicked in the 0.2ms
> > and cancelled the request, then we could need to check
> > request->fence.error.
> 
> I was going by the comment for i915_request_wait which says "Returns the 
> remaining time (in jiffies) if the request completed, which may be zero 
> or -ETIME if the request is unfinished after the timeout expires.". So 
> two timeout conditions. Anyways, I know it is highly hypothetical if that.

Hence why we check for < 0, though it could just be -ETIME.

Hmm, the sentence slightly runs on, it's the remaining time that may be
zero if the request completed just in the nick of time. -ETIME is always
the response if the request is not completed at timeout.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2,1/4] drm/i915: Verify workarounds immediately after application (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/4] drm/i915: Verify workarounds immediately after application (rev2)
URL   : https://patchwork.freedesktop.org/series/59560/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5936 -> Patchwork_12814
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59560/revisions/2/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12814 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@fork-compute0:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109315] +17
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +18

  * igt@gem_exec_basic@basic-bsd2:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109276] +7

  * igt@gem_exec_basic@gtt-bsd2:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] +52

  * igt@gem_exec_parse@basic-rejected:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109289] +1

  * igt@gem_exec_store@basic-bsd2:
    - fi-hsw-4770:        NOTRUN -> SKIP [fdo#109271] +41

  * igt@gem_workarounds@basic-read:
    - fi-snb-2600:        NOTRUN -> SKIP [fdo#109271] +52

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      PASS -> FAIL [fdo#108511]

  * igt@kms_addfb_basic@addfb25-y-tiled-small:
    - fi-byt-n2820:       NOTRUN -> SKIP [fdo#109271] +51

  * igt@kms_busy@basic-flip-c:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-snb-2600:        NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-byt-n2820:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@dp-crc-fast:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109284] +8

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-hsw-peppy:       NOTRUN -> SKIP [fdo#109271] +46

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       NOTRUN -> DMESG-FAIL [fdo#102614] / [fdo#107814]

  * igt@kms_psr@primary_mmap_gtt:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#110189] +3

  * igt@kms_psr@primary_page_flip:
    - fi-skl-lmem:        NOTRUN -> SKIP [fdo#109271] +37

  * igt@prime_vgem@basic-fence-flip:
    - fi-icl-y:           NOTRUN -> SKIP [fdo#109294]

  
#### Possible fixes ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107814]: https://bugs.freedesktop.org/show_bug.cgi?id=107814
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189


Participating hosts (43 -> 43)
------------------------------

  Additional (7): fi-hsw-peppy fi-hsw-4770 fi-icl-y fi-skl-lmem fi-byt-n2820 fi-byt-clapper fi-snb-2600 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5936 -> Patchwork_12814

  CI_DRM_5936: 0ad14bd30d830a1a355040b29bfafbe6623d84f0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4948: cf27a37b867bf31dccbe5f1b3bd84a2e606544f0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12814: 51bc3ba1bd403dc3fb98067f31b691d192098fa1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

51bc3ba1bd40 drm/i915/selftests: Verify whitelist of context registers
aee6d3ccd582 drm/i915: Make workaround verification *optional*
5ae134c4b5f5 drm/i915: Verify the engine workarounds stick on application
7b9f802e0167 drm/i915: Verify workarounds immediately after application

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12814/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/4] drm/i915: Make workaround verification *optional*

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-16 10:48:05)
> 
> On 16/04/2019 09:10, Chris Wilson wrote:
> > Sometimes the HW doesn't even play fair, and completely forgets about
> > register writes. Skip verifying known troublemakers.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=108954
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_workarounds.c      | 40 ++++++++++++++-----
> >   .../gpu/drm/i915/intel_workarounds_types.h    |  7 ++--
> >   2 files changed, 33 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> > index db99f2e676bb..ba58be05f58c 100644
> > --- a/drivers/gpu/drm/i915/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> > @@ -122,6 +122,7 @@ static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
> >                       wal->wa_count++;
> >                       wa_->val |= wa->val;
> >                       wa_->mask |= wa->mask;
> > +                     wa_->read |= wa->read;
> >                       return;
> >               }
> >       }
> > @@ -146,9 +147,10 @@ wa_write_masked_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask,
> >                  u32 val)
> >   {
> >       struct i915_wa wa = {
> > -             .reg = reg,
> > +             .reg  = reg,
> >               .mask = mask,
> > -             .val = val
> > +             .val  = val,
> > +             .read = mask,
> >       };
> >   
> >       _wa_add(wal, &wa);
> > @@ -172,6 +174,19 @@ wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 val)
> >       wa_write_masked_or(wal, reg, val, val);
> >   }
> >   
> > +static void
> > +ignore_wa_write_or(struct i915_wa_list *wal, i915_reg_t reg, u32 mask, u32 val)
> > +{
> > +     struct i915_wa wa = {
> > +             .reg  = reg,
> > +             .mask = mask,
> > +             .val  = val,
> > +             /* Bonkers HW, skip verifying */
> > +     };
> > +
> > +     _wa_add(wal, &wa);
> > +}
> > +
> >   #define WA_SET_BIT_MASKED(addr, mask) \
> >       wa_write_masked_or(wal, (addr), (mask), _MASKED_BIT_ENABLE(mask))
> >   
> > @@ -916,10 +931,11 @@ wal_get_fw_for_rmw(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> >   static bool
> >   wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> >   {
> > -     if ((cur ^ wa->val) & wa->mask) {
> > +     if ((cur ^ wa->val) & wa->read) {
> >               DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
> > -                       name, from, i915_mmio_reg_offset(wa->reg), cur,
> > -                       cur & wa->mask, wa->val, wa->mask);
> > +                       name, from, i915_mmio_reg_offset(wa->reg),
> > +                       cur, cur & wa->read,
> > +                       wa->val, wa->mask);
> >   
> >               return false;
> >       }
> > @@ -1122,9 +1138,10 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> >                            _3D_CHICKEN3_AA_LINE_QUALITY_FIX_ENABLE);
> >   
> >               /* WaPipelineFlushCoherentLines:icl */
> > -             wa_write_or(wal,
> > -                         GEN8_L3SQCREG4,
> > -                         GEN8_LQSC_FLUSH_COHERENT_LINES);
> > +             ignore_wa_write_or(wal,
> > +                                GEN8_L3SQCREG4,
> > +                                GEN8_LQSC_FLUSH_COHERENT_LINES,
> > +                                GEN8_LQSC_FLUSH_COHERENT_LINES);
> >   
> >               /*
> >                * Wa_1405543622:icl
> > @@ -1151,9 +1168,10 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> >                * Wa_1405733216:icl
> >                * Formerly known as WaDisableCleanEvicts
> >                */
> > -             wa_write_or(wal,
> > -                         GEN8_L3SQCREG4,
> > -                         GEN11_LQSC_CLEAN_EVICT_DISABLE);
> > +             ignore_wa_write_or(wal,
> > +                                GEN8_L3SQCREG4,
> > +                                GEN11_LQSC_CLEAN_EVICT_DISABLE,
> > +                                GEN11_LQSC_CLEAN_EVICT_DISABLE);
> >   
> >               /* WaForwardProgressSoftReset:icl */
> >               wa_write_or(wal,
> > diff --git a/drivers/gpu/drm/i915/intel_workarounds_types.h b/drivers/gpu/drm/i915/intel_workarounds_types.h
> > index 30918da180ff..42ac1fb99572 100644
> > --- a/drivers/gpu/drm/i915/intel_workarounds_types.h
> > +++ b/drivers/gpu/drm/i915/intel_workarounds_types.h
> > @@ -12,9 +12,10 @@
> >   #include "i915_reg.h"
> >   
> >   struct i915_wa {
> > -     i915_reg_t        reg;
> > -     u32               mask;
> > -     u32               val;
> > +     i915_reg_t      reg;
> > +     u32             mask;
> > +     u32             val;
> > +     u32             read;
> >   };
> >   
> >   struct i915_wa_list {
> > 
> 
> The sporadic nature of failures worries me here. What is better:
> 
> a) Stop verifying from the driver and potentially lose a workaround 
> and/or never figure out what is actually going on.
> 
> b) Have CI buglog track this as a known issue "forever"? Can't our 
> automated tooling handle this?

Either way, it's NOTOURBUG. If it's not clear, my intent is that this is
recognised as a HW issue. The problem with letting it reside in the test
suite is that it is hard for humans to discern an expected failure from
a new failure (and if it hard for us to tell at a glance, I fully expect
a generic over-reaching catch all filter to be applied in cibuglog), so
I anticipate that this test becomes useless if left to cibuglog -- plus
it will remain forever in the issues.html. Hence marking up known
failures, with one hopes an HSDES.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/selftests: Verify whitelist of context registers

[Tvrtko Ursulin]

On 16/04/2019 10:12, Chris Wilson wrote:
> The RING_NONPRIV allows us to add registers to a whitelist that allows
> userspace to modify them. Ideally such registers should be safe and
> saved within the context such that they do not impact system behaviour
> for other users. This selftest verifies that those registers we do add
> are (a) then writable by userspace and (b) only affect a single client.
> 
> Opens:
> - Is GEN9_SLICE_COMMON_ECO_CHICKEN1 really write-only?
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> Compile fix for writeonly_reg()
> ---
>   .../drm/i915/selftests/intel_workarounds.c    | 320 ++++++++++++++++++
>   1 file changed, 320 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> index a363748a7a4f..f40e0937ec96 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> @@ -700,6 +700,325 @@ static int live_reset_whitelist(void *arg)
>   	return err;
>   }
>   
> +static int read_whitelisted_registers(struct i915_gem_context *ctx,
> +				      struct intel_engine_cs *engine,
> +				      struct i915_vma *results)
> +{
> +	intel_wakeref_t wakeref;
> +	struct i915_request *rq;
> +	u32 srm, *cs;
> +	int err, i;
> +
> +	rq = ERR_PTR(-ENODEV);
> +	with_intel_runtime_pm(engine->i915, wakeref)
> +		rq = i915_request_alloc(engine, ctx);
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
> +
> +	err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
> +	if (err)
> +		goto err_req;

Why do you track the vma? There's a wait below and in scrub you even 
flush manually.

> +
> +	srm = MI_STORE_REGISTER_MEM;
> +	if (INTEL_GEN(ctx->i915) >= 8)
> +		srm++;
> +
> +	cs = intel_ring_begin(rq, 4 * engine->whitelist.count);
> +	if (IS_ERR(cs)) {
> +		err = PTR_ERR(cs);
> +		goto err_req;
> +	}
> +
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		u64 offset = results->node.start + sizeof(u32) * i;
> +
> +		*cs++ = srm;
> +		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		*cs++ = lower_32_bits(offset);
> +		*cs++ = upper_32_bits(offset);
> +	}
> +	intel_ring_advance(rq, cs);
> +
> +err_req:
> +	i915_request_add(rq);
> +
> +	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
> +		err = -EIO;
> +
> +	return err;
> +}
> +
> +static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
> +				       struct intel_engine_cs *engine)
> +{
> +	intel_wakeref_t wakeref;
> +	struct i915_request *rq;
> +	struct i915_vma *batch;
> +	int i, err;
> +	u32 *cs;
> +
> +	batch = create_batch(ctx);
> +	if (IS_ERR(batch))
> +		return PTR_ERR(batch);
> +
> +	cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC);
> +	if (IS_ERR(cs)) {
> +		err = PTR_ERR(cs);
> +		goto err_batch;
> +	}
> +
> +	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		*cs++ = 0xffffffff;
> +	}
> +	*cs++ = MI_BATCH_BUFFER_END;
> +
> +	i915_gem_object_flush_map(batch->obj);
> +	i915_gem_chipset_flush(ctx->i915);
> +
> +	rq = ERR_PTR(-ENODEV);
> +	with_intel_runtime_pm(engine->i915, wakeref)
> +		rq = i915_request_alloc(engine, ctx);
> +	if (IS_ERR(rq))
> +		goto err_unpin;
> +
> +	if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
> +		err = engine->emit_init_breadcrumb(rq);
> +		if (err)
> +			goto err_request;
> +	}
> +
> +	err = engine->emit_bb_start(rq, batch->node.start, 0, 0);

Why is batch needed here when everything else is happy to run from the ring?

> +
> +err_request:
> +	i915_request_add(rq);
> +	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
> +		err = -EIO;
> +
> +err_unpin:
> +	i915_gem_object_unpin_map(batch->obj);
> +err_batch:
> +	i915_vma_unpin_and_release(&batch, 0);
> +	return err;
> +}
> +
> +static bool pardon_reg(struct drm_i915_private *i915, i915_reg_t reg)
> +{
> +	/* Alas, we must pardon some whitelists */
> +	static const struct {
> +		i915_reg_t reg;
> +		unsigned long gen_mask;
> +	} pardon[] = {
> +		{ GEN9_CTX_PREEMPT_REG, INTEL_GEN_MASK(9, 9) },
> +		{ GEN8_L3SQCREG4, INTEL_GEN_MASK(9, 9) },
> +	};
> +	u32 offset = i915_mmio_reg_offset(reg);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pardon); i++) {
> +		if (INTEL_INFO(i915)->gen_mask & pardon[i].gen_mask &&
> +		    i915_mmio_reg_offset(pardon[i].reg) == offset)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int eq_whitelisted_registers(struct i915_vma *A,
> +				    struct i915_vma *B,
> +				    struct intel_engine_cs *engine)
> +{
> +	u32 *a, *b;
> +	int i, err;
> +
> +	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
> +	if (IS_ERR(a))
> +		return PTR_ERR(a);
> +
> +	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
> +	if (IS_ERR(b)) {
> +		err = PTR_ERR(b);
> +		goto err_a;
> +	}
> +
> +	err = 0;
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		if (a[i] != b[i] &&
> +		    !pardon_reg(engine->i915, engine->whitelist.list[i].reg)) {
> +			pr_err("[%d] Whitelisted register 0x%4x not context saved: A=%08x, B=%08x\n",
> +			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg),
> +			       a[i], b[i]);
> +			err = -EINVAL;
> +		}
> +	}
> +
> +	i915_gem_object_unpin_map(B->obj);
> +err_a:
> +	i915_gem_object_unpin_map(A->obj);
> +	return err;
> +}
> +
> +static bool writeonly_reg(struct drm_i915_private *i915, i915_reg_t reg)
> +{
> +	static const struct {
> +		i915_reg_t reg;
> +		unsigned long gen_mask;
> +	} wo[] = {
> +		{ GEN9_SLICE_COMMON_ECO_CHICKEN1, INTEL_GEN_MASK(9, 9) },
> +	};

What is the difference between pardoned and whitelisted?

> +	u32 offset = i915_mmio_reg_offset(reg);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(wo); i++) {
> +		if (INTEL_INFO(i915)->gen_mask & wo[i].gen_mask &&
> +		    i915_mmio_reg_offset(wo[i].reg) == offset)
> +			return true;
> +	}
> +
> +	return false;

You could consolidate into one find_reg which takes the array.

> +}
> +
> +static int neq_whitelisted_registers(struct i915_vma *A,
> +				     struct i915_vma *B,
> +				     struct intel_engine_cs *engine)
> +{
> +	u32 *a, *b;
> +	int i, err;
> +
> +	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
> +	if (IS_ERR(a))
> +		return PTR_ERR(a);
> +
> +	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
> +	if (IS_ERR(b)) {
> +		err = PTR_ERR(b);
> +		goto err_a;
> +	}
> +
> +	err = 0;
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		if (a[i] == b[i] &&
> +		    !writeonly_reg(engine->i915, engine->whitelist.list[i].reg)) {
> +			pr_err("[%d] Whitelist register 0x%4x:%08x was unwritable\n",
> +			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg), a[i]);
> +			err = -EINVAL;
> +		}
> +	}
> +
> +	i915_gem_object_unpin_map(B->obj);
> +err_a:
> +	i915_gem_object_unpin_map(A->obj);
> +	return err;
> +}

eq and neq could also be cheaply consolidated into one taking table and 
error message.

> +
> +static int live_isolated_whitelist(void *arg)
> +{
> +	struct drm_i915_private *i915 = arg;
> +	struct {
> +		struct i915_gem_context *ctx;
> +		struct i915_vma *scratch[2];
> +	} client[2] = {};
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +	int i, err = 0;
> +
> +	/*
> +	 * Check that a write into a whitelist register works, but
> +	 * invisible to a second context.
> +	 */
> +
> +	if (!intel_engines_has_context_isolation(i915))
> +		return 0;
> +
> +	if (!i915->kernel_context->ppgtt)
> +		return 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(client); i++) {
> +		struct i915_gem_context *c;
> +
> +		c = kernel_context(i915);
> +		if (IS_ERR(c)) {
> +			err = PTR_ERR(c);
> +			goto err;
> +		}
> +
> +		client[i].scratch[0] = create_scratch(&c->ppgtt->vm, 1024);
> +		if (IS_ERR(client[i].scratch[0])) {
> +			err = PTR_ERR(client[i].scratch[0]);
> +			kernel_context_close(c);
> +			goto err;
> +		}
> +
> +		client[i].scratch[1] = create_scratch(&c->ppgtt->vm, 1024);
> +		if (IS_ERR(client[i].scratch[1])) {
> +			err = PTR_ERR(client[i].scratch[1]);
> +			i915_vma_unpin_and_release(&client[i].scratch[0], 0);
> +			kernel_context_close(c);
> +			goto err;
> +		}
> +
> +		client[i].ctx = c;
> +	}
> +
> +	for_each_engine(engine, i915, id) {
> +		if (!engine->whitelist.count)
> +			continue;
> +
> +		/* Read default values */
> +		err = read_whitelisted_registers(client[0].ctx, engine,
> +						 client[0].scratch[0]);
> +		if (err)
> +			goto err;
> +
> +		/* Try to overwrite registers (should only affect ctx0) */
> +		err = scrub_whitelisted_registers(client[0].ctx, engine);
> +		if (err)
> +			goto err;
> +
> +		/* Read values from ctx1, we expect these to be defaults */
> +		err = read_whitelisted_registers(client[1].ctx, engine,
> +						 client[1].scratch[0]);
> +		if (err)
> +			goto err;
> +
> +		/* Verify that both reads return the same default values */
> +		err = eq_whitelisted_registers(client[0].scratch[0],
> +					       client[1].scratch[0],
> +					       engine);
> +		if (err)
> +			goto err;
> +
> +		/* Read back the updated values in ctx0 */
> +		err = read_whitelisted_registers(client[0].ctx, engine,
> +						 client[0].scratch[1]);
> +		if (err)
> +			goto err;
> +
> +		/* User should be granted privilege to overwhite regs */
> +		err = neq_whitelisted_registers(client[0].scratch[0],
> +						client[0].scratch[1],
> +						engine);
> +		if (err)
> +			goto err;
> +	}
> +
> +err:
> +	for (i = 0; i < ARRAY_SIZE(client); i++) {
> +		if (!client[i].ctx)
> +			break;
> +
> +		i915_vma_unpin_and_release(&client[i].scratch[1], 0);
> +		i915_vma_unpin_and_release(&client[i].scratch[0], 0);
> +		kernel_context_close(client[i].ctx);
> +	}
> +
> +	if (igt_flush_test(i915, I915_WAIT_LOCKED))
> +		err = -EIO;
> +
> +	return err;
> +}
> +
>   static bool verify_gt_engine_wa(struct drm_i915_private *i915,
>   				struct wa_lists *lists, const char *str)
>   {
> @@ -844,6 +1163,7 @@ int intel_workarounds_live_selftests(struct drm_i915_private *i915)
>   	static const struct i915_subtest tests[] = {
>   		SUBTEST(live_dirty_whitelist),
>   		SUBTEST(live_reset_whitelist),
> +		SUBTEST(live_isolated_whitelist),
>   		SUBTEST(live_gpu_reset_gt_engine_workarounds),
>   		SUBTEST(live_engine_reset_gt_engine_workarounds),
>   	};
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [v2,1/4] drm/i915: Verify workarounds immediately after application (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/4] drm/i915: Verify workarounds immediately after application (rev2)
URL   : https://patchwork.freedesktop.org/series/59560/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5936_full -> Patchwork_12814_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_12814_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_blt@normal-max:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          PASS -> DMESG-WARN [fdo#108566] +5

  * igt@kms_atomic_transition@3x-modeset-transitions:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +8

  * igt@kms_busy@basic-modeset-e:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-f:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +2

  * igt@kms_content_protection@legacy:
    - shard-kbl:          NOTRUN -> FAIL [fdo#110321] / [fdo#110336]

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-skl:          PASS -> INCOMPLETE [fdo#109507]
    - shard-kbl:          PASS -> DMESG-WARN [fdo#108566] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-mmap-wc:
    - shard-skl:          PASS -> FAIL [fdo#108040]

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-blt:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +21

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-blt:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +94

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +5

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-onoff:
    - shard-iclb:         PASS -> FAIL [fdo#109247] +7

  * igt@kms_lease@atomic_implicit_crtc:
    - shard-skl:          NOTRUN -> FAIL [fdo#110279]

  * igt@kms_lease@cursor_implicit_plane:
    - shard-apl:          NOTRUN -> FAIL [fdo#110278]

  * igt@kms_plane@pixel-format-pipe-c-planes:
    - shard-glk:          PASS -> SKIP [fdo#109271]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-transparant-fb:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
    - shard-apl:          NOTRUN -> FAIL [fdo#108145] +1
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +2

  * igt@kms_psr@psr2_primary_mmap_gtt:
    - shard-iclb:         PASS -> SKIP [fdo#109441] +2

  * igt@kms_psr@sprite_mmap_gtt:
    - shard-iclb:         PASS -> FAIL [fdo#107383] / [fdo#110215] +2

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_sysfs_edid_timing:
    - shard-apl:          NOTRUN -> FAIL [fdo#100047]

  * igt@kms_universal_plane@universal-plane-gen9-features-pipe-b:
    - shard-apl:          PASS -> DMESG-WARN [fdo#110376] +1

  * igt@prime_nv_api@i915_nv_reimport_twice_check_flink_name:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +72

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@vcs1-s3:
    - shard-kbl:          DMESG-WARN [fdo#108566] -> PASS +1

  * igt@i915_pm_rpm@dpms-non-lpsp:
    - shard-apl:          DMESG-WARN [fdo#110376] -> PASS

  * igt@i915_selftest@live_workarounds:
    - shard-iclb:         DMESG-FAIL [fdo#108954] -> PASS

  * igt@i915_suspend@debugfs-reader:
    - shard-apl:          DMESG-WARN [fdo#108566] -> PASS +3

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          FAIL [fdo#105363] -> PASS
    - shard-glk:          FAIL [fdo#102887] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-kbl:          FAIL [fdo#102887] / [fdo#105363] -> PASS
    - shard-glk:          FAIL [fdo#102887] / [fdo#105363] -> PASS

  * igt@kms_flip@flip-vs-suspend:
    - shard-skl:          INCOMPLETE [fdo#107773] / [fdo#109507] -> PASS

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#109247] -> PASS +16

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-msflip-blt:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +4

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          FAIL [fdo#108145] -> PASS +1

  * igt@kms_psr@psr2_cursor_mmap_cpu:
    - shard-iclb:         SKIP [fdo#109441] -> PASS +4

  * igt@kms_psr@sprite_mmap_cpu:
    - shard-iclb:         FAIL [fdo#107383] / [fdo#110215] -> PASS +4

  
  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954
  [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
  [fdo#110278]: https://bugs.freedesktop.org/show_bug.cgi?id=110278
  [fdo#110279]: https://bugs.freedesktop.org/show_bug.cgi?id=110279
  [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321
  [fdo#110336]: https://bugs.freedesktop.org/show_bug.cgi?id=110336
  [fdo#110376]: https://bugs.freedesktop.org/show_bug.cgi?id=110376
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 9)
------------------------------

  Missing    (1): shard-hsw 


Build changes
-------------

    * Linux: CI_DRM_5936 -> Patchwork_12814

  CI_DRM_5936: 0ad14bd30d830a1a355040b29bfafbe6623d84f0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4948: cf27a37b867bf31dccbe5f1b3bd84a2e606544f0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12814: 51bc3ba1bd403dc3fb98067f31b691d192098fa1 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12814/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/selftests: Verify whitelist of context registers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2614 bytes --]

Hi Chris,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20190416]
[cannot apply to v5.1-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Verify-workarounds-immediately-after-application/20190417-035630
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x017-201915 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/intel_workarounds.c:1401:0:
   drivers/gpu/drm/i915/selftests/intel_workarounds.c: In function 'neq_whitelisted_registers':
>> drivers/gpu/drm/i915/selftests/intel_workarounds.c:902:36: error: incompatible type for argument 2 of 'writeonly_reg'
          !writeonly_reg(engine->i915, engine->whitelist.list[i])) {
                                       ^~~~~~
   drivers/gpu/drm/i915/selftests/intel_workarounds.c:862:13: note: expected 'i915_reg_t {aka struct <anonymous>}' but argument is of type 'struct i915_wa'
    static bool writeonly_reg(struct drm_i915_private *i915, i915_reg_t reg)
                ^~~~~~~~~~~~~

vim +/writeonly_reg +902 drivers/gpu/drm/i915/selftests/intel_workarounds.c

   881	
   882	static int neq_whitelisted_registers(struct i915_vma *A,
   883					     struct i915_vma *B,
   884					     struct intel_engine_cs *engine)
   885	{
   886		u32 *a, *b;
   887		int i, err;
   888	
   889		a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
   890		if (IS_ERR(a))
   891			return PTR_ERR(a);
   892	
   893		b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
   894		if (IS_ERR(b)) {
   895			err = PTR_ERR(b);
   896			goto err_a;
   897		}
   898	
   899		err = 0;
   900		for (i = 0; i < engine->whitelist.count; i++) {
   901			if (a[i] == b[i] &&
 > 902			    !writeonly_reg(engine->i915, engine->whitelist.list[i])) {
   903				pr_err("[%d] Whitelist register 0x%4x:%08x was unwritable\n",
   904				       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg), a[i]);
   905				err = -EINVAL;
   906			}
   907		}
   908	
   909		i915_gem_object_unpin_map(B->obj);
   910	err_a:
   911		i915_gem_object_unpin_map(A->obj);
   912		return err;
   913	}
   914	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27290 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/selftests: Verify whitelist of context registers

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-16 15:50:27)
> 
> On 16/04/2019 10:12, Chris Wilson wrote:
> > The RING_NONPRIV allows us to add registers to a whitelist that allows
> > userspace to modify them. Ideally such registers should be safe and
> > saved within the context such that they do not impact system behaviour
> > for other users. This selftest verifies that those registers we do add
> > are (a) then writable by userspace and (b) only affect a single client.
> > 
> > Opens:
> > - Is GEN9_SLICE_COMMON_ECO_CHICKEN1 really write-only?
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> > Compile fix for writeonly_reg()
> > ---
> >   .../drm/i915/selftests/intel_workarounds.c    | 320 ++++++++++++++++++
> >   1 file changed, 320 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> > index a363748a7a4f..f40e0937ec96 100644
> > --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> > @@ -700,6 +700,325 @@ static int live_reset_whitelist(void *arg)
> >       return err;
> >   }
> >   
> > +static int read_whitelisted_registers(struct i915_gem_context *ctx,
> > +                                   struct intel_engine_cs *engine,
> > +                                   struct i915_vma *results)
> > +{
> > +     intel_wakeref_t wakeref;
> > +     struct i915_request *rq;
> > +     u32 srm, *cs;
> > +     int err, i;
> > +
> > +     rq = ERR_PTR(-ENODEV);
> > +     with_intel_runtime_pm(engine->i915, wakeref)
> > +             rq = i915_request_alloc(engine, ctx);
> > +     if (IS_ERR(rq))
> > +             return PTR_ERR(rq);
> > +
> > +     err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
> > +     if (err)
> > +             goto err_req;
> 
> Why do you track the vma? There's a wait below and in scrub you even 
> flush manually.

copy-and-paste.

> > +     srm = MI_STORE_REGISTER_MEM;
> > +     if (INTEL_GEN(ctx->i915) >= 8)
> > +             srm++;
> > +
> > +     cs = intel_ring_begin(rq, 4 * engine->whitelist.count);
> > +     if (IS_ERR(cs)) {
> > +             err = PTR_ERR(cs);
> > +             goto err_req;
> > +     }
> > +
> > +     for (i = 0; i < engine->whitelist.count; i++) {
> > +             u64 offset = results->node.start + sizeof(u32) * i;
> > +
> > +             *cs++ = srm;
> > +             *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> > +             *cs++ = lower_32_bits(offset);
> > +             *cs++ = upper_32_bits(offset);
> > +     }
> > +     intel_ring_advance(rq, cs);
> > +
> > +err_req:
> > +     i915_request_add(rq);
> > +
> > +     if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
> > +             err = -EIO;
> > +
> > +     return err;
> > +}
> > +
> > +static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
> > +                                    struct intel_engine_cs *engine)
> > +{
> > +     intel_wakeref_t wakeref;
> > +     struct i915_request *rq;
> > +     struct i915_vma *batch;
> > +     int i, err;
> > +     u32 *cs;
> > +
> > +     batch = create_batch(ctx);
> > +     if (IS_ERR(batch))
> > +             return PTR_ERR(batch);
> > +
> > +     cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC);
> > +     if (IS_ERR(cs)) {
> > +             err = PTR_ERR(cs);
> > +             goto err_batch;
> > +     }
> > +
> > +     *cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
> > +     for (i = 0; i < engine->whitelist.count; i++) {
> > +             *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> > +             *cs++ = 0xffffffff;
> > +     }
> > +     *cs++ = MI_BATCH_BUFFER_END;
> > +
> > +     i915_gem_object_flush_map(batch->obj);
> > +     i915_gem_chipset_flush(ctx->i915);
> > +
> > +     rq = ERR_PTR(-ENODEV);
> > +     with_intel_runtime_pm(engine->i915, wakeref)
> > +             rq = i915_request_alloc(engine, ctx);
> > +     if (IS_ERR(rq))
> > +             goto err_unpin;
> > +
> > +     if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
> > +             err = engine->emit_init_breadcrumb(rq);
> > +             if (err)
> > +                     goto err_request;
> > +     }
> > +
> > +     err = engine->emit_bb_start(rq, batch->node.start, 0, 0);
> 
> Why is batch needed here when everything else is happy to run from the ring?

Because we are playing with HW registers, I want reset to work safely.
So long as we have init_breadcrumb, the payload scrubbing should work.
Ok.

> > +static bool writeonly_reg(struct drm_i915_private *i915, i915_reg_t reg)
> > +{
> > +     static const struct {
> > +             i915_reg_t reg;
> > +             unsigned long gen_mask;
> > +     } wo[] = {
> > +             { GEN9_SLICE_COMMON_ECO_CHICKEN1, INTEL_GEN_MASK(9, 9) },
> > +     };
> 
> What is the difference between pardoned and whitelisted?

Pardoned is for non-context registers that are already exposed as
RING_NONPRIV ABI. Mistakes already made.

writeonly is for a register that doesn't seem to stick but everyone
believes is a crucial w/a for glk.

> > +     u32 offset = i915_mmio_reg_offset(reg);
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(wo); i++) {
> > +             if (INTEL_INFO(i915)->gen_mask & wo[i].gen_mask &&
> > +                 i915_mmio_reg_offset(wo[i].reg) == offset)
> > +                     return true;
> > +     }
> > +
> > +     return false;
> 
> You could consolidate into one find_reg which takes the array.

Next up, you'll be saying we could bsearch by addr! :)

> > +static int neq_whitelisted_registers(struct i915_vma *A,
> > +                                  struct i915_vma *B,
> > +                                  struct intel_engine_cs *engine)
> > +{
> > +     u32 *a, *b;
> > +     int i, err;
> > +
> > +     a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
> > +     if (IS_ERR(a))
> > +             return PTR_ERR(a);
> > +
> > +     b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
> > +     if (IS_ERR(b)) {
> > +             err = PTR_ERR(b);
> > +             goto err_a;
> > +     }
> > +
> > +     err = 0;
> > +     for (i = 0; i < engine->whitelist.count; i++) {
> > +             if (a[i] == b[i] &&
> > +                 !writeonly_reg(engine->i915, engine->whitelist.list[i].reg)) {
> > +                     pr_err("[%d] Whitelist register 0x%4x:%08x was unwritable\n",
> > +                            i, i915_mmio_reg_offset(engine->whitelist.list[i].reg), a[i]);
> > +                     err = -EINVAL;
> > +             }
> > +     }
> > +
> > +     i915_gem_object_unpin_map(B->obj);
> > +err_a:
> > +     i915_gem_object_unpin_map(A->obj);
> > +     return err;
> > +}
> 
> eq and neq could also be cheaply consolidated into one taking table and 
> error message.

And op. The preamble is the same, the loop innards are not, but we can
just make a callback.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/selftests: Verify whitelist of context registers

[Chris Wilson]

Quoting Chris Wilson (2019-04-24 11:00:03)
> Quoting Tvrtko Ursulin (2019-04-16 15:50:27)
> > 
> > On 16/04/2019 10:12, Chris Wilson wrote:
> > > +     rq = ERR_PTR(-ENODEV);
> > > +     with_intel_runtime_pm(engine->i915, wakeref)
> > > +             rq = i915_request_alloc(engine, ctx);
> > > +     if (IS_ERR(rq))
> > > +             goto err_unpin;
> > > +
> > > +     if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
> > > +             err = engine->emit_init_breadcrumb(rq);
> > > +             if (err)
> > > +                     goto err_request;
> > > +     }
> > > +
> > > +     err = engine->emit_bb_start(rq, batch->node.start, 0, 0);
> > 
> > Why is batch needed here when everything else is happy to run from the ring?
> 
> Because we are playing with HW registers, I want reset to work safely.
> So long as we have init_breadcrumb, the payload scrubbing should work.
> Ok.

No, I am an idiot. The batch is so that we emulate userspace with
unprivileged execution
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/selftests: Verify whitelist of context registers

[Tvrtko Ursulin]

On 24/04/2019 12:03, Chris Wilson wrote:
> Quoting Chris Wilson (2019-04-24 11:00:03)
>> Quoting Tvrtko Ursulin (2019-04-16 15:50:27)
>>>
>>> On 16/04/2019 10:12, Chris Wilson wrote:
>>>> +     rq = ERR_PTR(-ENODEV);
>>>> +     with_intel_runtime_pm(engine->i915, wakeref)
>>>> +             rq = i915_request_alloc(engine, ctx);
>>>> +     if (IS_ERR(rq))
>>>> +             goto err_unpin;
>>>> +
>>>> +     if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
>>>> +             err = engine->emit_init_breadcrumb(rq);
>>>> +             if (err)
>>>> +                     goto err_request;
>>>> +     }
>>>> +
>>>> +     err = engine->emit_bb_start(rq, batch->node.start, 0, 0);
>>>
>>> Why is batch needed here when everything else is happy to run from the ring?
>>
>> Because we are playing with HW registers, I want reset to work safely.
>> So long as we have init_breadcrumb, the payload scrubbing should work.
>> Ok.
> 
> No, I am an idiot. The batch is so that we emulate userspace with
> unprivileged execution

Okay, keep the r-b. :)

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/selftests: Verify whitelist of context registers

[Chris Wilson]

The RING_NONPRIV allows us to add registers to a whitelist that allows
userspace to modify them. Ideally such registers should be safe and
saved within the context such that they do not impact system behaviour
for other users. This selftest verifies that those registers we do add
are (a) then writable by userspace and (b) only affect a single client.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 .../drm/i915/selftests/intel_workarounds.c    | 289 ++++++++++++++++++
 1 file changed, 289 insertions(+)

diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
index a363748a7a4f..d71b5b062d6f 100644
--- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
@@ -700,6 +700,294 @@ static int live_reset_whitelist(void *arg)
 	return err;
 }
 
+static int read_whitelisted_registers(struct i915_gem_context *ctx,
+				      struct intel_engine_cs *engine,
+				      struct i915_vma *results)
+{
+	intel_wakeref_t wakeref;
+	struct i915_request *rq;
+	u32 srm, *cs;
+	int err, i;
+
+	rq = ERR_PTR(-ENODEV);
+	with_intel_runtime_pm(engine->i915, wakeref)
+		rq = i915_request_alloc(engine, ctx);
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
+
+	err = i915_vma_move_to_active(results, rq, EXEC_OBJECT_WRITE);
+	if (err)
+		goto err_req;
+
+	srm = MI_STORE_REGISTER_MEM;
+	if (INTEL_GEN(ctx->i915) >= 8)
+		srm++;
+
+	cs = intel_ring_begin(rq, 4 * engine->whitelist.count);
+	if (IS_ERR(cs)) {
+		err = PTR_ERR(cs);
+		goto err_req;
+	}
+
+	for (i = 0; i < engine->whitelist.count; i++) {
+		u64 offset = results->node.start + sizeof(u32) * i;
+
+		*cs++ = srm;
+		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = lower_32_bits(offset);
+		*cs++ = upper_32_bits(offset);
+	}
+	intel_ring_advance(rq, cs);
+
+err_req:
+	i915_request_add(rq);
+
+	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
+		err = -EIO;
+
+	return err;
+}
+
+static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
+				       struct intel_engine_cs *engine)
+{
+	intel_wakeref_t wakeref;
+	struct i915_request *rq;
+	struct i915_vma *batch;
+	int i, err;
+	u32 *cs;
+
+	batch = create_batch(ctx);
+	if (IS_ERR(batch))
+		return PTR_ERR(batch);
+
+	cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC);
+	if (IS_ERR(cs)) {
+		err = PTR_ERR(cs);
+		goto err_batch;
+	}
+
+	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
+	for (i = 0; i < engine->whitelist.count; i++) {
+		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = STACK_MAGIC;
+	}
+	*cs++ = MI_BATCH_BUFFER_END;
+
+	i915_gem_object_flush_map(batch->obj);
+	i915_gem_chipset_flush(ctx->i915);
+
+	rq = ERR_PTR(-ENODEV);
+	with_intel_runtime_pm(engine->i915, wakeref)
+		rq = i915_request_alloc(engine, ctx);
+	if (IS_ERR(rq))
+		goto err_unpin;
+
+	if (engine->emit_init_breadcrumb) { /* Be nice if we hang */
+		err = engine->emit_init_breadcrumb(rq);
+		if (err)
+			goto err_request;
+	}
+
+	err = engine->emit_bb_start(rq, batch->node.start, 0, 0);
+
+err_request:
+	i915_request_add(rq);
+	if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0)
+		err = -EIO;
+
+err_unpin:
+	i915_gem_object_unpin_map(batch->obj);
+err_batch:
+	i915_vma_unpin_and_release(&batch, 0);
+	return err;
+}
+
+static bool ignore_reg(struct drm_i915_private *i915, i915_reg_t reg)
+{
+	/* Alas, we must pardon some whitelists */
+	static const struct {
+		i915_reg_t reg;
+		unsigned long gen_mask;
+	} ignore[] = {
+		{ GEN9_CTX_PREEMPT_REG, INTEL_GEN_MASK(9, 9) },
+		{ GEN8_L3SQCREG4, INTEL_GEN_MASK(9, 9) },
+	};
+	u32 offset = i915_mmio_reg_offset(reg);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ignore); i++) {
+		if (INTEL_INFO(i915)->gen_mask & ignore[i].gen_mask &&
+		    i915_mmio_reg_offset(ignore[i].reg) == offset)
+			return true;
+	}
+
+	return false;
+}
+
+static int eq_whitelisted_registers(struct i915_vma *A,
+				    struct i915_vma *B,
+				    struct intel_engine_cs *engine)
+{
+	u32 *a, *b;
+	int i, err;
+
+	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
+	if (IS_ERR(a))
+		return PTR_ERR(a);
+
+	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
+	if (IS_ERR(b)) {
+		err = PTR_ERR(b);
+		goto err_a;
+	}
+
+	err = 0;
+	for (i = 0; i < engine->whitelist.count; i++) {
+		if (a[i] != b[i] &&
+		    !ignore_reg(engine->i915, engine->whitelist.list[i].reg)) {
+			pr_err("[%d] Whitelisted register 0x%4x not context saved: A=%08x, B=%08x\n",
+			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg),
+			       a[i], b[i]);
+			err = -EINVAL;
+		}
+	}
+
+	i915_gem_object_unpin_map(B->obj);
+err_a:
+	i915_gem_object_unpin_map(A->obj);
+	return err;
+}
+
+static int neq_whitelisted_registers(struct i915_vma *A,
+				     struct i915_vma *B,
+				     struct intel_engine_cs *engine)
+{
+	u32 *a, *b;
+	int i, err;
+
+	a = i915_gem_object_pin_map(A->obj, I915_MAP_WB);
+	if (IS_ERR(a))
+		return PTR_ERR(a);
+
+	b = i915_gem_object_pin_map(B->obj, I915_MAP_WB);
+	if (IS_ERR(b)) {
+		err = PTR_ERR(b);
+		goto err_a;
+	}
+
+	err = 0;
+	for (i = 0; i < engine->whitelist.count; i++) {
+		if (a[i] == b[i]) {
+			pr_err("[%d] Whitelist register 0x%4x:%08x was unwritable\n",
+			       i, i915_mmio_reg_offset(engine->whitelist.list[i].reg), a[i]);
+			err = -EINVAL;
+		}
+	}
+
+	i915_gem_object_unpin_map(B->obj);
+err_a:
+	i915_gem_object_unpin_map(A->obj);
+	return err;
+}
+
+static int live_isolated_whitelist(void *arg)
+{
+	struct drm_i915_private *i915 = arg;
+	struct {
+		struct i915_gem_context *ctx;
+		struct i915_vma *scratch[2];
+	} client[2] = {};
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int i, err = 0;
+
+	/*
+	 * Check that a write into a whitelist register works, but
+	 * invisible to a second context.
+	 */
+
+	for (i = 0; i < ARRAY_SIZE(client); i++) {
+		struct i915_gem_context *c;
+
+		c = kernel_context(i915);
+		if (IS_ERR(c))
+			goto err;
+
+		client[i].scratch[0] = create_scratch(&c->ppgtt->vm, 1024);
+		if (IS_ERR(client[i].scratch[0])) {
+			kernel_context_close(c);
+			goto err;
+		}
+
+		client[i].scratch[1] = create_scratch(&c->ppgtt->vm, 1024);
+		if (IS_ERR(client[i].scratch[1])) {
+			i915_vma_unpin_and_release(&client[i].scratch[0], 0);
+			kernel_context_close(c);
+			goto err;
+		}
+
+		client[i].ctx = c;
+	}
+
+	for_each_engine(engine, i915, id) {
+		if (!engine->whitelist.count)
+			continue;
+
+		/* Read default values */
+		err = read_whitelisted_registers(client[0].ctx, engine,
+						 client[0].scratch[0]);
+		if (err)
+			goto err;
+
+		/* Try to overwrite registers (should only affect ctx0) */
+		err = scrub_whitelisted_registers(client[0].ctx, engine);
+		if (err)
+			goto err;
+
+		/* Read values from ctx1, we expect these to be defaults */
+		err = read_whitelisted_registers(client[1].ctx, engine,
+						 client[1].scratch[0]);
+		if (err)
+			goto err;
+
+		/* Verify that both reads return the same default values */
+		err = eq_whitelisted_registers(client[0].scratch[0],
+					       client[1].scratch[0],
+					       engine);
+		if (err)
+			goto err;
+
+		/* Read back the updated values in ctx0 */
+		err = read_whitelisted_registers(client[0].ctx, engine,
+						 client[0].scratch[1]);
+		if (err)
+			goto err;
+
+		/* User should be granted privilege to overwhite regs */
+		err = neq_whitelisted_registers(client[0].scratch[0],
+						client[1].scratch[1],
+						engine);
+		if (err)
+			goto err;
+	}
+
+err:
+	for (i = 0; i < ARRAY_SIZE(client); i++) {
+		if (!client[i].ctx)
+			break;
+
+		i915_vma_unpin_and_release(&client[i].scratch[1], 0);
+		i915_vma_unpin_and_release(&client[i].scratch[0], 0);
+		kernel_context_close(client[i].ctx);
+	}
+
+	if (igt_flush_test(i915, I915_WAIT_LOCKED))
+		err = -EIO;
+
+	return err;
+}
+
 static bool verify_gt_engine_wa(struct drm_i915_private *i915,
 				struct wa_lists *lists, const char *str)
 {
@@ -844,6 +1132,7 @@ int intel_workarounds_live_selftests(struct drm_i915_private *i915)
 	static const struct i915_subtest tests[] = {
 		SUBTEST(live_dirty_whitelist),
 		SUBTEST(live_reset_whitelist),
+		SUBTEST(live_isolated_whitelist),
 		SUBTEST(live_gpu_reset_gt_engine_workarounds),
 		SUBTEST(live_engine_reset_gt_engine_workarounds),
 	};
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx