[Intel-gfx] [PATCH] drm/i915/perf: Fix OA context id overlap with idle context id

[Umesh Nerlige Ramappa]

On Sat, Jan 25, 2020 at 03:37:38AM +0200, Lionel Landwerlin wrote:
>On 24/01/2020 03:37, Umesh Nerlige Ramappa wrote:
>>Engine context pinned in perf OA was set to same context id as
>>the idle context. Set the context id to an unused value.
>>
>>Clear the sw context id field in lrc descriptor before ORing with
>>ce->tag (Chris)
>>
>>Fixes: https://gitlab.freedesktop.org/drm/intel/issues/756
>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>>---
>>  drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +-
>>  drivers/gpu/drm/i915/i915_perf.c    | 9 +++++++--
>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>index a13a8c4b65ab..cf6c43bd540a 100644
>>--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
>>+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
>>@@ -1211,12 +1211,12 @@ __execlists_schedule_in(struct i915_request *rq)
>>  	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
>>  		execlists_check_context(ce, engine);
>>+	ce->lrc_desc &= ~GENMASK_ULL(47, 37);
>>  	if (ce->tag) {
>>  		/* Use a fixed tag for OA and friends */
>>  		ce->lrc_desc |= (u64)ce->tag << 32;
>>  	} else {
>>  		/* We don't need a strict matching tag, just different values */
>>-		ce->lrc_desc &= ~GENMASK_ULL(47, 37);
>I guess you can remove the line just above.

Not sure what you mean, the line is moved from here to above the if.

>>  		ce->lrc_desc |=
>>  			(u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
>>  			GEN11_SW_CTX_ID_SHIFT;
>>diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>index 3c4647054557..5bd878c64504 100644
>>--- a/drivers/gpu/drm/i915/i915_perf.c
>>+++ b/drivers/gpu/drm/i915/i915_perf.c
>>@@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
>>  	case 12: {
>>  		stream->specific_ctx_id_mask =
>>  			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
>>-		stream->specific_ctx_id = stream->specific_ctx_id_mask;
>>+		/* Pick an unused context id
>>+		 * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts
>>+		 * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context
>>+		 */
>>+		stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
>>+		BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < NUM_CONTEXT_TAG);
>
>
>Arg yeah, we can't use an id that has all bits to 1 because that 
>matches the idle value in the OA reports :/
>
>This also affects gen8-10 cases (afaik).

For gen8-10, I did not see a specific definition for an idle context id.  
The from/to idle context switches are indicated by dedicated bits in the 
CSB instead (from spec).

Thanks,
Umesh

>
>
>Thanks for spotting this!
>
>
>-Lionel
>
>
>>  		break;
>>  	}
>>@@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
>>  		MISSING_CASE(INTEL_GEN(ce->engine->i915));
>>  	}
>>-	ce->tag = stream->specific_ctx_id_mask;
>>+	ce->tag = stream->specific_ctx_id;
>>  	DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n",
>>  			 stream->specific_ctx_id,
>
>