[PATCH 0/4] Update whitelist support for new hardware

[PATCH 0/4] Update whitelist support for new hardware

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Recent hardware adds support for finer-grained control over
whitelisting, allowing registers to be whitelisted independently
for reads and/or writes. This series will add the basic plumbing
to support that.

John Harrison (3):
  drm/i915: Support flags in whitlist WAs
  drm/i915: Support whitelist workarounds on all engines
  drm/i915: Add whitelist workarounds for ICL

Robert M. Fosha (1):
  drm/i915: Update workarounds selftest for read only regs

 drivers/gpu/drm/i915/gt/intel_workarounds.c   | 105 +++++++++++++-----
 .../gpu/drm/i915/gt/selftest_workarounds.c    |  43 ++++++-
 drivers/gpu/drm/i915/i915_reg.h               |   7 ++
 3 files changed, 124 insertions(+), 31 deletions(-)

-- 
2.21.0.5.gaeb582a983

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

[PATCH 1/4] drm/i915: Support flags in whitlist WAs

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Newer hardware adds flags to the whitelist work-around register. These
allow per access direction privileges and ranges.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
 drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 165b0a45e009..ae82340fff45 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
 }
 
 static void
-whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
+whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
 {
 	struct i915_wa wa = {
 		.reg = reg
@@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
 	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
 		return;
 
+	wa.reg.reg |= flags;
 	_wa_add(wal, &wa);
 }
 
+static void
+whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
+{
+	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
+}
+
 static void gen9_whitelist_build(struct i915_wa_list *w)
 {
 	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7a26766ba84d..cc295a4f6e92 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2513,6 +2513,13 @@ enum i915_power_well_id {
 #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
 
 #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
+#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
+#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
+#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
+#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
+#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
+#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
+#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
 #define   RING_MAX_NONPRIV_SLOTS  12
 
 #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
-- 
2.21.0.5.gaeb582a983

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

[PATCH 2/4] drm/i915: Support whitelist workarounds on all engines

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Newer hardware requires setting up whitelists on engines other than
render. So, extend the whitelist code to support all engines.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 66 +++++++++++++++------
 1 file changed, 47 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index ae82340fff45..5308a0864e78 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1043,48 +1043,79 @@ static void gen9_whitelist_build(struct i915_wa_list *w)
 	whitelist_reg(w, GEN8_HDC_CHICKEN1);
 }
 
-static void skl_whitelist_build(struct i915_wa_list *w)
+static void skl_whitelist_build(struct intel_engine_cs *engine)
 {
+	struct i915_wa_list *w = &engine->whitelist;
+
+	if (engine->class != RENDER_CLASS)
+		return;
+
 	gen9_whitelist_build(w);
 
 	/* WaDisableLSQCROPERFforOCL:skl */
 	whitelist_reg(w, GEN8_L3SQCREG4);
 }
 
-static void bxt_whitelist_build(struct i915_wa_list *w)
+static void bxt_whitelist_build(struct intel_engine_cs *engine)
 {
-	gen9_whitelist_build(w);
+	if (engine->class != RENDER_CLASS)
+		return;
+
+	gen9_whitelist_build(&engine->whitelist);
 }
 
-static void kbl_whitelist_build(struct i915_wa_list *w)
+static void kbl_whitelist_build(struct intel_engine_cs *engine)
 {
+	struct i915_wa_list *w = &engine->whitelist;
+
+	if (engine->class != RENDER_CLASS)
+		return;
+
 	gen9_whitelist_build(w);
 
 	/* WaDisableLSQCROPERFforOCL:kbl */
 	whitelist_reg(w, GEN8_L3SQCREG4);
 }
 
-static void glk_whitelist_build(struct i915_wa_list *w)
+static void glk_whitelist_build(struct intel_engine_cs *engine)
 {
+	struct i915_wa_list *w = &engine->whitelist;
+
+	if (engine->class != RENDER_CLASS)
+		return;
+
 	gen9_whitelist_build(w);
 
 	/* WA #0862: Userspace has to set "Barrier Mode" to avoid hangs. */
 	whitelist_reg(w, GEN9_SLICE_COMMON_ECO_CHICKEN1);
 }
 
-static void cfl_whitelist_build(struct i915_wa_list *w)
+static void cfl_whitelist_build(struct intel_engine_cs *engine)
 {
-	gen9_whitelist_build(w);
+	if (engine->class != RENDER_CLASS)
+		return;
+
+	gen9_whitelist_build(&engine->whitelist);
 }
 
-static void cnl_whitelist_build(struct i915_wa_list *w)
+static void cnl_whitelist_build(struct intel_engine_cs *engine)
 {
+	struct i915_wa_list *w = &engine->whitelist;
+
+	if (engine->class != RENDER_CLASS)
+		return;
+
 	/* WaEnablePreemptionGranularityControlByUMD:cnl */
 	whitelist_reg(w, GEN8_CS_CHICKEN1);
 }
 
-static void icl_whitelist_build(struct i915_wa_list *w)
+static void icl_whitelist_build(struct intel_engine_cs *engine)
 {
+	struct i915_wa_list *w = &engine->whitelist;
+
+	if (engine->class != RENDER_CLASS)
+		return;
+
 	/* WaAllowUMDToModifyHalfSliceChicken7:icl */
 	whitelist_reg(w, GEN9_HALF_SLICE_CHICKEN7);
 
@@ -1100,25 +1131,22 @@ void intel_engine_init_whitelist(struct intel_engine_cs *engine)
 	struct drm_i915_private *i915 = engine->i915;
 	struct i915_wa_list *w = &engine->whitelist;
 
-	if (engine->class != RENDER_CLASS)
-		return;
-
 	wa_init_start(w, "whitelist");
 
 	if (IS_GEN(i915, 11))
-		icl_whitelist_build(w);
+		icl_whitelist_build(engine);
 	else if (IS_CANNONLAKE(i915))
-		cnl_whitelist_build(w);
+		cnl_whitelist_build(engine);
 	else if (IS_COFFEELAKE(i915))
-		cfl_whitelist_build(w);
+		cfl_whitelist_build(engine);
 	else if (IS_GEMINILAKE(i915))
-		glk_whitelist_build(w);
+		glk_whitelist_build(engine);
 	else if (IS_KABYLAKE(i915))
-		kbl_whitelist_build(w);
+		kbl_whitelist_build(engine);
 	else if (IS_BROXTON(i915))
-		bxt_whitelist_build(w);
+		bxt_whitelist_build(engine);
 	else if (IS_SKYLAKE(i915))
-		skl_whitelist_build(w);
+		skl_whitelist_build(engine);
 	else if (INTEL_GEN(i915) <= 8)
 		return;
 	else
-- 
2.21.0.5.gaeb582a983

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

[PATCH 3/4] drm/i915: Add whitelist workarounds for ICL

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Updated whitelist table for ICL.

v2: Reduce changes to just those required for media driver until
the selftest can be updated to support the new features of the
other entries.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c | 38 +++++++++++++++------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 5308a0864e78..d37ebcddb963 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1113,17 +1113,33 @@ static void icl_whitelist_build(struct intel_engine_cs *engine)
 {
 	struct i915_wa_list *w = &engine->whitelist;
 
-	if (engine->class != RENDER_CLASS)
-		return;
-
-	/* WaAllowUMDToModifyHalfSliceChicken7:icl */
-	whitelist_reg(w, GEN9_HALF_SLICE_CHICKEN7);
-
-	/* WaAllowUMDToModifySamplerMode:icl */
-	whitelist_reg(w, GEN10_SAMPLER_MODE);
-
-	/* WaEnableStateCacheRedirectToCS:icl */
-	whitelist_reg(w, GEN9_SLICE_COMMON_ECO_CHICKEN1);
+	switch (engine->class) {
+	case RENDER_CLASS:
+		/* WaAllowUMDToModifyHalfSliceChicken7:icl */
+		whitelist_reg(w, GEN9_HALF_SLICE_CHICKEN7);
+
+		/* WaAllowUMDToModifySamplerMode:icl */
+		whitelist_reg(w, GEN10_SAMPLER_MODE);
+
+		/* WaEnableStateCacheRedirectToCS:icl */
+		whitelist_reg(w, GEN9_SLICE_COMMON_ECO_CHICKEN1);
+		break;
+
+	case VIDEO_DECODE_CLASS:
+		/* hucStatusRegOffset */
+		whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
+				  RING_FORCE_TO_NONPRIV_RD);
+		/* hucUKernelHdrInfoRegOffset */
+		whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
+				  RING_FORCE_TO_NONPRIV_RD);
+		/* hucStatus2RegOffset */
+		whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
+				  RING_FORCE_TO_NONPRIV_RD);
+		break;
+
+	default:
+		break;
+	}
 }
 
 void intel_engine_init_whitelist(struct intel_engine_cs *engine)
-- 
2.21.0.5.gaeb582a983

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

[PATCH 4/4] drm/i915: Update workarounds selftest for read only regs

[John.C.Harrison]

From: "Robert M. Fosha" <robert.m.fosha@intel.com>

Updates the live_workarounds selftest to handle whitelisted
registers that are flagged as read only.

Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index c8d335d63f9c..eb6d3aa2c8cc 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
 	return false;
 }
 
+static bool ro_register(u32 reg)
+{
+	if (reg & RING_FORCE_TO_NONPRIV_RD)
+		return true;
+
+	return false;
+}
+
+static int whitelist_writable_count(struct intel_engine_cs *engine)
+{
+	int count = engine->whitelist.count;
+	int i;
+
+	for (i = 0; i < engine->whitelist.count; i++) {
+		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+		if (ro_register(reg))
+			count--;
+	}
+
+	return count;
+}
+
 static int check_dirty_whitelist(struct i915_gem_context *ctx,
 				 struct intel_engine_cs *engine)
 {
@@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
 		if (wo_register(engine, reg))
 			continue;
 
+		if (ro_register(reg))
+			continue;
+
 		srm = MI_STORE_REGISTER_MEM;
 		lrm = MI_LOAD_REGISTER_MEM;
 		if (INTEL_GEN(ctx->i915) >= 8)
@@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
 
 	for (i = 0; i < engine->whitelist.count; i++) {
 		u64 offset = results->node.start + sizeof(u32) * i;
+		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+		/* Clear RD only and WR only flags */
+		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
 
 		*cs++ = srm;
-		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		*cs++ = reg;
 		*cs++ = lower_32_bits(offset);
 		*cs++ = upper_32_bits(offset);
 	}
@@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
 		goto err_batch;
 	}
 
-	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
+	*cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
 	for (i = 0; i < engine->whitelist.count; i++) {
-		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
+
+		if (ro_register(reg))
+			continue;
+
+		*cs++ = reg;
 		*cs++ = 0xffffffff;
 	}
 	*cs++ = MI_BATCH_BUFFER_END;
@@ -956,7 +991,7 @@ static int live_isolated_whitelist(void *arg)
 	}
 
 	for_each_engine(engine, i915, id) {
-		if (!engine->whitelist.count)
+		if (!whitelist_writable_count(engine))
 			continue;
 
 		/* Read default values */
-- 
2.21.0.5.gaeb582a983

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

✓ Fi.CI.BAT: success for Update whitelist support for new hardware (rev2)

[Patchwork]

== Series Details ==

Series: Update whitelist support for new hardware (rev2)
URL   : https://patchwork.freedesktop.org/series/62076/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6289 -> Patchwork_13319
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [PASS][1] -> [INCOMPLETE][2] ([fdo#107718])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  
#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - fi-icl-y:           [INCOMPLETE][3] ([fdo#107713] / [fdo#109100]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-icl-y/igt@gem_ctx_create@basic-files.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-icl-y/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_create@basic:
    - fi-icl-u2:          [INCOMPLETE][5] ([fdo#107713]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-icl-u2/igt@gem_exec_create@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-icl-u2/igt@gem_exec_create@basic.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][7] ([fdo#109485]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [DMESG-WARN][9] ([fdo#102614]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [DMESG-WARN][11] ([fdo#106387]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


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

  Additional (1): fi-icl-u3 
  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-bsw-n3050 fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-pnv-d510 fi-icl-dsi fi-bdw-samus 


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

  * Linux: CI_DRM_6289 -> Patchwork_13319

  CI_DRM_6289: 897314e20de3b565ab91637f69817ebeddde07ef @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5059: 1f67ee0d09d6513f487f2be74aae9700e755258a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13319: c69779fec5decc771ea5ab064964b8e4065de760 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

c69779fec5de drm/i915: Update workarounds selftest for read only regs
3734e4d0d4cc drm/i915: Add whitelist workarounds for ICL
4d5289caa541 drm/i915: Support whitelist workarounds on all engines
09c357e609ef drm/i915: Support flags in whitlist WAs

== Logs ==

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

Re: [PATCH 1/4] drm/i915: Support flags in whitlist WAs

[Tvrtko Ursulin]

On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware adds flags to the whitelist work-around register. These
> allow per access direction privileges and ranges.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 165b0a45e009..ae82340fff45 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
>   }
>   
>   static void
> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   {
>   	struct i915_wa wa = {
>   		.reg = reg
> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>   		return;
>   
> +	wa.reg.reg |= flags;
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +{
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +}
> +
>   static void gen9_whitelist_build(struct i915_wa_list *w)
>   {
>   	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7a26766ba84d..cc295a4f6e92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> +#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */

Repeating the question from previous round - isn't the RW legacy and the 
two new flags below are CFL+ & Gen11+?

> +#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
> 

If I am right that the comment should be moved a line down, then with 
that, or if I am not right like it is:

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 2/4] drm/i915: Support whitelist workarounds on all engines

[Tvrtko Ursulin]

On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware requires setting up whitelists on engines other than
> render. So, extend the whitelist code to support all engines.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 66 +++++++++++++++------
>   1 file changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index ae82340fff45..5308a0864e78 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1043,48 +1043,79 @@ static void gen9_whitelist_build(struct i915_wa_list *w)
>   	whitelist_reg(w, GEN8_HDC_CHICKEN1);
>   }
>   
> -static void skl_whitelist_build(struct i915_wa_list *w)
> +static void skl_whitelist_build(struct intel_engine_cs *engine)
>   {
> +	struct i915_wa_list *w = &engine->whitelist;
> +
> +	if (engine->class != RENDER_CLASS)
> +		return;
> +
>   	gen9_whitelist_build(w);
>   
>   	/* WaDisableLSQCROPERFforOCL:skl */
>   	whitelist_reg(w, GEN8_L3SQCREG4);
>   }
>   
> -static void bxt_whitelist_build(struct i915_wa_list *w)
> +static void bxt_whitelist_build(struct intel_engine_cs *engine)
>   {
> -	gen9_whitelist_build(w);
> +	if (engine->class != RENDER_CLASS)
> +		return;
> +
> +	gen9_whitelist_build(&engine->whitelist);
>   }
>   
> -static void kbl_whitelist_build(struct i915_wa_list *w)
> +static void kbl_whitelist_build(struct intel_engine_cs *engine)
>   {
> +	struct i915_wa_list *w = &engine->whitelist;
> +
> +	if (engine->class != RENDER_CLASS)
> +		return;
> +
>   	gen9_whitelist_build(w);
>   
>   	/* WaDisableLSQCROPERFforOCL:kbl */
>   	whitelist_reg(w, GEN8_L3SQCREG4);
>   }
>   
> -static void glk_whitelist_build(struct i915_wa_list *w)
> +static void glk_whitelist_build(struct intel_engine_cs *engine)
>   {
> +	struct i915_wa_list *w = &engine->whitelist;
> +
> +	if (engine->class != RENDER_CLASS)
> +		return;
> +
>   	gen9_whitelist_build(w);
>   
>   	/* WA #0862: Userspace has to set "Barrier Mode" to avoid hangs. */
>   	whitelist_reg(w, GEN9_SLICE_COMMON_ECO_CHICKEN1);
>   }
>   
> -static void cfl_whitelist_build(struct i915_wa_list *w)
> +static void cfl_whitelist_build(struct intel_engine_cs *engine)
>   {
> -	gen9_whitelist_build(w);
> +	if (engine->class != RENDER_CLASS)
> +		return;
> +
> +	gen9_whitelist_build(&engine->whitelist);
>   }
>   
> -static void cnl_whitelist_build(struct i915_wa_list *w)
> +static void cnl_whitelist_build(struct intel_engine_cs *engine)
>   {
> +	struct i915_wa_list *w = &engine->whitelist;
> +
> +	if (engine->class != RENDER_CLASS)
> +		return;
> +
>   	/* WaEnablePreemptionGranularityControlByUMD:cnl */
>   	whitelist_reg(w, GEN8_CS_CHICKEN1);
>   }
>   
> -static void icl_whitelist_build(struct i915_wa_list *w)
> +static void icl_whitelist_build(struct intel_engine_cs *engine)
>   {
> +	struct i915_wa_list *w = &engine->whitelist;
> +
> +	if (engine->class != RENDER_CLASS)
> +		return;
> +
>   	/* WaAllowUMDToModifyHalfSliceChicken7:icl */
>   	whitelist_reg(w, GEN9_HALF_SLICE_CHICKEN7);
>   
> @@ -1100,25 +1131,22 @@ void intel_engine_init_whitelist(struct intel_engine_cs *engine)
>   	struct drm_i915_private *i915 = engine->i915;
>   	struct i915_wa_list *w = &engine->whitelist;
>   
> -	if (engine->class != RENDER_CLASS)
> -		return;
> -
>   	wa_init_start(w, "whitelist");
>   
>   	if (IS_GEN(i915, 11))
> -		icl_whitelist_build(w);
> +		icl_whitelist_build(engine);
>   	else if (IS_CANNONLAKE(i915))
> -		cnl_whitelist_build(w);
> +		cnl_whitelist_build(engine);
>   	else if (IS_COFFEELAKE(i915))
> -		cfl_whitelist_build(w);
> +		cfl_whitelist_build(engine);
>   	else if (IS_GEMINILAKE(i915))
> -		glk_whitelist_build(w);
> +		glk_whitelist_build(engine);
>   	else if (IS_KABYLAKE(i915))
> -		kbl_whitelist_build(w);
> +		kbl_whitelist_build(engine);
>   	else if (IS_BROXTON(i915))
> -		bxt_whitelist_build(w);
> +		bxt_whitelist_build(engine);
>   	else if (IS_SKYLAKE(i915))
> -		skl_whitelist_build(w);
> +		skl_whitelist_build(engine);
>   	else if (INTEL_GEN(i915) <= 8)
>   		return;
>   	else
> 

 From last round:

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 3/4] drm/i915: Add whitelist workarounds for ICL

[Tvrtko Ursulin]

On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Updated whitelist table for ICL.
> 
> v2: Reduce changes to just those required for media driver until
> the selftest can be updated to support the new features of the
> other entries.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 38 +++++++++++++++------
>   1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 5308a0864e78..d37ebcddb963 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1113,17 +1113,33 @@ static void icl_whitelist_build(struct intel_engine_cs *engine)
>   {
>   	struct i915_wa_list *w = &engine->whitelist;
>   
> -	if (engine->class != RENDER_CLASS)
> -		return;
> -
> -	/* WaAllowUMDToModifyHalfSliceChicken7:icl */
> -	whitelist_reg(w, GEN9_HALF_SLICE_CHICKEN7);
> -
> -	/* WaAllowUMDToModifySamplerMode:icl */
> -	whitelist_reg(w, GEN10_SAMPLER_MODE);
> -
> -	/* WaEnableStateCacheRedirectToCS:icl */
> -	whitelist_reg(w, GEN9_SLICE_COMMON_ECO_CHICKEN1);
> +	switch (engine->class) {
> +	case RENDER_CLASS:
> +		/* WaAllowUMDToModifyHalfSliceChicken7:icl */
> +		whitelist_reg(w, GEN9_HALF_SLICE_CHICKEN7);
> +
> +		/* WaAllowUMDToModifySamplerMode:icl */
> +		whitelist_reg(w, GEN10_SAMPLER_MODE);
> +
> +		/* WaEnableStateCacheRedirectToCS:icl */
> +		whitelist_reg(w, GEN9_SLICE_COMMON_ECO_CHICKEN1);
> +		break;
> +
> +	case VIDEO_DECODE_CLASS:
> +		/* hucStatusRegOffset */
> +		whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
> +				  RING_FORCE_TO_NONPRIV_RD);
> +		/* hucUKernelHdrInfoRegOffset */
> +		whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
> +				  RING_FORCE_TO_NONPRIV_RD);
> +		/* hucStatus2RegOffset */
> +		whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
> +				  RING_FORCE_TO_NONPRIV_RD);
> +		break;
> +
> +	default:
> +		break;
> +	}
>   }
>   
>   void intel_engine_init_whitelist(struct intel_engine_cs *engine)
> 

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 1/4] drm/i915: Support flags in whitlist WAs

[Tvrtko Ursulin]

On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware adds flags to the whitelist work-around register. These
> allow per access direction privileges and ranges.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 165b0a45e009..ae82340fff45 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
>   }
>   
>   static void
> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   {
>   	struct i915_wa wa = {
>   		.reg = reg
> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>   		return;

Actually how about we add somewhere around here:

GEM_BUG_ON(hweight32(flags & (..RD | .. WR)) > 1);

To ensure correct usage of the flags?

Regards,

Tvrtko

> +	wa.reg.reg |= flags;
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +{
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +}
> +
>   static void gen9_whitelist_build(struct i915_wa_list *w)
>   {
>   	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7a26766ba84d..cc295a4f6e92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> +#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Update workarounds selftest for read only regs

[Tvrtko Ursulin]

On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: "Robert M. Fosha" <robert.m.fosha@intel.com>
> 
> Updates the live_workarounds selftest to handle whitelisted
> registers that are flagged as read only.
> 
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
>   1 file changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index c8d335d63f9c..eb6d3aa2c8cc 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   	return false;
>   }
>   
> +static bool ro_register(u32 reg)
> +{
> +	if (reg & RING_FORCE_TO_NONPRIV_RD)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int whitelist_writable_count(struct intel_engine_cs *engine)
> +{
> +	int count = engine->whitelist.count;
> +	int i;
> +
> +	for (i = 0; i < engine->whitelist.count; i++) {
> +		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +
> +		if (ro_register(reg))
> +			count--;
> +	}
> +
> +	return count;
> +}
> +
>   static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   				 struct intel_engine_cs *engine)
>   {
> @@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		if (wo_register(engine, reg))
>   			continue;
>   
> +		if (ro_register(reg))
> +			continue;
> +
>   		srm = MI_STORE_REGISTER_MEM;
>   		lrm = MI_LOAD_REGISTER_MEM;
>   		if (INTEL_GEN(ctx->i915) >= 8)
> @@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
>   
>   	for (i = 0; i < engine->whitelist.count; i++) {
>   		u64 offset = results->node.start + sizeof(u32) * i;
> +		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +
> +		/* Clear RD only and WR only flags */
> +		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>   
>   		*cs++ = srm;
> -		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		*cs++ = reg;
>   		*cs++ = lower_32_bits(offset);
>   		*cs++ = upper_32_bits(offset);
>   	}
> @@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct i915_gem_context *ctx,
>   		goto err_batch;
>   	}
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
> +	*cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
>   	for (i = 0; i < engine->whitelist.count; i++) {
> -		*cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
> +
> +		if (ro_register(reg))
> +			continue;
> +

Are we not able to test the read-only property at all?

Regards,

Tvrtko

> +		*cs++ = reg;
>   		*cs++ = 0xffffffff;
>   	}
>   	*cs++ = MI_BATCH_BUFFER_END;
> @@ -956,7 +991,7 @@ static int live_isolated_whitelist(void *arg)
>   	}
>   
>   	for_each_engine(engine, i915, id) {
> -		if (!engine->whitelist.count)
> +		if (!whitelist_writable_count(engine))
>   			continue;
>   
>   		/* Read default values */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Update workarounds selftest for read only regs

[John Harrison]

On 6/17/2019 23:42, Tvrtko Ursulin wrote:
> On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
>> From: "Robert M. Fosha" <robert.m.fosha@intel.com>
>>
>> Updates the live_workarounds selftest to handle whitelisted
>> registers that are flagged as read only.
>>
>> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
>>   1 file changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> index c8d335d63f9c..eb6d3aa2c8cc 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> @@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs 
>> *engine, u32 reg)
>>       return false;
>>   }
>>   +static bool ro_register(u32 reg)
>> +{
>> +    if (reg & RING_FORCE_TO_NONPRIV_RD)
>> +        return true;
>> +
>> +    return false;
>> +}
>> +
>> +static int whitelist_writable_count(struct intel_engine_cs *engine)
>> +{
>> +    int count = engine->whitelist.count;
>> +    int i;
>> +
>> +    for (i = 0; i < engine->whitelist.count; i++) {
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        if (ro_register(reg))
>> +            count--;
>> +    }
>> +
>> +    return count;
>> +}
>> +
>>   static int check_dirty_whitelist(struct i915_gem_context *ctx,
>>                    struct intel_engine_cs *engine)
>>   {
>> @@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct 
>> i915_gem_context *ctx,
>>           if (wo_register(engine, reg))
>>               continue;
>>   +        if (ro_register(reg))
>> +            continue;
>> +
>>           srm = MI_STORE_REGISTER_MEM;
>>           lrm = MI_LOAD_REGISTER_MEM;
>>           if (INTEL_GEN(ctx->i915) >= 8)
>> @@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct 
>> i915_gem_context *ctx,
>>         for (i = 0; i < engine->whitelist.count; i++) {
>>           u64 offset = results->node.start + sizeof(u32) * i;
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        /* Clear RD only and WR only flags */
>> +        reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>>             *cs++ = srm;
>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +        *cs++ = reg;
>>           *cs++ = lower_32_bits(offset);
>>           *cs++ = upper_32_bits(offset);
>>       }
>> @@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct 
>> i915_gem_context *ctx,
>>           goto err_batch;
>>       }
>>   -    *cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
>> +    *cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
>>       for (i = 0; i < engine->whitelist.count; i++) {
>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        if (ro_register(reg))
>> +            continue;
>> +
>
> Are we not able to test the read-only property at all?
>
I am sure it would be possible to make such work. But can that wait 
until the next round when we add support for ranges? And write only 
access too if any registers actually use that and there is a way to test 
that it really does do something?

I believe Robert was looking at getting something going but it wasn't 
immediately working and we urgently need to get the HUC whitelist 
updates merged to hit the next release window. So right now, it is 
sufficient to say that the user land media driver works with these 
changes and therefore the whitelisting must be working. The kernel 
selftest is just a belt and braces check on top of that and therefore 
can wait until later.

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

Re: [PATCH 1/4] drm/i915: Support flags in whitlist WAs

[John Harrison]

On 6/17/2019 23:35, Tvrtko Ursulin wrote:
> On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Newer hardware adds flags to the whitelist work-around register. These
>> allow per access direction privileges and ranges.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>>   2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 165b0a45e009..ae82340fff45 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct 
>> drm_i915_private *i915,
>>   }
>>     static void
>> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>>   {
>>       struct i915_wa wa = {
>>           .reg = reg
>> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, 
>> i915_reg_t reg)
>>       if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>>           return;
>
> Actually how about we add somewhere around here:
>
> GEM_BUG_ON(hweight32(flags & (..RD | .. WR)) > 1);
>
> To ensure correct usage of the flags?
>

It should probably be more like BUG_ON((flags & ACCESS_MASK) > 
ACCESS_MAX). It is intended to be an access enum with three valid values 
rather than a pair of flags. But yes, such a check could be added in the 
next version of the patch series along with the selftest updates.

John.


> Regards,
>
> Tvrtko
>
>> +    wa.reg.reg |= flags;
>>       _wa_add(wal, &wa);
>>   }
>>   +static void
>> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>> +{
>> +    whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
>> +}
>> +
>>   static void gen9_whitelist_build(struct i915_wa_list *w)
>>   {
>>       /* 
>> WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 7a26766ba84d..cc295a4f6e92 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>>   #define   RING_WAIT_SEMAPHORE    (1 << 10) /* gen6+ */
>>     #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + 
>> (i) * 4)
>> +#define   RING_FORCE_TO_NONPRIV_RW        (0 << 28)    /* CFL+ & 
>> Gen11+ */
>> +#define   RING_FORCE_TO_NONPRIV_RD        (1 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_WR        (2 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_1        (0 << 0)     /* CFL+ 
>> & Gen11+ */
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_4        (1 << 0)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_16    (2 << 0)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_64    (3 << 0)
>>   #define   RING_MAX_NONPRIV_SLOTS  12
>>     #define GEN7_TLB_RD_ADDR    _MMIO(0x4700)
>>

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

Re: [PATCH 1/4] drm/i915: Support flags in whitlist WAs

[Tvrtko Ursulin]

On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware adds flags to the whitelist work-around register. These
> allow per access direction privileges and ranges.

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

Regards,

Tvrtko

> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 ++++++++-
>   drivers/gpu/drm/i915/i915_reg.h             | 7 +++++++
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 165b0a45e009..ae82340fff45 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1012,7 +1012,7 @@ bool intel_gt_verify_workarounds(struct drm_i915_private *i915,
>   }
>   
>   static void
> -whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   {
>   	struct i915_wa wa = {
>   		.reg = reg
> @@ -1021,9 +1021,16 @@ whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   	if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
>   		return;
>   
> +	wa.reg.reg |= flags;
>   	_wa_add(wal, &wa);
>   }
>   
> +static void
> +whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> +{
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +}
> +
>   static void gen9_whitelist_build(struct i915_wa_list *w)
>   {
>   	/* WaVFEStateAfterPipeControlwithMediaStateClear:skl,bxt,glk,cfl */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7a26766ba84d..cc295a4f6e92 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,6 +2513,13 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> +#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Update workarounds selftest for read only regs

[Tvrtko Ursulin]

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

I don't agree that it is just belt and braces but in fact a way to check 
that the thing really is read-only like it says on the tin. Lesson here 
is that history keeps repeating panics which could have been avoided by 
running the pre-existing tests during development.

However since I don't think there is any risk to driver or system 
stability, even if the read-only property happened to be broken, which 
it probably isn't, you can have a grumpy:

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

Regards,

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

✓ Fi.CI.IGT: success for Update whitelist support for new hardware (rev2)

[Patchwork]

== Series Details ==

Series: Update whitelist support for new hardware (rev2)
URL   : https://patchwork.freedesktop.org/series/62076/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6289_full -> Patchwork_13319_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-apl7/igt@gem_ctx_isolation@rcs0-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-apl2/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_eio@in-flight-internal-immediate:
    - shard-apl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#110913 ])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-apl3/igt@gem_eio@in-flight-internal-immediate.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-apl2/igt@gem_eio@in-flight-internal-immediate.html

  * igt@gem_eio@throttle:
    - shard-kbl:          [PASS][5] -> [DMESG-WARN][6] ([fdo#110913 ])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-kbl4/igt@gem_eio@throttle.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-kbl7/igt@gem_eio@throttle.html

  * igt@gem_persistent_relocs@forked-faulting-reloc-thrashing:
    - shard-snb:          [PASS][7] -> [DMESG-WARN][8] ([fdo#110789] / [fdo#110913 ])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-snb7/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-snb2/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy-gup:
    - shard-snb:          [PASS][9] -> [DMESG-WARN][10] ([fdo#110913 ]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-snb7/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html

  * igt@i915_pm_rpm@gem-mmap-cpu:
    - shard-iclb:         [PASS][11] -> [INCOMPLETE][12] ([fdo#107713] / [fdo#108840])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb6/igt@i915_pm_rpm@gem-mmap-cpu.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb7/igt@i915_pm_rpm@gem-mmap-cpu.html

  * igt@i915_selftest@live_evict:
    - shard-kbl:          [PASS][13] -> [INCOMPLETE][14] ([fdo#103665] / [fdo#110938])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-kbl2/igt@i915_selftest@live_evict.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-kbl4/igt@i915_selftest@live_evict.html

  * igt@kms_busy@extended-modeset-hang-newfb-render-c:
    - shard-iclb:         [PASS][15] -> [INCOMPLETE][16] ([fdo#107713])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb2/igt@kms_busy@extended-modeset-hang-newfb-render-c.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb7/igt@kms_busy@extended-modeset-hang-newfb-render-c.html

  * igt@kms_cursor_edge_walk@pipe-b-64x64-bottom-edge:
    - shard-snb:          [PASS][17] -> [SKIP][18] ([fdo#109271] / [fdo#109278])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-snb7/igt@kms_cursor_edge_walk@pipe-b-64x64-bottom-edge.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-snb2/igt@kms_cursor_edge_walk@pipe-b-64x64-bottom-edge.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-snb:          [PASS][19] -> [INCOMPLETE][20] ([fdo#105411])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-snb2/igt@kms_flip@flip-vs-suspend.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-snb1/igt@kms_flip@flip-vs-suspend.html
    - shard-glk:          [PASS][21] -> [INCOMPLETE][22] ([fdo#103359] / [k.org#198133])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-glk3/igt@kms_flip@flip-vs-suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-glk6/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-skl:          [PASS][23] -> [INCOMPLETE][24] ([fdo#107773] / [fdo#109507])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-skl5/igt@kms_flip@flip-vs-suspend-interruptible.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-skl2/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@basic:
    - shard-iclb:         [PASS][25] -> [FAIL][26] ([fdo#103167]) +3 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb5/igt@kms_frontbuffer_tracking@basic.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb6/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-indfb-pgflip-blt:
    - shard-hsw:          [PASS][27] -> [SKIP][28] ([fdo#109271]) +11 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-hsw4/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-indfb-pgflip-blt.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-indfb-pgflip-blt.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - shard-kbl:          [PASS][29] -> [DMESG-WARN][30] ([fdo#108566])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-kbl2/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         [PASS][31] -> [FAIL][32] ([fdo#103166])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [PASS][33] -> [SKIP][34] ([fdo#109441]) +2 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb7/igt@kms_psr@psr2_primary_page_flip.html

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-snb:          [PASS][35] -> [SKIP][36] ([fdo#109271])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-snb7/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-snb2/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html

  
#### Possible fixes ####

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [SKIP][37] ([fdo#110854]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb5/igt@gem_exec_balancer@smoke.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb1/igt@gem_exec_balancer@smoke.html

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrash-inactive:
    - shard-apl:          [INCOMPLETE][39] ([fdo#103927]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-apl1/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrash-inactive.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-apl6/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrash-inactive.html

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
    - shard-snb:          [DMESG-WARN][41] ([fdo#110789] / [fdo#110913 ]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-snb1/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-snb4/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-apl:          [DMESG-WARN][43] ([fdo#110913 ]) -> [PASS][44] +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-apl8/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-apl3/igt@gem_persistent_relocs@forked-interruptible-thrashing.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
    - shard-snb:          [DMESG-WARN][45] ([fdo#110913 ]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-snb4/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-snb1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding:
    - shard-skl:          [FAIL][47] ([fdo#103232]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-skl1/igt@kms_cursor_crc@pipe-a-cursor-256x256-sliding.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-skl:          [FAIL][49] ([fdo#102670]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-skl3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-skl9/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [SKIP][51] ([fdo#109349]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb5/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled:
    - shard-skl:          [FAIL][53] ([fdo#103184] / [fdo#103232]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-skl8/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-skl1/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-ytiled.html

  * igt@kms_flip@2x-plain-flip:
    - shard-hsw:          [SKIP][55] ([fdo#109271]) -> [PASS][56] +36 similar issues
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-hsw1/igt@kms_flip@2x-plain-flip.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-hsw7/igt@kms_flip@2x-plain-flip.html

  * igt@kms_flip@flip-vs-absolute-wf_vblank:
    - shard-hsw:          [INCOMPLETE][57] ([fdo#103540]) -> [PASS][58] +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-hsw5/igt@kms_flip@flip-vs-absolute-wf_vblank.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-hsw5/igt@kms_flip@flip-vs-absolute-wf_vblank.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][59] ([fdo#108566]) -> [PASS][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-apl1/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-apl1/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [FAIL][61] ([fdo#103167]) -> [PASS][62] +10 similar issues
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-kbl:          [DMESG-WARN][63] ([fdo#108566]) -> [PASS][64] +1 similar issue
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-kbl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-kbl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [FAIL][65] ([fdo#108145]) -> [PASS][66] +1 similar issue
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-skl1/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [SKIP][67] ([fdo#109441]) -> [PASS][68] +2 similar issues
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-iclb5/igt@kms_psr@psr2_primary_mmap_cpu.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-skl:          [INCOMPLETE][69] ([fdo#104108]) -> [PASS][70]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-skl4/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-skl8/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html

  * igt@tools_test@tools_test:
    - shard-glk:          [SKIP][71] ([fdo#109271]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-glk1/igt@tools_test@tools_test.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-glk2/igt@tools_test@tools_test.html

  
#### Warnings ####

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
    - shard-hsw:          [SKIP][73] ([fdo#109271]) -> [FAIL][74] ([fdo#105767])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-hsw1/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-hsw5/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt:
    - shard-skl:          [FAIL][75] ([fdo#103167]) -> [FAIL][76] ([fdo#108040])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/shard-skl8/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/shard-skl9/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt.html

  
  [fdo#102670]: https://bugs.freedesktop.org/show_bug.cgi?id=102670
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [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#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#110789]: https://bugs.freedesktop.org/show_bug.cgi?id=110789
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#110913 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110913 
  [fdo#110938]: https://bugs.freedesktop.org/show_bug.cgi?id=110938
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


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

  No changes in participating hosts


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

  * Linux: CI_DRM_6289 -> Patchwork_13319

  CI_DRM_6289: 897314e20de3b565ab91637f69817ebeddde07ef @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5059: 1f67ee0d09d6513f487f2be74aae9700e755258a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13319: c69779fec5decc771ea5ab064964b8e4065de760 @ 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_13319/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for Update whitelist support for new hardware (rev2)

[Tvrtko Ursulin]

On 18/06/2019 02:50, Patchwork wrote:
> == Series Details ==
> 
> Series: Update whitelist support for new hardware (rev2)
> URL   : https://patchwork.freedesktop.org/series/62076/
> State : success
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_6289 -> Patchwork_13319
> ====================================================
> 
> Summary
> -------
> 
>    **SUCCESS**
> 
>    No regressions found.
> 
>    External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/
> 
> Known issues
> ------------
> 
>    Here are the changes found in Patchwork_13319 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>    * igt@gem_exec_suspend@basic-s4-devices:
>      - fi-blb-e6850:       [PASS][1] -> [INCOMPLETE][2] ([fdo#107718])
>     [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
>     [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
> 
>    
> #### Possible fixes ####
> 
>    * igt@gem_ctx_create@basic-files:
>      - fi-icl-y:           [INCOMPLETE][3] ([fdo#107713] / [fdo#109100]) -> [PASS][4]
>     [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-icl-y/igt@gem_ctx_create@basic-files.html
>     [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-icl-y/igt@gem_ctx_create@basic-files.html
> 
>    * igt@gem_exec_create@basic:
>      - fi-icl-u2:          [INCOMPLETE][5] ([fdo#107713]) -> [PASS][6]
>     [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-icl-u2/igt@gem_exec_create@basic.html
>     [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-icl-u2/igt@gem_exec_create@basic.html
> 
>    * igt@kms_chamelium@hdmi-hpd-fast:
>      - fi-kbl-7500u:       [FAIL][7] ([fdo#109485]) -> [PASS][8]
>     [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
>     [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
> 
>    * igt@kms_frontbuffer_tracking@basic:
>      - fi-hsw-peppy:       [DMESG-WARN][9] ([fdo#102614]) -> [PASS][10]
>     [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
>     [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
> 
>    * igt@prime_vgem@basic-fence-flip:
>      - fi-ilk-650:         [DMESG-WARN][11] ([fdo#106387]) -> [PASS][12]
>     [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6289/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
>     [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13319/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
> 
>    
>    [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
>    [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
>    [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
>    [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
>    [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
>    [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
> 
> 
> Participating hosts (43 -> 35)
> ------------------------------
> 
>    Additional (1): fi-icl-u3
>    Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-bsw-n3050 fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-pnv-d510 fi-icl-dsi fi-bdw-samus
> 
> 
> Build changes
> -------------
> 
>    * Linux: CI_DRM_6289 -> Patchwork_13319
> 
>    CI_DRM_6289: 897314e20de3b565ab91637f69817ebeddde07ef @ git://anongit.freedesktop.org/gfx-ci/linux
>    IGT_5059: 1f67ee0d09d6513f487f2be74aae9700e755258a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>    Patchwork_13319: c69779fec5decc771ea5ab064964b8e4065de760 @ git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> c69779fec5de drm/i915: Update workarounds selftest for read only regs
> 3734e4d0d4cc drm/i915: Add whitelist workarounds for ICL
> 4d5289caa541 drm/i915: Support whitelist workarounds on all engines
> 09c357e609ef drm/i915: Support flags in whitlist WAs

I have pushed this but now I ask for the read-only whitelist selftest to 
be written really ASAP.

Regards,

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

[PATCH] drm/i915: Implement read-only support in whitelist selftest

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Newer hardware supports extra feature in the whitelist registers. This
patch updates the selftest to test that entries marked as read only
are actually read only.

Also updated the read/write access definitions to make it clearer that
they are an enum field not a set of single bit flags.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_workarounds.c   |  8 +--
 .../gpu/drm/i915/gt/selftest_workarounds.c    | 53 +++++++++++++------
 drivers/gpu/drm/i915/i915_reg.h               |  9 ++--
 3 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 93caa7b6d7a9..4429ee08b20e 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
 static void
 whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
 {
-	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
+	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
 }
 
 static void gen9_whitelist_build(struct i915_wa_list *w)
@@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine = %s, instance = %d, base = 0x%X, reg
 
 		/* hucStatusRegOffset */
 		whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
-				  RING_FORCE_TO_NONPRIV_RD);
+				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
 		/* hucUKernelHdrInfoRegOffset */
 		whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
-				  RING_FORCE_TO_NONPRIV_RD);
+				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
 		/* hucStatus2RegOffset */
 		whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
-				  RING_FORCE_TO_NONPRIV_RD);
+				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
 		break;
 
 	default:
diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index eb6d3aa2c8cc..a0a88ec66861 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -399,6 +399,10 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
 	enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
 	int i;
 
+	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
+	     RING_FORCE_TO_NONPRIV_ACCESS_WR)
+		return true;
+
 	for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
 		if (wo_registers[i].platform == platform &&
 		    wo_registers[i].reg == reg)
@@ -410,7 +414,8 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
 
 static bool ro_register(u32 reg)
 {
-	if (reg & RING_FORCE_TO_NONPRIV_RD)
+	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
+	     RING_FORCE_TO_NONPRIV_ACCESS_RD)
 		return true;
 
 	return false;
@@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
 		u32 srm, lrm, rsvd;
 		u32 expect;
 		int idx;
+		bool ro_reg;
 
 		if (wo_register(engine, reg))
 			continue;
 
-		if (ro_register(reg))
-			continue;
+		ro_reg = ro_register(reg);
 
 		srm = MI_STORE_REGISTER_MEM;
 		lrm = MI_LOAD_REGISTER_MEM;
@@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
 		}
 
 		GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
-		rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
-		if (!rsvd) {
-			pr_err("%s: Unable to write to whitelisted register %x\n",
-			       engine->name, reg);
-			err = -EINVAL;
-			goto out_unpin;
+		if (ro_reg) {
+			rsvd = 0xFFFFFFFF;
+		} else {
+			rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
+			if (!rsvd) {
+				pr_err("%s: Unable to write to whitelisted register %x\n",
+				       engine->name, reg);
+				err = -EINVAL;
+				goto out_unpin;
+			}
 		}
 
 		expect = results[0];
 		idx = 1;
 		for (v = 0; v < ARRAY_SIZE(values); v++) {
-			expect = reg_write(expect, values[v], rsvd);
+			if (ro_reg)
+				expect = results[0];
+			else
+				expect = reg_write(expect, values[v], rsvd);
+
 			if (results[idx] != expect)
 				err++;
 			idx++;
 		}
 		for (v = 0; v < ARRAY_SIZE(values); v++) {
-			expect = reg_write(expect, ~values[v], rsvd);
+			if (ro_reg)
+				expect = results[0];
+			else
+				expect = reg_write(expect, ~values[v], rsvd);
+
 			if (results[idx] != expect)
 				err++;
 			idx++;
@@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
 			for (v = 0; v < ARRAY_SIZE(values); v++) {
 				u32 w = values[v];
 
-				expect = reg_write(expect, w, rsvd);
+				if (ro_reg)
+					expect = results[0];
+				else
+					expect = reg_write(expect, w, rsvd);
 				pr_info("Wrote %08x, read %08x, expect %08x\n",
 					w, results[idx], expect);
 				idx++;
@@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
 			for (v = 0; v < ARRAY_SIZE(values); v++) {
 				u32 w = ~values[v];
 
-				expect = reg_write(expect, w, rsvd);
+				if (ro_reg)
+					expect = results[0];
+				else
+					expect = reg_write(expect, w, rsvd);
 				pr_info("Wrote %08x, read %08x, expect %08x\n",
 					w, results[idx], expect);
 				idx++;
@@ -762,8 +785,8 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
 		u64 offset = results->node.start + sizeof(u32) * i;
 		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
 
-		/* Clear RD only and WR only flags */
-		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
+		/* Clear access permission field */
+		reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
 
 		*cs++ = srm;
 		*cs++ = reg;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cc295a4f6e92..ba22e3b8f77e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2513,13 +2513,16 @@ enum i915_power_well_id {
 #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
 
 #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
-#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
-#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
-#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
+#define   RING_FORCE_TO_NONPRIV_ACCESS_RW	(0 << 28)    /* CFL+ & Gen11+ */
+#define   RING_FORCE_TO_NONPRIV_ACCESS_RD	(1 << 28)
+#define   RING_FORCE_TO_NONPRIV_ACCESS_WR	(2 << 28)
+#define   RING_FORCE_TO_NONPRIV_ACCESS_INVALID	(3 << 28)
+#define   RING_FORCE_TO_NONPRIV_ACCESS_MASK	(3 << 28)
 #define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
 #define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
 #define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
 #define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
+#define   RING_FORCE_TO_NONPRIV_RANGE_MASK	(3 << 0)
 #define   RING_MAX_NONPRIV_SLOTS  12
 
 #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
-- 
2.21.0.5.gaeb582a983

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

Re: [PATCH] drm/i915: Implement read-only support in whitelist selftest

[John Harrison]

Tvrtko, does this look plausible?

It seems to work for me in that it passes on ICL with the new read-only 
registers. I'm not sure if there is a valid way to detect whether the 
registers are actually readable though. How would the test know what is 
a valid value? If one assumes that one gets back zero from an invalid 
read, how does one know that the read-only register is not supposed to 
return zero at this point anyway?

Or is it worth just putting in a test for non-zero and if we do find a 
register that can validly return zero then we special case that one and 
ignore it?

John.


On 6/18/2019 12:54, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Newer hardware supports extra feature in the whitelist registers. This
> patch updates the selftest to test that entries marked as read only
> are actually read only.
>
> Also updated the read/write access definitions to make it clearer that
> they are an enum field not a set of single bit flags.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c   |  8 +--
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 53 +++++++++++++------
>   drivers/gpu/drm/i915/i915_reg.h               |  9 ++--
>   3 files changed, 48 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 93caa7b6d7a9..4429ee08b20e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   static void
>   whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   {
> -	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
>   }
>   
>   static void gen9_whitelist_build(struct i915_wa_list *w)
> @@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine = %s, instance = %d, base = 0x%X, reg
>   
>   		/* hucStatusRegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		/* hucUKernelHdrInfoRegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		/* hucStatus2RegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		break;
>   
>   	default:
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index eb6d3aa2c8cc..a0a88ec66861 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -399,6 +399,10 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   	enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
>   	int i;
>   
> +	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> +	     RING_FORCE_TO_NONPRIV_ACCESS_WR)
> +		return true;
> +
>   	for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
>   		if (wo_registers[i].platform == platform &&
>   		    wo_registers[i].reg == reg)
> @@ -410,7 +414,8 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   
>   static bool ro_register(u32 reg)
>   {
> -	if (reg & RING_FORCE_TO_NONPRIV_RD)
> +	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> +	     RING_FORCE_TO_NONPRIV_ACCESS_RD)
>   		return true;
>   
>   	return false;
> @@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		u32 srm, lrm, rsvd;
>   		u32 expect;
>   		int idx;
> +		bool ro_reg;
>   
>   		if (wo_register(engine, reg))
>   			continue;
>   
> -		if (ro_register(reg))
> -			continue;
> +		ro_reg = ro_register(reg);
>   
>   		srm = MI_STORE_REGISTER_MEM;
>   		lrm = MI_LOAD_REGISTER_MEM;
> @@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		}
>   
>   		GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
> -		rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> -		if (!rsvd) {
> -			pr_err("%s: Unable to write to whitelisted register %x\n",
> -			       engine->name, reg);
> -			err = -EINVAL;
> -			goto out_unpin;
> +		if (ro_reg) {
> +			rsvd = 0xFFFFFFFF;
> +		} else {
> +			rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> +			if (!rsvd) {
> +				pr_err("%s: Unable to write to whitelisted register %x\n",
> +				       engine->name, reg);
> +				err = -EINVAL;
> +				goto out_unpin;
> +			}
>   		}
>   
>   		expect = results[0];
>   		idx = 1;
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
>   		}
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, ~values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, ~values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
> @@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = ~values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -762,8 +785,8 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
>   		u64 offset = results->node.start + sizeof(u32) * i;
>   		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>   
> -		/* Clear RD only and WR only flags */
> -		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
> +		/* Clear access permission field */
> +		reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
>   
>   		*cs++ = srm;
>   		*cs++ = reg;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc295a4f6e92..ba22e3b8f77e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,13 +2513,16 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> -#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
> -#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> -#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RW	(0 << 28)    /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RD	(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_WR	(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_INVALID	(3 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_MASK	(3 << 28)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
>   #define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_MASK	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)

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

✗ Fi.CI.BAT: failure for drm/i915: Implement read-only support in whitelist selftest

[Patchwork]

== Series Details ==

Series: drm/i915: Implement read-only support in whitelist selftest
URL   : https://patchwork.freedesktop.org/series/62339/
State : failure

== Summary ==

Applying: drm/i915: Implement read-only support in whitelist selftest
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/gt/intel_workarounds.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 drm/i915: Implement read-only support in whitelist selftest
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [PATCH] drm/i915: Implement read-only support in whitelist selftest

[Tvrtko Ursulin]

On 18/06/2019 21:08, John Harrison wrote:
> Tvrtko, does this look plausible?
> 
> It seems to work for me in that it passes on ICL with the new read-only 
> registers. I'm not sure if there is a valid way to detect whether the 
> registers are actually readable though. How would the test know what is 
> a valid value? If one assumes that one gets back zero from an invalid 
> read, how does one know that the read-only register is not supposed to 
> return zero at this point anyway?
> 
> Or is it worth just putting in a test for non-zero and if we do find a 
> register that can validly return zero then we special case that one and 
> ignore it?

I was thinking we just read the register and then verify it is unchanged 
after every existing write to it.

Regards,

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

Re: [PATCH] drm/i915: Implement read-only support in whitelist selftest

[Tvrtko Ursulin]

On 18/06/2019 20:54, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware supports extra feature in the whitelist registers. This
> patch updates the selftest to test that entries marked as read only
> are actually read only.
> 
> Also updated the read/write access definitions to make it clearer that
> they are an enum field not a set of single bit flags.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c   |  8 +--
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 53 +++++++++++++------
>   drivers/gpu/drm/i915/i915_reg.h               |  9 ++--
>   3 files changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 93caa7b6d7a9..4429ee08b20e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)

Reminder to add a GEM_BUG_ON if invalid flags are passed in.

>   static void
>   whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   {
> -	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
>   }
>   
>   static void gen9_whitelist_build(struct i915_wa_list *w)
> @@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine = %s, instance = %d, base = 0x%X, reg
>   
>   		/* hucStatusRegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		/* hucUKernelHdrInfoRegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		/* hucStatus2RegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		break;
>   
>   	default:
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index eb6d3aa2c8cc..a0a88ec66861 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -399,6 +399,10 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   	enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
>   	int i;
>   
> +	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> +	     RING_FORCE_TO_NONPRIV_ACCESS_WR)
> +		return true;
> +
>   	for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
>   		if (wo_registers[i].platform == platform &&
>   		    wo_registers[i].reg == reg)
> @@ -410,7 +414,8 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   
>   static bool ro_register(u32 reg)
>   {
> -	if (reg & RING_FORCE_TO_NONPRIV_RD)
> +	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> +	     RING_FORCE_TO_NONPRIV_ACCESS_RD)
>   		return true;
>   
>   	return false;
> @@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		u32 srm, lrm, rsvd;
>   		u32 expect;
>   		int idx;
> +		bool ro_reg;
>   
>   		if (wo_register(engine, reg))
>   			continue;
>   
> -		if (ro_register(reg))
> -			continue;
> +		ro_reg = ro_register(reg);
>   
>   		srm = MI_STORE_REGISTER_MEM;
>   		lrm = MI_LOAD_REGISTER_MEM;
> @@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		}
>   
>   		GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
> -		rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> -		if (!rsvd) {
> -			pr_err("%s: Unable to write to whitelisted register %x\n",
> -			       engine->name, reg);
> -			err = -EINVAL;
> -			goto out_unpin;
> +		if (ro_reg) {
> +			rsvd = 0xFFFFFFFF;

I guess it doesn't matter what value you set since it is only used on 
pr_info on ro_reg paths?

> +		} else {
> +			rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> +			if (!rsvd) {
> +				pr_err("%s: Unable to write to whitelisted register %x\n",
> +				       engine->name, reg);
> +				err = -EINVAL;
> +				goto out_unpin;
> +			}
>   		}
>   
>   		expect = results[0];
>   		idx = 1;
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
>   		}
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, ~values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, ~values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
> @@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = ~values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -762,8 +785,8 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
>   		u64 offset = results->node.start + sizeof(u32) * i;
>   		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>   
> -		/* Clear RD only and WR only flags */
> -		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
> +		/* Clear access permission field */
> +		reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
>   
>   		*cs++ = srm;
>   		*cs++ = reg;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc295a4f6e92..ba22e3b8f77e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,13 +2513,16 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> -#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
> -#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> -#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RW	(0 << 28)    /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RD	(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_WR	(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_INVALID	(3 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_MASK	(3 << 28)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
>   #define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_MASK	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
> 

This in fact looks fine to me.

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] drm/i915: Implement read-only support in whitelist selftest

[Tvrtko Ursulin]

Hi,

You will need to send this not as reply to this thread so it is picked 
up by CI and then can be merged.

But please also add a patch which adds that GEM_BUG_ON reg->flags check 
we talked about.

Regards,

Tvrtko

On 18/06/2019 20:54, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Newer hardware supports extra feature in the whitelist registers. This
> patch updates the selftest to test that entries marked as read only
> are actually read only.
> 
> Also updated the read/write access definitions to make it clearer that
> they are an enum field not a set of single bit flags.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c   |  8 +--
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 53 +++++++++++++------
>   drivers/gpu/drm/i915/i915_reg.h               |  9 ++--
>   3 files changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 93caa7b6d7a9..4429ee08b20e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
>   static void
>   whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>   {
> -	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> +	whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
>   }
>   
>   static void gen9_whitelist_build(struct i915_wa_list *w)
> @@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine = %s, instance = %d, base = 0x%X, reg
>   
>   		/* hucStatusRegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		/* hucUKernelHdrInfoRegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		/* hucStatus2RegOffset */
>   		whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
> -				  RING_FORCE_TO_NONPRIV_RD);
> +				  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>   		break;
>   
>   	default:
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index eb6d3aa2c8cc..a0a88ec66861 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -399,6 +399,10 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   	enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
>   	int i;
>   
> +	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> +	     RING_FORCE_TO_NONPRIV_ACCESS_WR)
> +		return true;
> +
>   	for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
>   		if (wo_registers[i].platform == platform &&
>   		    wo_registers[i].reg == reg)
> @@ -410,7 +414,8 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>   
>   static bool ro_register(u32 reg)
>   {
> -	if (reg & RING_FORCE_TO_NONPRIV_RD)
> +	if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> +	     RING_FORCE_TO_NONPRIV_ACCESS_RD)
>   		return true;
>   
>   	return false;
> @@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		u32 srm, lrm, rsvd;
>   		u32 expect;
>   		int idx;
> +		bool ro_reg;
>   
>   		if (wo_register(engine, reg))
>   			continue;
>   
> -		if (ro_register(reg))
> -			continue;
> +		ro_reg = ro_register(reg);
>   
>   		srm = MI_STORE_REGISTER_MEM;
>   		lrm = MI_LOAD_REGISTER_MEM;
> @@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		}
>   
>   		GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
> -		rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> -		if (!rsvd) {
> -			pr_err("%s: Unable to write to whitelisted register %x\n",
> -			       engine->name, reg);
> -			err = -EINVAL;
> -			goto out_unpin;
> +		if (ro_reg) {
> +			rsvd = 0xFFFFFFFF;
> +		} else {
> +			rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> +			if (!rsvd) {
> +				pr_err("%s: Unable to write to whitelisted register %x\n",
> +				       engine->name, reg);
> +				err = -EINVAL;
> +				goto out_unpin;
> +			}
>   		}
>   
>   		expect = results[0];
>   		idx = 1;
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
>   		}
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, ~values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, ~values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
> @@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = ~values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -762,8 +785,8 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
>   		u64 offset = results->node.start + sizeof(u32) * i;
>   		u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>   
> -		/* Clear RD only and WR only flags */
> -		reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
> +		/* Clear access permission field */
> +		reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
>   
>   		*cs++ = srm;
>   		*cs++ = reg;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc295a4f6e92..ba22e3b8f77e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,13 +2513,16 @@ enum i915_power_well_id {
>   #define   RING_WAIT_SEMAPHORE	(1 << 10) /* gen6+ */
>   
>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> -#define   RING_FORCE_TO_NONPRIV_RW		(0 << 28)    /* CFL+ & Gen11+ */
> -#define   RING_FORCE_TO_NONPRIV_RD		(1 << 28)
> -#define   RING_FORCE_TO_NONPRIV_WR		(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RW	(0 << 28)    /* CFL+ & Gen11+ */
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RD	(1 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_WR	(2 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_INVALID	(3 << 28)
> +#define   RING_FORCE_TO_NONPRIV_ACCESS_MASK	(3 << 28)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_1		(0 << 0)     /* CFL+ & Gen11+ */
>   #define   RING_FORCE_TO_NONPRIV_RANGE_4		(1 << 0)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_16	(2 << 0)
>   #define   RING_FORCE_TO_NONPRIV_RANGE_64	(3 << 0)
> +#define   RING_FORCE_TO_NONPRIV_RANGE_MASK	(3 << 0)
>   #define   RING_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Implement read-only support in whitelist selftest

[Tvrtko Ursulin]

Ping.

We agreed to follow up with a test ASAP after merging.

Here's another feature request for you: Add engine->name logging to 
wa_init_start in intel_engine_init_whitelist. Because with the change to 
add whitelist on other engines there are now multiple identical lines in 
dmesg.

To sum up that's three todo items:

1. Resend the selftest for CI.
2. Add GEM_BUG_ON for reg->flags checking invalid flag usage.
3. Improve dmesg so we know which engine got how many whitelist entries.

Thanks,

Tvrtko

On 20/06/2019 16:43, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> You will need to send this not as reply to this thread so it is picked 
> up by CI and then can be merged.
> 
> But please also add a patch which adds that GEM_BUG_ON reg->flags check 
> we talked about.
> 
> Regards,
> 
> Tvrtko
> 
> On 18/06/2019 20:54, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Newer hardware supports extra feature in the whitelist registers. This
>> patch updates the selftest to test that entries marked as read only
>> are actually read only.
>>
>> Also updated the read/write access definitions to make it clearer that
>> they are an enum field not a set of single bit flags.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_workarounds.c   |  8 +--
>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 53 +++++++++++++------
>>   drivers/gpu/drm/i915/i915_reg.h               |  9 ++--
>>   3 files changed, 48 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 93caa7b6d7a9..4429ee08b20e 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, 
>> i915_reg_t reg, u32 flags)
>>   static void
>>   whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>>   {
>> -    whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
>> +    whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
>>   }
>>   static void gen9_whitelist_build(struct i915_wa_list *w)
>> @@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine = 
>> %s, instance = %d, base = 0x%X, reg
>>           /* hucStatusRegOffset */
>>           whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
>> -                  RING_FORCE_TO_NONPRIV_RD);
>> +                  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>           /* hucUKernelHdrInfoRegOffset */
>>           whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
>> -                  RING_FORCE_TO_NONPRIV_RD);
>> +                  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>           /* hucStatus2RegOffset */
>>           whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
>> -                  RING_FORCE_TO_NONPRIV_RD);
>> +                  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>           break;
>>       default:
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> index eb6d3aa2c8cc..a0a88ec66861 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> @@ -399,6 +399,10 @@ static bool wo_register(struct intel_engine_cs 
>> *engine, u32 reg)
>>       enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
>>       int i;
>> +    if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
>> +         RING_FORCE_TO_NONPRIV_ACCESS_WR)
>> +        return true;
>> +
>>       for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
>>           if (wo_registers[i].platform == platform &&
>>               wo_registers[i].reg == reg)
>> @@ -410,7 +414,8 @@ static bool wo_register(struct intel_engine_cs 
>> *engine, u32 reg)
>>   static bool ro_register(u32 reg)
>>   {
>> -    if (reg & RING_FORCE_TO_NONPRIV_RD)
>> +    if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
>> +         RING_FORCE_TO_NONPRIV_ACCESS_RD)
>>           return true;
>>       return false;
>> @@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct 
>> i915_gem_context *ctx,
>>           u32 srm, lrm, rsvd;
>>           u32 expect;
>>           int idx;
>> +        bool ro_reg;
>>           if (wo_register(engine, reg))
>>               continue;
>> -        if (ro_register(reg))
>> -            continue;
>> +        ro_reg = ro_register(reg);
>>           srm = MI_STORE_REGISTER_MEM;
>>           lrm = MI_LOAD_REGISTER_MEM;
>> @@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct 
>> i915_gem_context *ctx,
>>           }
>>           GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
>> -        rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
>> -        if (!rsvd) {
>> -            pr_err("%s: Unable to write to whitelisted register %x\n",
>> -                   engine->name, reg);
>> -            err = -EINVAL;
>> -            goto out_unpin;
>> +        if (ro_reg) {
>> +            rsvd = 0xFFFFFFFF;
>> +        } else {
>> +            rsvd = results[ARRAY_SIZE(values)]; /* detect write 
>> masking */
>> +            if (!rsvd) {
>> +                pr_err("%s: Unable to write to whitelisted register 
>> %x\n",
>> +                       engine->name, reg);
>> +                err = -EINVAL;
>> +                goto out_unpin;
>> +            }
>>           }
>>           expect = results[0];
>>           idx = 1;
>>           for (v = 0; v < ARRAY_SIZE(values); v++) {
>> -            expect = reg_write(expect, values[v], rsvd);
>> +            if (ro_reg)
>> +                expect = results[0];
>> +            else
>> +                expect = reg_write(expect, values[v], rsvd);
>> +
>>               if (results[idx] != expect)
>>                   err++;
>>               idx++;
>>           }
>>           for (v = 0; v < ARRAY_SIZE(values); v++) {
>> -            expect = reg_write(expect, ~values[v], rsvd);
>> +            if (ro_reg)
>> +                expect = results[0];
>> +            else
>> +                expect = reg_write(expect, ~values[v], rsvd);
>> +
>>               if (results[idx] != expect)
>>                   err++;
>>               idx++;
>> @@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct 
>> i915_gem_context *ctx,
>>               for (v = 0; v < ARRAY_SIZE(values); v++) {
>>                   u32 w = values[v];
>> -                expect = reg_write(expect, w, rsvd);
>> +                if (ro_reg)
>> +                    expect = results[0];
>> +                else
>> +                    expect = reg_write(expect, w, rsvd);
>>                   pr_info("Wrote %08x, read %08x, expect %08x\n",
>>                       w, results[idx], expect);
>>                   idx++;
>> @@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct 
>> i915_gem_context *ctx,
>>               for (v = 0; v < ARRAY_SIZE(values); v++) {
>>                   u32 w = ~values[v];
>> -                expect = reg_write(expect, w, rsvd);
>> +                if (ro_reg)
>> +                    expect = results[0];
>> +                else
>> +                    expect = reg_write(expect, w, rsvd);
>>                   pr_info("Wrote %08x, read %08x, expect %08x\n",
>>                       w, results[idx], expect);
>>                   idx++;
>> @@ -762,8 +785,8 @@ static int read_whitelisted_registers(struct 
>> i915_gem_context *ctx,
>>           u64 offset = results->node.start + sizeof(u32) * i;
>>           u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> -        /* Clear RD only and WR only flags */
>> -        reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>> +        /* Clear access permission field */
>> +        reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
>>           *cs++ = srm;
>>           *cs++ = reg;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index cc295a4f6e92..ba22e3b8f77e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2513,13 +2513,16 @@ enum i915_power_well_id {
>>   #define   RING_WAIT_SEMAPHORE    (1 << 10) /* gen6+ */
>>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) 
>> * 4)
>> -#define   RING_FORCE_TO_NONPRIV_RW        (0 << 28)    /* CFL+ & 
>> Gen11+ */
>> -#define   RING_FORCE_TO_NONPRIV_RD        (1 << 28)
>> -#define   RING_FORCE_TO_NONPRIV_WR        (2 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RW    (0 << 28)    /* CFL+ & 
>> Gen11+ */
>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RD    (1 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_WR    (2 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_INVALID    (3 << 28)
>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_MASK    (3 << 28)
>>   #define   RING_FORCE_TO_NONPRIV_RANGE_1        (0 << 0)     /* CFL+ 
>> & Gen11+ */
>>   #define   RING_FORCE_TO_NONPRIV_RANGE_4        (1 << 0)
>>   #define   RING_FORCE_TO_NONPRIV_RANGE_16    (2 << 0)
>>   #define   RING_FORCE_TO_NONPRIV_RANGE_64    (3 << 0)
>> +#define   RING_FORCE_TO_NONPRIV_RANGE_MASK    (3 << 0)
>>   #define   RING_MAX_NONPRIV_SLOTS  12
>>   #define GEN7_TLB_RD_ADDR    _MMIO(0x4700)
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Implement read-only support in whitelist selftest

[John Harrison]

Patches sent.

I haven't made any changes to dmesg output as I'm not sure what you mean.

Ah, do you mean the debug print in wa_init_finish()? Sure, I can add the 
engine name to that.

John.


On 6/25/2019 01:33, Tvrtko Ursulin wrote:
>
> Ping.
>
> We agreed to follow up with a test ASAP after merging.
>
> Here's another feature request for you: Add engine->name logging to 
> wa_init_start in intel_engine_init_whitelist. Because with the change 
> to add whitelist on other engines there are now multiple identical 
> lines in dmesg.
>
> To sum up that's three todo items:
>
> 1. Resend the selftest for CI.
> 2. Add GEM_BUG_ON for reg->flags checking invalid flag usage.
> 3. Improve dmesg so we know which engine got how many whitelist entries.
>
> Thanks,
>
> Tvrtko
>
> On 20/06/2019 16:43, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> You will need to send this not as reply to this thread so it is 
>> picked up by CI and then can be merged.
>>
>> But please also add a patch which adds that GEM_BUG_ON reg->flags 
>> check we talked about.
>>
>> Regards,
>>
>> Tvrtko
>>
>> On 18/06/2019 20:54, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> Newer hardware supports extra feature in the whitelist registers. This
>>> patch updates the selftest to test that entries marked as read only
>>> are actually read only.
>>>
>>> Also updated the read/write access definitions to make it clearer that
>>> they are an enum field not a set of single bit flags.
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_workarounds.c   |  8 +--
>>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 53 
>>> +++++++++++++------
>>>   drivers/gpu/drm/i915/i915_reg.h               |  9 ++--
>>>   3 files changed, 48 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c 
>>> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>> index 93caa7b6d7a9..4429ee08b20e 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>> @@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, 
>>> i915_reg_t reg, u32 flags)
>>>   static void
>>>   whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
>>>   {
>>> -    whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
>>> +    whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
>>>   }
>>>   static void gen9_whitelist_build(struct i915_wa_list *w)
>>> @@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine = 
>>> %s, instance = %d, base = 0x%X, reg
>>>           /* hucStatusRegOffset */
>>>           whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
>>> -                  RING_FORCE_TO_NONPRIV_RD);
>>> +                  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>>           /* hucUKernelHdrInfoRegOffset */
>>>           whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
>>> -                  RING_FORCE_TO_NONPRIV_RD);
>>> +                  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>>           /* hucStatus2RegOffset */
>>>           whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
>>> -                  RING_FORCE_TO_NONPRIV_RD);
>>> +                  RING_FORCE_TO_NONPRIV_ACCESS_RD);
>>>           break;
>>>       default:
>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> index eb6d3aa2c8cc..a0a88ec66861 100644
>>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> @@ -399,6 +399,10 @@ static bool wo_register(struct intel_engine_cs 
>>> *engine, u32 reg)
>>>       enum intel_platform platform = 
>>> INTEL_INFO(engine->i915)->platform;
>>>       int i;
>>> +    if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
>>> +         RING_FORCE_TO_NONPRIV_ACCESS_WR)
>>> +        return true;
>>> +
>>>       for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
>>>           if (wo_registers[i].platform == platform &&
>>>               wo_registers[i].reg == reg)
>>> @@ -410,7 +414,8 @@ static bool wo_register(struct intel_engine_cs 
>>> *engine, u32 reg)
>>>   static bool ro_register(u32 reg)
>>>   {
>>> -    if (reg & RING_FORCE_TO_NONPRIV_RD)
>>> +    if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
>>> +         RING_FORCE_TO_NONPRIV_ACCESS_RD)
>>>           return true;
>>>       return false;
>>> @@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct 
>>> i915_gem_context *ctx,
>>>           u32 srm, lrm, rsvd;
>>>           u32 expect;
>>>           int idx;
>>> +        bool ro_reg;
>>>           if (wo_register(engine, reg))
>>>               continue;
>>> -        if (ro_register(reg))
>>> -            continue;
>>> +        ro_reg = ro_register(reg);
>>>           srm = MI_STORE_REGISTER_MEM;
>>>           lrm = MI_LOAD_REGISTER_MEM;
>>> @@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct 
>>> i915_gem_context *ctx,
>>>           }
>>>           GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
>>> -        rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
>>> -        if (!rsvd) {
>>> -            pr_err("%s: Unable to write to whitelisted register %x\n",
>>> -                   engine->name, reg);
>>> -            err = -EINVAL;
>>> -            goto out_unpin;
>>> +        if (ro_reg) {
>>> +            rsvd = 0xFFFFFFFF;
>>> +        } else {
>>> +            rsvd = results[ARRAY_SIZE(values)]; /* detect write 
>>> masking */
>>> +            if (!rsvd) {
>>> +                pr_err("%s: Unable to write to whitelisted register 
>>> %x\n",
>>> +                       engine->name, reg);
>>> +                err = -EINVAL;
>>> +                goto out_unpin;
>>> +            }
>>>           }
>>>           expect = results[0];
>>>           idx = 1;
>>>           for (v = 0; v < ARRAY_SIZE(values); v++) {
>>> -            expect = reg_write(expect, values[v], rsvd);
>>> +            if (ro_reg)
>>> +                expect = results[0];
>>> +            else
>>> +                expect = reg_write(expect, values[v], rsvd);
>>> +
>>>               if (results[idx] != expect)
>>>                   err++;
>>>               idx++;
>>>           }
>>>           for (v = 0; v < ARRAY_SIZE(values); v++) {
>>> -            expect = reg_write(expect, ~values[v], rsvd);
>>> +            if (ro_reg)
>>> +                expect = results[0];
>>> +            else
>>> +                expect = reg_write(expect, ~values[v], rsvd);
>>> +
>>>               if (results[idx] != expect)
>>>                   err++;
>>>               idx++;
>>> @@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct 
>>> i915_gem_context *ctx,
>>>               for (v = 0; v < ARRAY_SIZE(values); v++) {
>>>                   u32 w = values[v];
>>> -                expect = reg_write(expect, w, rsvd);
>>> +                if (ro_reg)
>>> +                    expect = results[0];
>>> +                else
>>> +                    expect = reg_write(expect, w, rsvd);
>>>                   pr_info("Wrote %08x, read %08x, expect %08x\n",
>>>                       w, results[idx], expect);
>>>                   idx++;
>>> @@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct 
>>> i915_gem_context *ctx,
>>>               for (v = 0; v < ARRAY_SIZE(values); v++) {
>>>                   u32 w = ~values[v];
>>> -                expect = reg_write(expect, w, rsvd);
>>> +                if (ro_reg)
>>> +                    expect = results[0];
>>> +                else
>>> +                    expect = reg_write(expect, w, rsvd);
>>>                   pr_info("Wrote %08x, read %08x, expect %08x\n",
>>>                       w, results[idx], expect);
>>>                   idx++;
>>> @@ -762,8 +785,8 @@ static int read_whitelisted_registers(struct 
>>> i915_gem_context *ctx,
>>>           u64 offset = results->node.start + sizeof(u32) * i;
>>>           u32 reg = 
>>> i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>>> -        /* Clear RD only and WR only flags */
>>> -        reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
>>> +        /* Clear access permission field */
>>> +        reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
>>>           *cs++ = srm;
>>>           *cs++ = reg;
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index cc295a4f6e92..ba22e3b8f77e 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -2513,13 +2513,16 @@ enum i915_power_well_id {
>>>   #define   RING_WAIT_SEMAPHORE    (1 << 10) /* gen6+ */
>>>   #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + 
>>> (i) * 4)
>>> -#define   RING_FORCE_TO_NONPRIV_RW        (0 << 28) /* CFL+ & 
>>> Gen11+ */
>>> -#define   RING_FORCE_TO_NONPRIV_RD        (1 << 28)
>>> -#define   RING_FORCE_TO_NONPRIV_WR        (2 << 28)
>>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RW    (0 << 28)    /* CFL+ & 
>>> Gen11+ */
>>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_RD    (1 << 28)
>>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_WR    (2 << 28)
>>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_INVALID    (3 << 28)
>>> +#define   RING_FORCE_TO_NONPRIV_ACCESS_MASK    (3 << 28)
>>>   #define   RING_FORCE_TO_NONPRIV_RANGE_1        (0 << 0)     /* 
>>> CFL+ & Gen11+ */
>>>   #define   RING_FORCE_TO_NONPRIV_RANGE_4        (1 << 0)
>>>   #define   RING_FORCE_TO_NONPRIV_RANGE_16    (2 << 0)
>>>   #define   RING_FORCE_TO_NONPRIV_RANGE_64    (3 << 0)
>>> +#define   RING_FORCE_TO_NONPRIV_RANGE_MASK    (3 << 0)
>>>   #define   RING_MAX_NONPRIV_SLOTS  12
>>>   #define GEN7_TLB_RD_ADDR    _MMIO(0x4700)
>>>

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