[PATCH v3 0/2] drm/i915/perf: fix context filtering with GuC & ICL

[PATCH v3 0/2] drm/i915/perf: fix context filtering with GuC & ICL

[Lionel Landwerlin]

Hi,

Addressing some comments from Chris & Michel.

Thanks for your time,

Lionel Landwerlin (2):
  drm/i915: drop one bit on the hw_id when using guc
  drm/i915/perf: fix ctx_id read with GuC & ICL

 drivers/gpu/drm/i915/i915_drv.h         |   2 +
 drivers/gpu/drm/i915/i915_gem_context.c |  15 ++-
 drivers/gpu/drm/i915/i915_perf.c        | 126 ++++++++++++++++++------
 drivers/gpu/drm/i915/intel_lrc.c        |   7 +-
 4 files changed, 116 insertions(+), 34 deletions(-)

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

[PATCH v3 1/2] drm/i915: drop one bit on the hw_id when using guc

[Lionel Landwerlin]

We currently using GuC as a proxy to the hardware. When Guc is used in
such mode, it consumes the bit 20 of the hw_id to indicate that the
workload was submitted by proxy.

So far we probably haven't seen the issue because we need to allocate
1048576+ contexts to hit this issue. Still, we should avoid allocating
the hw_id on that bit and restrict to bits [0:19] (i.e 20bits instead
of 21).

v2: Leave the max hw_id computation in i915_gem_context.c (Michel)

v3: Be consistent on if/else usage (Chris)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
BSpec: 1237
Reviewed-by: Michel Thierry <michel.thierry@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h         |  1 +
 drivers/gpu/drm/i915/i915_gem_context.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_lrc.c        |  2 +-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 38157df6ff5c..0ae88b671e8d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1841,6 +1841,7 @@ struct drm_i915_private {
 		 */
 		struct ida hw_ida;
 #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
+#define MAX_GUC_CONTEXT_HW_ID (1 << 20) /* exclusive */
 #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
 	} contexts;

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 94e4db1870aa..38c6e9e4e91b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -208,10 +208,19 @@ static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
 	int ret;
 	unsigned int max;

-	if (INTEL_GEN(dev_priv) >= 11)
+	if (INTEL_GEN(dev_priv) >= 11) {
 		max = GEN11_MAX_CONTEXT_HW_ID;
-	else
-		max = MAX_CONTEXT_HW_ID;
+	} else {
+		/*
+		 * When using GuC in proxy submission, GuC consumes the
+		 * highest bit in the context id to indicate proxy submission.
+		 */
+		if (USES_GUC_SUBMISSION(dev_priv))
+			max = MAX_GUC_CONTEXT_HW_ID;
+		else
+			max = MAX_CONTEXT_HW_ID;
+	}
+

 	ret = ida_simple_get(&dev_priv->contexts.hw_ida,
 			     0, max, GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 517e92c6a70b..d09d2b79552f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -200,7 +200,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine,
  *
  *      bits  0-11:    flags, GEN8_CTX_* (cached in ctx->desc_template)
  *      bits 12-31:    LRCA, GTT address of (the HWSP of) this context
- *      bits 32-52:    ctx ID, a globally unique tag
+ *      bits 32-52:    ctx ID, a globally unique tag (highest bit used by GuC)
  *      bits 53-54:    mbz, reserved for use by hardware
  *      bits 55-63:    group ID, currently unused and set to 0
  *
--
2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 2/2] drm/i915/perf: fix ctx_id read with GuC & ICL

[Lionel Landwerlin]

One thing we didn't really understand about the OA report is that the
ContextID field (dword 2) is copy of the context descriptor (dword 1).

On Gen8->10 and without using GuC we didn't notice the issue because
we only checked the 21bits of the ContextID field in the OA reports
which matches exactly the hw_id stored into the context descriptor.

When using GuC submission we have an issue of a non matching hw_id
because GuC uses bit 20 of the hw_id to signal proxy submission. This
change introduces a mask to compare only the relevant bits.

On ICL the context descriptor format has changed and we failed to
address this. On top of using a mask we also need to shift the bits
properly.

v2: Reuse lrc_desc rather than recomputing part of it (Chris/Michel)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: 1de401c08fa805 ("drm/i915/perf: enable perf support on ICL")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104252
BSpec: 1237
Testcase: igt/perf/gen8-unprivileged-single-ctx-counters
---
 drivers/gpu/drm/i915/i915_drv.h  |   1 +
 drivers/gpu/drm/i915/i915_perf.c | 126 +++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_lrc.c |   5 ++
 3 files changed, 102 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0ae88b671e8d..c568160ea53c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1951,6 +1951,7 @@ struct drm_i915_private {
 
 			struct intel_context *pinned_ctx;
 			u32 specific_ctx_id;
+			u32 specific_ctx_id_mask;
 
 			struct hrtimer poll_check_timer;
 			wait_queue_head_t poll_wq;
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 4f0eb84b3c00..48124d206c79 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -737,12 +737,7 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
 			continue;
 		}
 
-		/*
-		 * XXX: Just keep the lower 21 bits for now since I'm not
-		 * entirely sure if the HW touches any of the higher bits in
-		 * this field
-		 */
-		ctx_id = report32[2] & 0x1fffff;
+		ctx_id = report32[2] & dev_priv->perf.oa.specific_ctx_id_mask;
 
 		/*
 		 * Squash whatever is in the CTX_ID field if it's marked as
@@ -1203,6 +1198,33 @@ static int i915_oa_read(struct i915_perf_stream *stream,
 	return dev_priv->perf.oa.ops.read(stream, buf, count, offset);
 }
 
+static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
+					    struct i915_gem_context *ctx)
+{
+	struct intel_engine_cs *engine = i915->engine[RCS];
+	struct intel_context *ce;
+	int ret;
+
+	ret = i915_mutex_lock_interruptible(&i915->drm);
+	if (ret)
+		return ERR_PTR(ret);
+
+	/*
+	 * As the ID is the gtt offset of the context's vma we
+	 * pin the vma to ensure the ID remains fixed.
+	 *
+	 * NB: implied RCS engine...
+	 */
+	ce = intel_context_pin(ctx, engine);
+	mutex_unlock(&i915->drm.struct_mutex);
+	if (IS_ERR(ce))
+		return ce;
+
+	i915->perf.oa.pinned_ctx = ce;
+
+	return ce;
+}
+
 /**
  * oa_get_render_ctx_id - determine and hold ctx hw id
  * @stream: An i915-perf stream opened for OA metrics
@@ -1215,40 +1237,83 @@ static int i915_oa_read(struct i915_perf_stream *stream,
  */
 static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
 {
-	struct drm_i915_private *dev_priv = stream->dev_priv;
+	struct drm_i915_private *i915 = stream->dev_priv;
 
-	if (HAS_LOGICAL_RING_CONTEXTS(dev_priv)) {
-		dev_priv->perf.oa.specific_ctx_id = stream->ctx->hw_id;
-	} else {
-		struct intel_engine_cs *engine = dev_priv->engine[RCS];
+	switch (INTEL_GEN(i915)) {
+	case 7: {
 		struct intel_context *ce;
-		int ret;
-
-		ret = i915_mutex_lock_interruptible(&dev_priv->drm);
-		if (ret)
-			return ret;
 
-		/*
-		 * As the ID is the gtt offset of the context's vma we
-		 * pin the vma to ensure the ID remains fixed.
-		 *
-		 * NB: implied RCS engine...
-		 */
-		ce = intel_context_pin(stream->ctx, engine);
-		mutex_unlock(&dev_priv->drm.struct_mutex);
+		ce = oa_pin_context(i915, stream->ctx);
 		if (IS_ERR(ce))
 			return PTR_ERR(ce);
 
-		dev_priv->perf.oa.pinned_ctx = ce;
-
 		/*
-		 * Explicitly track the ID (instead of calling
-		 * i915_ggtt_offset() on the fly) considering the difference
-		 * with gen8+ and execlists
+		 * On Haswell we don't do any post processing of the reports
+		 * and don't need to use the mask.
 		 */
-		dev_priv->perf.oa.specific_ctx_id = i915_ggtt_offset(ce->state);
+		i915->perf.oa.specific_ctx_id = i915_ggtt_offset(ce->state);
+		i915->perf.oa.specific_ctx_id_mask = 0;
+		break;
 	}
 
+	case 8:
+	case 9:
+	case 10:
+		if (USES_GUC_SUBMISSION(i915)) {
+			struct intel_context *ce;
+
+			ce = oa_pin_context(i915, stream->ctx);
+			if (IS_ERR(ce))
+				return PTR_ERR(ce);
+
+			/*
+			 * When using GuC, the context descriptor we write in
+			 * i915 is read by GuC and rewritten before it's
+			 * actually written into the hardware. The LRCA is
+			 * what is put into the context id field of the
+			 * context descriptor by GuC. Because it's aligned to
+			 * a page, the lower 12bits are always at 0 and
+			 * dropped by GuC. They won't be part of the context
+			 * ID in the OA reports, so squash those lower bits.
+			 */
+			i915->perf.oa.specific_ctx_id =
+				lower_32_bits(ce->lrc_desc) >> 12;
+
+			/*
+			 * GuC uses the top bit to signal proxy submission, so
+			 * ignore that bit.
+			 */
+			i915->perf.oa.specific_ctx_id_mask =
+				(1U << (GEN8_CTX_ID_WIDTH - 1)) - 1;
+		} else {
+			i915->perf.oa.specific_ctx_id = stream->ctx->hw_id;
+			i915->perf.oa.specific_ctx_id_mask =
+				(1U << GEN8_CTX_ID_WIDTH) - 1;
+		}
+		break;
+
+	case 11: {
+		struct intel_engine_cs *engine = i915->engine[RCS];
+
+		i915->perf.oa.specific_ctx_id =
+			stream->ctx->hw_id << (GEN11_SW_CTX_ID_SHIFT - 32) |
+			engine->instance << (GEN11_ENGINE_INSTANCE_SHIFT - 32) |
+			engine->class << (GEN11_ENGINE_INSTANCE_SHIFT - 32);
+		i915->perf.oa.specific_ctx_id_mask =
+			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32) |
+			((1U << GEN11_ENGINE_INSTANCE_WIDTH) - 1) << (GEN11_ENGINE_INSTANCE_SHIFT - 32) |
+			((1 << GEN11_ENGINE_CLASS_WIDTH) - 1) << (GEN11_ENGINE_CLASS_SHIFT - 32);
+		break;
+	}
+
+	default:
+		MISSING_CASE(INTEL_GEN(i915));
+	}
+
+	DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n",
+			 i915->perf.oa.specific_ctx_id,
+			 i915->perf.oa.specific_ctx_id_mask);
+
 	return 0;
 }
 
@@ -1265,6 +1330,7 @@ static void oa_put_render_ctx_id(struct i915_perf_stream *stream)
 	struct intel_context *ce;
 
 	dev_priv->perf.oa.specific_ctx_id = INVALID_CTX_ID;
+	dev_priv->perf.oa.specific_ctx_id_mask = 0;
 
 	ce = fetch_and_zero(&dev_priv->perf.oa.pinned_ctx);
 	if (ce) {
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d09d2b79552f..eb25afa9694f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -233,6 +233,11 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
 								/* bits 12-31 */
 	GEM_BUG_ON(desc & GENMASK_ULL(63, 32));
 
+	/*
+	 * The following 32bits are copied into the OA reports (dword 2).
+	 * Consider updating oa_get_render_ctx_id in i915_perf.c when changing
+	 * anything below.
+	 */
 	if (INTEL_GEN(ctx->i915) >= 11) {
 		GEM_BUG_ON(ctx->hw_id >= BIT(GEN11_SW_CTX_ID_WIDTH));
 		desc |= (u64)ctx->hw_id << GEN11_SW_CTX_ID_SHIFT;
-- 
2.17.1

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915/perf: fix context filtering with GuC & ICL (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/perf: fix context filtering with GuC & ICL (rev3)
URL   : https://patchwork.freedesktop.org/series/44043/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
4bb0abe855ff drm/i915: drop one bit on the hw_id when using guc
9d90eabec73c drm/i915/perf: fix ctx_id read with GuC & ICL
-:185: WARNING:LONG_LINE: line over 100 characters
#185: FILE: drivers/gpu/drm/i915/i915_perf.c:1304:
+			((1U << GEN11_ENGINE_INSTANCE_WIDTH) - 1) << (GEN11_ENGINE_INSTANCE_SHIFT - 32) |

total: 0 errors, 1 warnings, 0 checks, 178 lines checked

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

✗ Fi.CI.SPARSE: warning for drm/i915/perf: fix context filtering with GuC & ICL (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/perf: fix context filtering with GuC & ICL (rev3)
URL   : https://patchwork.freedesktop.org/series/44043/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: drop one bit on the hw_id when using guc
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3665:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3666:16: warning: expression using sizeof(void)

Commit: drm/i915/perf: fix ctx_id read with GuC & ICL
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3666:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3667:16: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for drm/i915/perf: fix context filtering with GuC & ICL (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/perf: fix context filtering with GuC & ICL (rev3)
URL   : https://patchwork.freedesktop.org/series/44043/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4274 -> Patchwork_9175 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44043/revisions/3/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_basic@bad-close:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#105128)

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000)

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         PASS -> FAIL (fdo#104008)

    
    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-cfl-s3:          FAIL (fdo#100368, fdo#103928) -> PASS

    igt@kms_pipe_crc_basic@read-crc-pipe-c-frame-sequence:
      fi-cfl-s3:          FAIL (fdo#103481) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000


== Participating hosts (41 -> 37) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4274 -> Patchwork_9175

  CI_DRM_4274: 37fae8f4dd2fa03d699ab6bf37a673660dafa4a2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4505: 8a8f0271a71e2e0d2a2caa4d41f4ad1d9c89670e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9175: 9d90eabec73c4f15469a623351834c77c0cb513c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

9d90eabec73c drm/i915/perf: fix ctx_id read with GuC & ICL
4bb0abe855ff drm/i915: drop one bit on the hw_id when using guc

== Logs ==

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

✓ Fi.CI.IGT: success for drm/i915/perf: fix context filtering with GuC & ICL (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/perf: fix context filtering with GuC & ICL (rev3)
URL   : https://patchwork.freedesktop.org/series/44043/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4274_full -> Patchwork_9175_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9175_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9175_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44043/revisions/3/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9175_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          PASS -> SKIP +1

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_gtt:
      shard-glk:          PASS -> INCOMPLETE (fdo#103359, k.org#198133)

    igt@gem_eio@hibernate:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      shard-apl:          DMESG-FAIL (fdo#106560) -> PASS

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-glk:          FAIL (fdo#103060) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@perf@blocking:
      shard-hsw:          FAIL (fdo#102252) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4274 -> Patchwork_9175

  CI_DRM_4274: 37fae8f4dd2fa03d699ab6bf37a673660dafa4a2 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4505: 8a8f0271a71e2e0d2a2caa4d41f4ad1d9c89670e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9175: 9d90eabec73c4f15469a623351834c77c0cb513c @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

Re: [PATCH v3 2/2] drm/i915/perf: fix ctx_id read with GuC & ICL

[Chris Wilson]

Quoting Lionel Landwerlin (2018-06-02 01:40:29)
> One thing we didn't really understand about the OA report is that the
> ContextID field (dword 2) is copy of the context descriptor (dword 1).
> 
> On Gen8->10 and without using GuC we didn't notice the issue because
> we only checked the 21bits of the ContextID field in the OA reports
> which matches exactly the hw_id stored into the context descriptor.
> 
> When using GuC submission we have an issue of a non matching hw_id
> because GuC uses bit 20 of the hw_id to signal proxy submission. This
> change introduces a mask to compare only the relevant bits.
> 
> On ICL the context descriptor format has changed and we failed to
> address this. On top of using a mask we also need to shift the bits
> properly.
> 
> v2: Reuse lrc_desc rather than recomputing part of it (Chris/Michel)
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: 1de401c08fa805 ("drm/i915/perf: enable perf support on ICL")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104252
> BSpec: 1237
> Testcase: igt/perf/gen8-unprivileged-single-ctx-counters
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   1 +
>  drivers/gpu/drm/i915/i915_perf.c | 126 +++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_lrc.c |   5 ++
>  3 files changed, 102 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0ae88b671e8d..c568160ea53c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1951,6 +1951,7 @@ struct drm_i915_private {
>  
>                         struct intel_context *pinned_ctx;
>                         u32 specific_ctx_id;
> +                       u32 specific_ctx_id_mask;
>  
>                         struct hrtimer poll_check_timer;
>                         wait_queue_head_t poll_wq;
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 4f0eb84b3c00..48124d206c79 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -737,12 +737,7 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
>                         continue;
>                 }
>  
> -               /*
> -                * XXX: Just keep the lower 21 bits for now since I'm not
> -                * entirely sure if the HW touches any of the higher bits in
> -                * this field
> -                */
> -               ctx_id = report32[2] & 0x1fffff;
> +               ctx_id = report32[2] & dev_priv->perf.oa.specific_ctx_id_mask;
>  
>                 /*
>                  * Squash whatever is in the CTX_ID field if it's marked as
> @@ -1203,6 +1198,33 @@ static int i915_oa_read(struct i915_perf_stream *stream,
>         return dev_priv->perf.oa.ops.read(stream, buf, count, offset);
>  }
>  
> +static struct intel_context *oa_pin_context(struct drm_i915_private *i915,
> +                                           struct i915_gem_context *ctx)
> +{
> +       struct intel_engine_cs *engine = i915->engine[RCS];
> +       struct intel_context *ce;
> +       int ret;
> +
> +       ret = i915_mutex_lock_interruptible(&i915->drm);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       /*
> +        * As the ID is the gtt offset of the context's vma we
> +        * pin the vma to ensure the ID remains fixed.
> +        *
> +        * NB: implied RCS engine...
> +        */
> +       ce = intel_context_pin(ctx, engine);
> +       mutex_unlock(&i915->drm.struct_mutex);
> +       if (IS_ERR(ce))
> +               return ce;
> +
> +       i915->perf.oa.pinned_ctx = ce;
> +
> +       return ce;
> +}
> +
>  /**
>   * oa_get_render_ctx_id - determine and hold ctx hw id
>   * @stream: An i915-perf stream opened for OA metrics
> @@ -1215,40 +1237,83 @@ static int i915_oa_read(struct i915_perf_stream *stream,
>   */
>  static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
>  {
> -       struct drm_i915_private *dev_priv = stream->dev_priv;
> +       struct drm_i915_private *i915 = stream->dev_priv;
>  
> -       if (HAS_LOGICAL_RING_CONTEXTS(dev_priv)) {
> -               dev_priv->perf.oa.specific_ctx_id = stream->ctx->hw_id;
> -       } else {
> -               struct intel_engine_cs *engine = dev_priv->engine[RCS];
> +       switch (INTEL_GEN(i915)) {
> +       case 7: {
>                 struct intel_context *ce;
> -               int ret;
> -
> -               ret = i915_mutex_lock_interruptible(&dev_priv->drm);
> -               if (ret)
> -                       return ret;
>  
> -               /*
> -                * As the ID is the gtt offset of the context's vma we
> -                * pin the vma to ensure the ID remains fixed.
> -                *
> -                * NB: implied RCS engine...
> -                */
> -               ce = intel_context_pin(stream->ctx, engine);
> -               mutex_unlock(&dev_priv->drm.struct_mutex);
> +               ce = oa_pin_context(i915, stream->ctx);
>                 if (IS_ERR(ce))
>                         return PTR_ERR(ce);
>  
> -               dev_priv->perf.oa.pinned_ctx = ce;
> -
>                 /*
> -                * Explicitly track the ID (instead of calling
> -                * i915_ggtt_offset() on the fly) considering the difference
> -                * with gen8+ and execlists
> +                * On Haswell we don't do any post processing of the reports
> +                * and don't need to use the mask.
>                  */
> -               dev_priv->perf.oa.specific_ctx_id = i915_ggtt_offset(ce->state);
> +               i915->perf.oa.specific_ctx_id = i915_ggtt_offset(ce->state);
> +               i915->perf.oa.specific_ctx_id_mask = 0;
> +               break;
>         }
>  
> +       case 8:
> +       case 9:
> +       case 10:
> +               if (USES_GUC_SUBMISSION(i915)) {
> +                       struct intel_context *ce;
> +
> +                       ce = oa_pin_context(i915, stream->ctx);
> +                       if (IS_ERR(ce))
> +                               return PTR_ERR(ce);
> +
> +                       /*
> +                        * When using GuC, the context descriptor we write in
> +                        * i915 is read by GuC and rewritten before it's
> +                        * actually written into the hardware. The LRCA is
> +                        * what is put into the context id field of the
> +                        * context descriptor by GuC. Because it's aligned to
> +                        * a page, the lower 12bits are always at 0 and
> +                        * dropped by GuC. They won't be part of the context
> +                        * ID in the OA reports, so squash those lower bits.
> +                        */
> +                       i915->perf.oa.specific_ctx_id =
> +                               lower_32_bits(ce->lrc_desc) >> 12;
> +
> +                       /*
> +                        * GuC uses the top bit to signal proxy submission, so
> +                        * ignore that bit.
> +                        */
> +                       i915->perf.oa.specific_ctx_id_mask =
> +                               (1U << (GEN8_CTX_ID_WIDTH - 1)) - 1;
> +               } else {
> +                       i915->perf.oa.specific_ctx_id = stream->ctx->hw_id;

Plan for the hw_id not being valid unless pinned. Just the pin the
context everywhere and use ce for deriving the specific_ctx_id on all
paths.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx