[Intel-gfx] [PATCH 1/2] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Intel-gfx] [PATCH 1/2] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Chris Wilson]

Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v5.5+
---
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
 drivers/gpu/drm/i915/gt/intel_lrc.c          | 23 ++++++++++++++------
 drivers/gpu/drm/i915/i915_perf.c             |  3 +--
 drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index bf395227c99f..a9fc3fbbe482 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -304,8 +304,7 @@ struct intel_engine_cs {
 	u32 context_size;
 	u32 mmio_base;
 
-	unsigned int context_tag;
-#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
+	unsigned long context_tag;
 
 	struct rb_node uabi_node;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 93a1b73ad96b..d68a04f2a9d5 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1404,13 +1404,16 @@ __execlists_schedule_in(struct i915_request *rq)
 	ce->lrc_desc &= ~GENMASK_ULL(47, 37);
 	if (ce->tag) {
 		/* Use a fixed tag for OA and friends */
+		GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
 		ce->lrc_desc |= (u64)ce->tag << 32;
 	} else {
 		/* We don't need a strict matching tag, just different values */
-		ce->lrc_desc |=
-			(u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
-			GEN11_SW_CTX_ID_SHIFT;
-		BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
+		unsigned int tag = ffs(engine->context_tag);
+
+		clear_bit(tag - 1, &engine->context_tag);
+		ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
+
+		BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
 	}
 
 	__intel_gt_pm_get(engine->gt);
@@ -1452,7 +1455,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
 
 static inline void
 __execlists_schedule_out(struct i915_request *rq,
-			 struct intel_engine_cs * const engine)
+			 struct intel_engine_cs * const engine,
+			 int tag)
 {
 	struct intel_context * const ce = rq->context;
 
@@ -1470,6 +1474,9 @@ __execlists_schedule_out(struct i915_request *rq,
 	    i915_request_completed(rq))
 		intel_engine_add_retire(engine, ce->timeline);
 
+	if (tag <= BITS_PER_TYPE(engine->context_tag))
+		set_bit(tag - 1, &engine->context_tag);
+
 	intel_context_update_runtime(ce);
 	intel_engine_context_out(engine);
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
@@ -1495,15 +1502,17 @@ execlists_schedule_out(struct i915_request *rq)
 {
 	struct intel_context * const ce = rq->context;
 	struct intel_engine_cs *cur, *old;
+	int tag;
 
 	trace_i915_request_out(rq);
 
+	tag = upper_32_bits(rq->context->lrc_desc);
 	old = READ_ONCE(ce->inflight);
 	do
 		cur = ptr_unmask_bits(old, 2) ? ptr_dec(old) : NULL;
 	while (!try_cmpxchg(&ce->inflight, &old, cur));
 	if (!cur)
-		__execlists_schedule_out(rq, old);
+		__execlists_schedule_out(rq, old, tag);
 
 	i915_request_put(rq);
 }
@@ -4002,7 +4011,7 @@ static void enable_execlists(struct intel_engine_cs *engine)
 
 	enable_error_interrupt(engine);
 
-	engine->context_tag = 0;
+	engine->context_tag = -1u;
 }
 
 static bool unexpected_starting_state(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index dec1b33e4da8..1863a5c4778d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1281,11 +1281,10 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
 			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
 		/*
 		 * Pick an unused context id
-		 * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts
+		 * 0 - BITS_PER_LONG 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);
 		break;
 	}
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
index 58b5f40a07dd..af89c7fc8f59 100644
--- a/drivers/gpu/drm/i915/selftests/i915_vma.c
+++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
@@ -173,7 +173,7 @@ static int igt_vma_create(void *arg)
 		}
 
 		nc = 0;
-		for_each_prime_number(num_ctx, 2 * NUM_CONTEXT_TAG) {
+		for_each_prime_number(num_ctx, 2 * BITS_PER_LONG) {
 			for (; nc < num_ctx; nc++) {
 				ctx = mock_context(i915, "mock");
 				if (!ctx)
-- 
2.20.1

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

[PATCH 1/2] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Chris Wilson]

Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v5.5+
---
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
 drivers/gpu/drm/i915/gt/intel_lrc.c          | 23 ++++++++++++++------
 drivers/gpu/drm/i915/i915_perf.c             |  3 +--
 drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
 4 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index bf395227c99f..a9fc3fbbe482 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -304,8 +304,7 @@ struct intel_engine_cs {
 	u32 context_size;
 	u32 mmio_base;
 
-	unsigned int context_tag;
-#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
+	unsigned long context_tag;
 
 	struct rb_node uabi_node;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 93a1b73ad96b..d68a04f2a9d5 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1404,13 +1404,16 @@ __execlists_schedule_in(struct i915_request *rq)
 	ce->lrc_desc &= ~GENMASK_ULL(47, 37);
 	if (ce->tag) {
 		/* Use a fixed tag for OA and friends */
+		GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
 		ce->lrc_desc |= (u64)ce->tag << 32;
 	} else {
 		/* We don't need a strict matching tag, just different values */
-		ce->lrc_desc |=
-			(u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
-			GEN11_SW_CTX_ID_SHIFT;
-		BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
+		unsigned int tag = ffs(engine->context_tag);
+
+		clear_bit(tag - 1, &engine->context_tag);
+		ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
+
+		BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
 	}
 
 	__intel_gt_pm_get(engine->gt);
@@ -1452,7 +1455,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
 
 static inline void
 __execlists_schedule_out(struct i915_request *rq,
-			 struct intel_engine_cs * const engine)
+			 struct intel_engine_cs * const engine,
+			 int tag)
 {
 	struct intel_context * const ce = rq->context;
 
@@ -1470,6 +1474,9 @@ __execlists_schedule_out(struct i915_request *rq,
 	    i915_request_completed(rq))
 		intel_engine_add_retire(engine, ce->timeline);
 
+	if (tag <= BITS_PER_TYPE(engine->context_tag))
+		set_bit(tag - 1, &engine->context_tag);
+
 	intel_context_update_runtime(ce);
 	intel_engine_context_out(engine);
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
@@ -1495,15 +1502,17 @@ execlists_schedule_out(struct i915_request *rq)
 {
 	struct intel_context * const ce = rq->context;
 	struct intel_engine_cs *cur, *old;
+	int tag;
 
 	trace_i915_request_out(rq);
 
+	tag = upper_32_bits(rq->context->lrc_desc);
 	old = READ_ONCE(ce->inflight);
 	do
 		cur = ptr_unmask_bits(old, 2) ? ptr_dec(old) : NULL;
 	while (!try_cmpxchg(&ce->inflight, &old, cur));
 	if (!cur)
-		__execlists_schedule_out(rq, old);
+		__execlists_schedule_out(rq, old, tag);
 
 	i915_request_put(rq);
 }
@@ -4002,7 +4011,7 @@ static void enable_execlists(struct intel_engine_cs *engine)
 
 	enable_error_interrupt(engine);
 
-	engine->context_tag = 0;
+	engine->context_tag = -1u;
 }
 
 static bool unexpected_starting_state(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index dec1b33e4da8..1863a5c4778d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1281,11 +1281,10 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
 			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
 		/*
 		 * Pick an unused context id
-		 * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts
+		 * 0 - BITS_PER_LONG 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);
 		break;
 	}
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
index 58b5f40a07dd..af89c7fc8f59 100644
--- a/drivers/gpu/drm/i915/selftests/i915_vma.c
+++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
@@ -173,7 +173,7 @@ static int igt_vma_create(void *arg)
 		}
 
 		nc = 0;
-		for_each_prime_number(num_ctx, 2 * NUM_CONTEXT_TAG) {
+		for_each_prime_number(num_ctx, 2 * BITS_PER_LONG) {
 			for (; nc < num_ctx; nc++) {
 				ctx = mock_context(i915, "mock");
 				if (!ctx)
-- 
2.20.1

[Intel-gfx] [PATCH 2/2] drm/i915/execlists: Verify we don't submit two identical CCIDs

[Chris Wilson]

Check that we do not submit two contexts into ELSP with the same CCID
[upper portion of the descriptor].

References: https://gitlab.freedesktop.org/drm/intel/-/issues/1793
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index d68a04f2a9d5..f8a8cd72f227 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1621,6 +1621,7 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 	struct i915_request * const *port, *rq;
 	struct intel_context *ce = NULL;
 	bool sentinel = false;
+	u32 ccid = -1;
 
 	trace_ports(execlists, msg, execlists->pending);
 
@@ -1654,6 +1655,14 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		}
 		ce = rq->context;
 
+		if (ccid == upper_32_bits(ce->lrc_desc)) {
+			GEM_TRACE_ERR("Dup ccid:%x context:%llx in pending[%zd]\n",
+				      ccid, ce->timeline->fence_context,
+				      port - execlists->pending);
+			return false;
+		}
+		ccid = upper_32_bits(ce->lrc_desc);
+
 		/*
 		 * Sentinels are supposed to be lonely so they flush the
 		 * current exection off the HW. Check that they are the
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH 1/2] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v5.5+
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
>  drivers/gpu/drm/i915/gt/intel_lrc.c          | 23 ++++++++++++++------
>  drivers/gpu/drm/i915/i915_perf.c             |  3 +--
>  drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
>  4 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index bf395227c99f..a9fc3fbbe482 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -304,8 +304,7 @@ struct intel_engine_cs {
>  	u32 context_size;
>  	u32 mmio_base;
>  
> -	unsigned int context_tag;
> -#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
> +	unsigned long context_tag;
>  
>  	struct rb_node uabi_node;
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 93a1b73ad96b..d68a04f2a9d5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1404,13 +1404,16 @@ __execlists_schedule_in(struct i915_request *rq)
>  	ce->lrc_desc &= ~GENMASK_ULL(47, 37);
>  	if (ce->tag) {
>  		/* Use a fixed tag for OA and friends */
> +		GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
>  		ce->lrc_desc |= (u64)ce->tag << 32;

I see danger here to completely trash the upper part our our lrc_desc.
Is the ce->tag validated or should we add more enforcement in here?

>  	} else {
>  		/* We don't need a strict matching tag, just different values */
> -		ce->lrc_desc |=
> -			(u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
> -			GEN11_SW_CTX_ID_SHIFT;
> -		BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
> +		unsigned int tag = ffs(engine->context_tag);
> +
> +		clear_bit(tag - 1, &engine->context_tag);
> +		ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
> +
> +		BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
>  	}
>  
>  	__intel_gt_pm_get(engine->gt);
> @@ -1452,7 +1455,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
>  
>  static inline void
>  __execlists_schedule_out(struct i915_request *rq,
> -			 struct intel_engine_cs * const engine)
> +			 struct intel_engine_cs * const engine,
> +			 int tag)
>  {
>  	struct intel_context * const ce = rq->context;
>  
> @@ -1470,6 +1474,9 @@ __execlists_schedule_out(struct i915_request *rq,
>  	    i915_request_completed(rq))
>  		intel_engine_add_retire(engine, ce->timeline);
>  
> +	if (tag <= BITS_PER_TYPE(engine->context_tag))
> +		set_bit(tag - 1, &engine->context_tag);
> +
>  	intel_context_update_runtime(ce);
>  	intel_engine_context_out(engine);
>  	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
> @@ -1495,15 +1502,17 @@ execlists_schedule_out(struct i915_request *rq)
>  {
>  	struct intel_context * const ce = rq->context;
>  	struct intel_engine_cs *cur, *old;
> +	int tag;
>  
>  	trace_i915_request_out(rq);
>  
> +	tag = upper_32_bits(rq->context->lrc_desc);

There is more in the upper part than just a tag (sw field).
So we need to only set/get a particular masked field.

-Mika

>  	old = READ_ONCE(ce->inflight);
>  	do
>  		cur = ptr_unmask_bits(old, 2) ? ptr_dec(old) : NULL;
>  	while (!try_cmpxchg(&ce->inflight, &old, cur));
>  	if (!cur)
> -		__execlists_schedule_out(rq, old);
> +		__execlists_schedule_out(rq, old, tag);
>  
>  	i915_request_put(rq);
>  }
> @@ -4002,7 +4011,7 @@ static void enable_execlists(struct intel_engine_cs *engine)
>  
>  	enable_error_interrupt(engine);
>  
> -	engine->context_tag = 0;
> +	engine->context_tag = -1u;
>  }
>  
>  static bool unexpected_starting_state(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index dec1b33e4da8..1863a5c4778d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1281,11 +1281,10 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
>  			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
>  		/*
>  		 * Pick an unused context id
> -		 * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts
> +		 * 0 - BITS_PER_LONG 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);
>  		break;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
> index 58b5f40a07dd..af89c7fc8f59 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
> @@ -173,7 +173,7 @@ static int igt_vma_create(void *arg)
>  		}
>  
>  		nc = 0;
> -		for_each_prime_number(num_ctx, 2 * NUM_CONTEXT_TAG) {
> +		for_each_prime_number(num_ctx, 2 * BITS_PER_LONG) {
>  			for (; nc < num_ctx; nc++) {
>  				ctx = mock_context(i915, "mock");
>  				if (!ctx)
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v5.5+
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
>  drivers/gpu/drm/i915/gt/intel_lrc.c          | 23 ++++++++++++++------
>  drivers/gpu/drm/i915/i915_perf.c             |  3 +--
>  drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
>  4 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index bf395227c99f..a9fc3fbbe482 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -304,8 +304,7 @@ struct intel_engine_cs {
>  	u32 context_size;
>  	u32 mmio_base;
>  
> -	unsigned int context_tag;
> -#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
> +	unsigned long context_tag;
>  
>  	struct rb_node uabi_node;
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 93a1b73ad96b..d68a04f2a9d5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1404,13 +1404,16 @@ __execlists_schedule_in(struct i915_request *rq)
>  	ce->lrc_desc &= ~GENMASK_ULL(47, 37);
>  	if (ce->tag) {
>  		/* Use a fixed tag for OA and friends */
> +		GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
>  		ce->lrc_desc |= (u64)ce->tag << 32;

I see danger here to completely trash the upper part our our lrc_desc.
Is the ce->tag validated or should we add more enforcement in here?

>  	} else {
>  		/* We don't need a strict matching tag, just different values */
> -		ce->lrc_desc |=
> -			(u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
> -			GEN11_SW_CTX_ID_SHIFT;
> -		BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
> +		unsigned int tag = ffs(engine->context_tag);
> +
> +		clear_bit(tag - 1, &engine->context_tag);
> +		ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
> +
> +		BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
>  	}
>  
>  	__intel_gt_pm_get(engine->gt);
> @@ -1452,7 +1455,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
>  
>  static inline void
>  __execlists_schedule_out(struct i915_request *rq,
> -			 struct intel_engine_cs * const engine)
> +			 struct intel_engine_cs * const engine,
> +			 int tag)
>  {
>  	struct intel_context * const ce = rq->context;
>  
> @@ -1470,6 +1474,9 @@ __execlists_schedule_out(struct i915_request *rq,
>  	    i915_request_completed(rq))
>  		intel_engine_add_retire(engine, ce->timeline);
>  
> +	if (tag <= BITS_PER_TYPE(engine->context_tag))
> +		set_bit(tag - 1, &engine->context_tag);
> +
>  	intel_context_update_runtime(ce);
>  	intel_engine_context_out(engine);
>  	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
> @@ -1495,15 +1502,17 @@ execlists_schedule_out(struct i915_request *rq)
>  {
>  	struct intel_context * const ce = rq->context;
>  	struct intel_engine_cs *cur, *old;
> +	int tag;
>  
>  	trace_i915_request_out(rq);
>  
> +	tag = upper_32_bits(rq->context->lrc_desc);

There is more in the upper part than just a tag (sw field).
So we need to only set/get a particular masked field.

-Mika

>  	old = READ_ONCE(ce->inflight);
>  	do
>  		cur = ptr_unmask_bits(old, 2) ? ptr_dec(old) : NULL;
>  	while (!try_cmpxchg(&ce->inflight, &old, cur));
>  	if (!cur)
> -		__execlists_schedule_out(rq, old);
> +		__execlists_schedule_out(rq, old, tag);
>  
>  	i915_request_put(rq);
>  }
> @@ -4002,7 +4011,7 @@ static void enable_execlists(struct intel_engine_cs *engine)
>  
>  	enable_error_interrupt(engine);
>  
> -	engine->context_tag = 0;
> +	engine->context_tag = -1u;
>  }
>  
>  static bool unexpected_starting_state(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index dec1b33e4da8..1863a5c4778d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1281,11 +1281,10 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
>  			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
>  		/*
>  		 * Pick an unused context id
> -		 * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts
> +		 * 0 - BITS_PER_LONG 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);
>  		break;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
> index 58b5f40a07dd..af89c7fc8f59 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
> @@ -173,7 +173,7 @@ static int igt_vma_create(void *arg)
>  		}
>  
>  		nc = 0;
> -		for_each_prime_number(num_ctx, 2 * NUM_CONTEXT_TAG) {
> +		for_each_prime_number(num_ctx, 2 * BITS_PER_LONG) {
>  			for (; nc < num_ctx; nc++) {
>  				ctx = mock_context(i915, "mock");
>  				if (!ctx)
> -- 
> 2.20.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/execlists: Verify we don't submit two identical CCIDs

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Check that we do not submit two contexts into ELSP with the same CCID
> [upper portion of the descriptor].
>
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/1793
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index d68a04f2a9d5..f8a8cd72f227 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1621,6 +1621,7 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
>  	struct i915_request * const *port, *rq;
>  	struct intel_context *ce = NULL;
>  	bool sentinel = false;
> +	u32 ccid = -1;
>  
>  	trace_ports(execlists, msg, execlists->pending);
>  
> @@ -1654,6 +1655,14 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
>  		}
>  		ce = rq->context;
>  
> +		if (ccid == upper_32_bits(ce->lrc_desc)) {
> +			GEM_TRACE_ERR("Dup ccid:%x context:%llx in pending[%zd]\n",
> +				      ccid, ce->timeline->fence_context,
> +				      port - execlists->pending);

The trace was lost, atleast from me, on the previous logs I looked
and thus the value. trace buffer overflowed? But if it
was reader error, then perhaps putting this explicitly in dmesg
is not necessary.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +			return false;
> +		}
> +		ccid = upper_32_bits(ce->lrc_desc);
> +
>  		/*
>  		 * Sentinels are supposed to be lonely so they flush the
>  		 * current exection off the HW. Check that they are the
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Chris Wilson]

Quoting Mika Kuoppala (2020-04-27 18:28:12)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v5.5+
> > ---
> >  drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
> >  drivers/gpu/drm/i915/gt/intel_lrc.c          | 23 ++++++++++++++------
> >  drivers/gpu/drm/i915/i915_perf.c             |  3 +--
> >  drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
> >  4 files changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index bf395227c99f..a9fc3fbbe482 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -304,8 +304,7 @@ struct intel_engine_cs {
> >       u32 context_size;
> >       u32 mmio_base;
> >  
> > -     unsigned int context_tag;
> > -#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
> > +     unsigned long context_tag;
> >  
> >       struct rb_node uabi_node;
> >  
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 93a1b73ad96b..d68a04f2a9d5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1404,13 +1404,16 @@ __execlists_schedule_in(struct i915_request *rq)
> >       ce->lrc_desc &= ~GENMASK_ULL(47, 37);
> >       if (ce->tag) {
> >               /* Use a fixed tag for OA and friends */
> > +             GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
> >               ce->lrc_desc |= (u64)ce->tag << 32;
> 
> I see danger here to completely trash the upper part our our lrc_desc.
> Is the ce->tag validated or should we add more enforcement in here?

It's a single special case atm.

It's a problem for tomorrow, but it'll probably mean a small ida if we
have multiple users who must dictate the CCID. Pita.

> 
> >       } else {
> >               /* We don't need a strict matching tag, just different values */
> > -             ce->lrc_desc |=
> > -                     (u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
> > -                     GEN11_SW_CTX_ID_SHIFT;
> > -             BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
> > +             unsigned int tag = ffs(engine->context_tag);
> > +
> > +             clear_bit(tag - 1, &engine->context_tag);
> > +             ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
> > +
> > +             BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
> >       }
> >  
> >       __intel_gt_pm_get(engine->gt);
> > @@ -1452,7 +1455,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
> >  
> >  static inline void
> >  __execlists_schedule_out(struct i915_request *rq,
> > -                      struct intel_engine_cs * const engine)
> > +                      struct intel_engine_cs * const engine,
> > +                      int tag)
> >  {
> >       struct intel_context * const ce = rq->context;
> >  
> > @@ -1470,6 +1474,9 @@ __execlists_schedule_out(struct i915_request *rq,
> >           i915_request_completed(rq))
> >               intel_engine_add_retire(engine, ce->timeline);
> >  
> > +     if (tag <= BITS_PER_TYPE(engine->context_tag))
> > +             set_bit(tag - 1, &engine->context_tag);
> > +
> >       intel_context_update_runtime(ce);
> >       intel_engine_context_out(engine);
> >       execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
> > @@ -1495,15 +1502,17 @@ execlists_schedule_out(struct i915_request *rq)
> >  {
> >       struct intel_context * const ce = rq->context;
> >       struct intel_engine_cs *cur, *old;
> > +     int tag;
> >  
> >       trace_i915_request_out(rq);
> >  
> > +     tag = upper_32_bits(rq->context->lrc_desc);
> 
> There is more in the upper part than just a tag (sw field).
> So we need to only set/get a particular masked field.

We control the contents, though. Oops, but what I did forget was the
shift.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Chris Wilson]

Quoting Mika Kuoppala (2020-04-27 18:28:12)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v5.5+
> > ---
> >  drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
> >  drivers/gpu/drm/i915/gt/intel_lrc.c          | 23 ++++++++++++++------
> >  drivers/gpu/drm/i915/i915_perf.c             |  3 +--
> >  drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
> >  4 files changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > index bf395227c99f..a9fc3fbbe482 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > @@ -304,8 +304,7 @@ struct intel_engine_cs {
> >       u32 context_size;
> >       u32 mmio_base;
> >  
> > -     unsigned int context_tag;
> > -#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
> > +     unsigned long context_tag;
> >  
> >       struct rb_node uabi_node;
> >  
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 93a1b73ad96b..d68a04f2a9d5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1404,13 +1404,16 @@ __execlists_schedule_in(struct i915_request *rq)
> >       ce->lrc_desc &= ~GENMASK_ULL(47, 37);
> >       if (ce->tag) {
> >               /* Use a fixed tag for OA and friends */
> > +             GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
> >               ce->lrc_desc |= (u64)ce->tag << 32;
> 
> I see danger here to completely trash the upper part our our lrc_desc.
> Is the ce->tag validated or should we add more enforcement in here?

It's a single special case atm.

It's a problem for tomorrow, but it'll probably mean a small ida if we
have multiple users who must dictate the CCID. Pita.

> 
> >       } else {
> >               /* We don't need a strict matching tag, just different values */
> > -             ce->lrc_desc |=
> > -                     (u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
> > -                     GEN11_SW_CTX_ID_SHIFT;
> > -             BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
> > +             unsigned int tag = ffs(engine->context_tag);
> > +
> > +             clear_bit(tag - 1, &engine->context_tag);
> > +             ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
> > +
> > +             BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
> >       }
> >  
> >       __intel_gt_pm_get(engine->gt);
> > @@ -1452,7 +1455,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
> >  
> >  static inline void
> >  __execlists_schedule_out(struct i915_request *rq,
> > -                      struct intel_engine_cs * const engine)
> > +                      struct intel_engine_cs * const engine,
> > +                      int tag)
> >  {
> >       struct intel_context * const ce = rq->context;
> >  
> > @@ -1470,6 +1474,9 @@ __execlists_schedule_out(struct i915_request *rq,
> >           i915_request_completed(rq))
> >               intel_engine_add_retire(engine, ce->timeline);
> >  
> > +     if (tag <= BITS_PER_TYPE(engine->context_tag))
> > +             set_bit(tag - 1, &engine->context_tag);
> > +
> >       intel_context_update_runtime(ce);
> >       intel_engine_context_out(engine);
> >       execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
> > @@ -1495,15 +1502,17 @@ execlists_schedule_out(struct i915_request *rq)
> >  {
> >       struct intel_context * const ce = rq->context;
> >       struct intel_engine_cs *cur, *old;
> > +     int tag;
> >  
> >       trace_i915_request_out(rq);
> >  
> > +     tag = upper_32_bits(rq->context->lrc_desc);
> 
> There is more in the upper part than just a tag (sw field).
> So we need to only set/get a particular masked field.

We control the contents, though. Oops, but what I did forget was the
shift.
-Chris

[Intel-gfx] [PATCH] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Chris Wilson]

Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v5.5+
---
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
 drivers/gpu/drm/i915/gt/intel_lrc.c          | 25 ++++++++++++++------
 drivers/gpu/drm/i915/i915_perf.c             |  3 +--
 drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index bf395227c99f..a9fc3fbbe482 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -304,8 +304,7 @@ struct intel_engine_cs {
 	u32 context_size;
 	u32 mmio_base;
 
-	unsigned int context_tag;
-#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
+	unsigned long context_tag;
 
 	struct rb_node uabi_node;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 93a1b73ad96b..3a9d3ebf3e5b 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1404,13 +1404,17 @@ __execlists_schedule_in(struct i915_request *rq)
 	ce->lrc_desc &= ~GENMASK_ULL(47, 37);
 	if (ce->tag) {
 		/* Use a fixed tag for OA and friends */
+		GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
 		ce->lrc_desc |= (u64)ce->tag << 32;
 	} else {
 		/* We don't need a strict matching tag, just different values */
-		ce->lrc_desc |=
-			(u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
-			GEN11_SW_CTX_ID_SHIFT;
-		BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
+		unsigned int tag = ffs(engine->context_tag);
+
+		GEM_BUG_ON(tag >= BITS_PER_LONG);
+		clear_bit(tag - 1, &engine->context_tag);
+		ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
+
+		BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
 	}
 
 	__intel_gt_pm_get(engine->gt);
@@ -1452,7 +1456,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
 
 static inline void
 __execlists_schedule_out(struct i915_request *rq,
-			 struct intel_engine_cs * const engine)
+			 struct intel_engine_cs * const engine,
+			 unsigned int ccid)
 {
 	struct intel_context * const ce = rq->context;
 
@@ -1470,6 +1475,10 @@ __execlists_schedule_out(struct i915_request *rq,
 	    i915_request_completed(rq))
 		intel_engine_add_retire(engine, ce->timeline);
 
+	ccid >>= (GEN11_SW_CTX_ID_SHIFT - 32);
+	if (ccid < BITS_PER_TYPE(engine->context_tag))
+		set_bit(ccid - 1, &engine->context_tag);
+
 	intel_context_update_runtime(ce);
 	intel_engine_context_out(engine);
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
@@ -1495,15 +1504,17 @@ execlists_schedule_out(struct i915_request *rq)
 {
 	struct intel_context * const ce = rq->context;
 	struct intel_engine_cs *cur, *old;
+	unsigned int ccid;
 
 	trace_i915_request_out(rq);
 
+	ccid = upper_32_bits(rq->context->lrc_desc);
 	old = READ_ONCE(ce->inflight);
 	do
 		cur = ptr_unmask_bits(old, 2) ? ptr_dec(old) : NULL;
 	while (!try_cmpxchg(&ce->inflight, &old, cur));
 	if (!cur)
-		__execlists_schedule_out(rq, old);
+		__execlists_schedule_out(rq, old, ccid);
 
 	i915_request_put(rq);
 }
@@ -4002,7 +4013,7 @@ static void enable_execlists(struct intel_engine_cs *engine)
 
 	enable_error_interrupt(engine);
 
-	engine->context_tag = 0;
+	engine->context_tag = GENMASK(BITS_PER_LONG - 2, 0);
 }
 
 static bool unexpected_starting_state(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index dec1b33e4da8..1863a5c4778d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1281,11 +1281,10 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
 			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
 		/*
 		 * Pick an unused context id
-		 * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts
+		 * 0 - BITS_PER_LONG 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);
 		break;
 	}
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
index 58b5f40a07dd..af89c7fc8f59 100644
--- a/drivers/gpu/drm/i915/selftests/i915_vma.c
+++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
@@ -173,7 +173,7 @@ static int igt_vma_create(void *arg)
 		}
 
 		nc = 0;
-		for_each_prime_number(num_ctx, 2 * NUM_CONTEXT_TAG) {
+		for_each_prime_number(num_ctx, 2 * BITS_PER_LONG) {
 			for (; nc < num_ctx; nc++) {
 				ctx = mock_context(i915, "mock");
 				if (!ctx)
-- 
2.20.1

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

[PATCH] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Chris Wilson]

Fixes: 2935ed5339c4 ("drm/i915: Remove logical HW ID")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v5.5+
---
 drivers/gpu/drm/i915/gt/intel_engine_types.h |  3 +--
 drivers/gpu/drm/i915/gt/intel_lrc.c          | 25 ++++++++++++++------
 drivers/gpu/drm/i915/i915_perf.c             |  3 +--
 drivers/gpu/drm/i915/selftests/i915_vma.c    |  2 +-
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index bf395227c99f..a9fc3fbbe482 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -304,8 +304,7 @@ struct intel_engine_cs {
 	u32 context_size;
 	u32 mmio_base;
 
-	unsigned int context_tag;
-#define NUM_CONTEXT_TAG roundup_pow_of_two(2 * EXECLIST_MAX_PORTS)
+	unsigned long context_tag;
 
 	struct rb_node uabi_node;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 93a1b73ad96b..3a9d3ebf3e5b 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1404,13 +1404,17 @@ __execlists_schedule_in(struct i915_request *rq)
 	ce->lrc_desc &= ~GENMASK_ULL(47, 37);
 	if (ce->tag) {
 		/* Use a fixed tag for OA and friends */
+		GEM_BUG_ON(ce->tag <= BITS_PER_TYPE(engine->context_tag));
 		ce->lrc_desc |= (u64)ce->tag << 32;
 	} else {
 		/* We don't need a strict matching tag, just different values */
-		ce->lrc_desc |=
-			(u64)(++engine->context_tag % NUM_CONTEXT_TAG) <<
-			GEN11_SW_CTX_ID_SHIFT;
-		BUILD_BUG_ON(NUM_CONTEXT_TAG > GEN12_MAX_CONTEXT_HW_ID);
+		unsigned int tag = ffs(engine->context_tag);
+
+		GEM_BUG_ON(tag >= BITS_PER_LONG);
+		clear_bit(tag - 1, &engine->context_tag);
+		ce->lrc_desc |= (u64)tag << GEN11_SW_CTX_ID_SHIFT;
+
+		BUILD_BUG_ON(BITS_PER_TYPE(engine->context_tag) > GEN12_MAX_CONTEXT_HW_ID);
 	}
 
 	__intel_gt_pm_get(engine->gt);
@@ -1452,7 +1456,8 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
 
 static inline void
 __execlists_schedule_out(struct i915_request *rq,
-			 struct intel_engine_cs * const engine)
+			 struct intel_engine_cs * const engine,
+			 unsigned int ccid)
 {
 	struct intel_context * const ce = rq->context;
 
@@ -1470,6 +1475,10 @@ __execlists_schedule_out(struct i915_request *rq,
 	    i915_request_completed(rq))
 		intel_engine_add_retire(engine, ce->timeline);
 
+	ccid >>= (GEN11_SW_CTX_ID_SHIFT - 32);
+	if (ccid < BITS_PER_TYPE(engine->context_tag))
+		set_bit(ccid - 1, &engine->context_tag);
+
 	intel_context_update_runtime(ce);
 	intel_engine_context_out(engine);
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
@@ -1495,15 +1504,17 @@ execlists_schedule_out(struct i915_request *rq)
 {
 	struct intel_context * const ce = rq->context;
 	struct intel_engine_cs *cur, *old;
+	unsigned int ccid;
 
 	trace_i915_request_out(rq);
 
+	ccid = upper_32_bits(rq->context->lrc_desc);
 	old = READ_ONCE(ce->inflight);
 	do
 		cur = ptr_unmask_bits(old, 2) ? ptr_dec(old) : NULL;
 	while (!try_cmpxchg(&ce->inflight, &old, cur));
 	if (!cur)
-		__execlists_schedule_out(rq, old);
+		__execlists_schedule_out(rq, old, ccid);
 
 	i915_request_put(rq);
 }
@@ -4002,7 +4013,7 @@ static void enable_execlists(struct intel_engine_cs *engine)
 
 	enable_error_interrupt(engine);
 
-	engine->context_tag = 0;
+	engine->context_tag = GENMASK(BITS_PER_LONG - 2, 0);
 }
 
 static bool unexpected_starting_state(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index dec1b33e4da8..1863a5c4778d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1281,11 +1281,10 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
 			((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32);
 		/*
 		 * Pick an unused context id
-		 * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts
+		 * 0 - BITS_PER_LONG 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);
 		break;
 	}
 
diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
index 58b5f40a07dd..af89c7fc8f59 100644
--- a/drivers/gpu/drm/i915/selftests/i915_vma.c
+++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
@@ -173,7 +173,7 @@ static int igt_vma_create(void *arg)
 		}
 
 		nc = 0;
-		for_each_prime_number(num_ctx, 2 * NUM_CONTEXT_TAG) {
+		for_each_prime_number(num_ctx, 2 * BITS_PER_LONG) {
 			for (; nc < num_ctx; nc++) {
 				ctx = mock_context(i915, "mock");
 				if (!ctx)
-- 
2.20.1

Re: [Intel-gfx] [PATCH 2/2] drm/i915/execlists: Verify we don't submit two identical CCIDs

[Chris Wilson]

Quoting Mika Kuoppala (2020-04-27 18:31:37)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Check that we do not submit two contexts into ELSP with the same CCID
> > [upper portion of the descriptor].
> >
> > References: https://gitlab.freedesktop.org/drm/intel/-/issues/1793
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/gt/intel_lrc.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index d68a04f2a9d5..f8a8cd72f227 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -1621,6 +1621,7 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
> >       struct i915_request * const *port, *rq;
> >       struct intel_context *ce = NULL;
> >       bool sentinel = false;
> > +     u32 ccid = -1;
> >  
> >       trace_ports(execlists, msg, execlists->pending);
> >  
> > @@ -1654,6 +1655,14 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
> >               }
> >               ce = rq->context;
> >  
> > +             if (ccid == upper_32_bits(ce->lrc_desc)) {
> > +                     GEM_TRACE_ERR("Dup ccid:%x context:%llx in pending[%zd]\n",
> > +                                   ccid, ce->timeline->fence_context,
> > +                                   port - execlists->pending);
> 
> The trace was lost, atleast from me, on the previous logs I looked
> and thus the value. trace buffer overflowed? But if it
> was reader error, then perhaps putting this explicitly in dmesg
> is not necessary.

The trick is to look at the pstores. Or to reproduce it locally where
you can remotely capture the full trace.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with drm/i915/execlists: Avoid reusing the same logical CC_ID (rev2)

[Patchwork]

== Series Details ==

Series: series starting with drm/i915/execlists: Avoid reusing the same logical CC_ID (rev2)
URL   : https://patchwork.freedesktop.org/series/76563/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8376 -> Patchwork_17484
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/index.html

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_close_race@basic-process:
    - fi-icl-y:           [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-icl-y/igt@gem_close_race@basic-process.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-icl-y/igt@gem_close_race@basic-process.html

  * igt@gem_close_race@basic-threads:
    - fi-icl-u2:          [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-icl-u2/igt@gem_close_race@basic-threads.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-icl-u2/igt@gem_close_race@basic-threads.html
    - fi-icl-guc:         [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-icl-guc/igt@gem_close_race@basic-threads.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-icl-guc/igt@gem_close_race@basic-threads.html

  * igt@gem_exec_gttfill@basic:
    - fi-kbl-x1275:       [PASS][7] -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-kbl-x1275/igt@gem_exec_gttfill@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-kbl-x1275/igt@gem_exec_gttfill@basic.html
    - fi-tgl-y:           [PASS][9] -> [INCOMPLETE][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-tgl-y/igt@gem_exec_gttfill@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-tgl-y/igt@gem_exec_gttfill@basic.html
    - fi-cfl-8109u:       [PASS][11] -> [INCOMPLETE][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-cfl-8109u/igt@gem_exec_gttfill@basic.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-cfl-8109u/igt@gem_exec_gttfill@basic.html
    - fi-bdw-5557u:       [PASS][13] -> [INCOMPLETE][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-bdw-5557u/igt@gem_exec_gttfill@basic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-bdw-5557u/igt@gem_exec_gttfill@basic.html
    - fi-skl-guc:         [PASS][15] -> [INCOMPLETE][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-skl-guc/igt@gem_exec_gttfill@basic.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-skl-guc/igt@gem_exec_gttfill@basic.html
    - fi-kbl-7500u:       [PASS][17] -> [INCOMPLETE][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-kbl-7500u/igt@gem_exec_gttfill@basic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-kbl-7500u/igt@gem_exec_gttfill@basic.html
    - fi-kbl-guc:         [PASS][19] -> [INCOMPLETE][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-kbl-guc/igt@gem_exec_gttfill@basic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-kbl-guc/igt@gem_exec_gttfill@basic.html
    - fi-cfl-8700k:       [PASS][21] -> [INCOMPLETE][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-cfl-8700k/igt@gem_exec_gttfill@basic.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-cfl-8700k/igt@gem_exec_gttfill@basic.html
    - fi-whl-u:           [PASS][23] -> [INCOMPLETE][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-whl-u/igt@gem_exec_gttfill@basic.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-whl-u/igt@gem_exec_gttfill@basic.html
    - fi-skl-6600u:       [PASS][25] -> [INCOMPLETE][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-skl-6600u/igt@gem_exec_gttfill@basic.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-skl-6600u/igt@gem_exec_gttfill@basic.html
    - fi-skl-lmem:        [PASS][27] -> [INCOMPLETE][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-skl-lmem/igt@gem_exec_gttfill@basic.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-skl-lmem/igt@gem_exec_gttfill@basic.html
    - fi-cfl-guc:         [PASS][29] -> [INCOMPLETE][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-cfl-guc/igt@gem_exec_gttfill@basic.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-cfl-guc/igt@gem_exec_gttfill@basic.html
    - fi-skl-6700k2:      [PASS][31] -> [INCOMPLETE][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-skl-6700k2/igt@gem_exec_gttfill@basic.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-skl-6700k2/igt@gem_exec_gttfill@basic.html

  * igt@gem_sync@basic-each:
    - fi-bsw-nick:        [PASS][33] -> [INCOMPLETE][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-bsw-nick/igt@gem_sync@basic-each.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-bsw-nick/igt@gem_sync@basic-each.html
    - fi-kbl-soraka:      [PASS][35] -> [INCOMPLETE][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-kbl-soraka/igt@gem_sync@basic-each.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-kbl-soraka/igt@gem_sync@basic-each.html
    - fi-apl-guc:         [PASS][37] -> [INCOMPLETE][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-apl-guc/igt@gem_sync@basic-each.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-apl-guc/igt@gem_sync@basic-each.html
    - fi-bxt-dsi:         [PASS][39] -> [INCOMPLETE][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-bxt-dsi/igt@gem_sync@basic-each.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-bxt-dsi/igt@gem_sync@basic-each.html

  * igt@i915_selftest@live@gt_contexts:
    - fi-bsw-kefka:       [PASS][41] -> [INCOMPLETE][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-bsw-kefka/igt@i915_selftest@live@gt_contexts.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-bsw-kefka/igt@i915_selftest@live@gt_contexts.html
    - fi-cml-s:           [PASS][43] -> [INCOMPLETE][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-cml-s/igt@i915_selftest@live@gt_contexts.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-cml-s/igt@i915_selftest@live@gt_contexts.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_close_race@basic-process:
    - {fi-ehl-1}:         [PASS][45] -> [INCOMPLETE][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-ehl-1/igt@gem_close_race@basic-process.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-ehl-1/igt@gem_close_race@basic-process.html

  * igt@gem_close_race@basic-threads:
    - {fi-tgl-u}:         [PASS][47] -> [INCOMPLETE][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-tgl-u/igt@gem_close_race@basic-threads.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-tgl-u/igt@gem_close_race@basic-threads.html

  * {igt@gem_exec_parallel@engines@contexts}:
    - {fi-tgl-dsi}:       [PASS][49] -> [INCOMPLETE][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-tgl-dsi/igt@gem_exec_parallel@engines@contexts.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-tgl-dsi/igt@gem_exec_parallel@engines@contexts.html

  * igt@gem_sync@basic-each:
    - {fi-kbl-7560u}:     NOTRUN -> [INCOMPLETE][51]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-kbl-7560u/igt@gem_sync@basic-each.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@blt:
    - fi-bsw-n3050:       [PASS][52] -> [INCOMPLETE][53] ([i915#392])
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-bsw-n3050/igt@i915_selftest@live@blt.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-bsw-n3050/igt@i915_selftest@live@blt.html

  * igt@i915_selftest@live@gem_contexts:
    - fi-glk-dsi:         [PASS][54] -> [INCOMPLETE][55] ([i915#1591] / [i915#58] / [k.org#198133])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-glk-dsi/igt@i915_selftest@live@gem_contexts.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-glk-dsi/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@hangcheck:
    - fi-kbl-8809g:       [PASS][56] -> [INCOMPLETE][57] ([fdo#108744])
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8376/fi-kbl-8809g/igt@i915_selftest@live@hangcheck.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17484/fi-kbl-8809g/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [i915#1591]: https://gitlab.freedesktop.org/drm/intel/issues/1591
  [i915#392]: https://gitlab.freedesktop.org/drm/intel/issues/392
  [i915#58]: https://gitlab.freedesktop.org/drm/intel/issues/58
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (49 -> 44)
------------------------------

  Missing    (5): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8376 -> Patchwork_17484

  CI-20190529: 20190529
  CI_DRM_8376: 3c32812d682625cce30878fa3139b58a014304c4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5612: c8dc1fd926a550308b971ca7d83fe0a927a38152 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17484: f8ef49c73a6ed9cf7d1e70b8de63700c0cdd4496 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f8ef49c73a6e drm/i915/execlists: Verify we don't submit two identical CCIDs
7285bba3d1a5 drm/i915/execlists: Avoid reusing the same logical CC_ID

== Logs ==

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

[Intel-gfx] [PATCH 2/2] drm/i915/execlists: Verify we don't submit two identical CCIDs

[Chris Wilson]

Check that we do not submit two contexts into ELSP with the same CCID
[upper portion of the descriptor].

References: https://gitlab.freedesktop.org/drm/intel/-/issues/1793
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 37 ++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index e84d277d1f02..8fffbeaef8b0 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1621,9 +1621,12 @@ static __maybe_unused bool
 assert_pending_valid(const struct intel_engine_execlists *execlists,
 		     const char *msg)
 {
+	struct intel_engine_cs *engine =
+		container_of(execlists, typeof(*engine), execlists);
 	struct i915_request * const *port, *rq;
 	struct intel_context *ce = NULL;
 	bool sentinel = false;
+	u32 ccid = -1;
 
 	trace_ports(execlists, msg, execlists->pending);
 
@@ -1632,13 +1635,14 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		return true;
 
 	if (!execlists->pending[0]) {
-		GEM_TRACE_ERR("Nothing pending for promotion!\n");
+		GEM_TRACE_ERR("%s: Nothing pending for promotion!\n",
+			      engine->name);
 		return false;
 	}
 
 	if (execlists->pending[execlists_num_ports(execlists)]) {
-		GEM_TRACE_ERR("Excess pending[%d] for promotion!\n",
-			      execlists_num_ports(execlists));
+		GEM_TRACE_ERR("%s: Excess pending[%d] for promotion!\n",
+			      engine->name, execlists_num_ports(execlists));
 		return false;
 	}
 
@@ -1650,20 +1654,31 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		GEM_BUG_ON(!i915_request_is_active(rq));
 
 		if (ce == rq->context) {
-			GEM_TRACE_ERR("Dup context:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Dup context:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			return false;
 		}
 		ce = rq->context;
 
+		if (ccid == upper_32_bits(ce->lrc_desc)) {
+			GEM_TRACE_ERR("%s: Dup ccid:%x context:%llx in pending[%zd]\n",
+				      engine->name,
+				      ccid, ce->timeline->fence_context,
+				      port - execlists->pending);
+			return false;
+		}
+		ccid = upper_32_bits(ce->lrc_desc);
+
 		/*
 		 * Sentinels are supposed to be lonely so they flush the
 		 * current exection off the HW. Check that they are the
 		 * only request in the pending submission.
 		 */
 		if (sentinel) {
-			GEM_TRACE_ERR("context:%llx after sentinel in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: context:%llx after sentinel in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			return false;
@@ -1671,7 +1686,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 
 		sentinel = i915_request_has_sentinel(rq);
 		if (sentinel && port != execlists->pending) {
-			GEM_TRACE_ERR("sentinel context:%llx not in prime position[%zd]\n",
+			GEM_TRACE_ERR("%s: sentinel context:%llx not in prime position[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			return false;
@@ -1686,7 +1702,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 
 		if (i915_active_is_idle(&ce->active) &&
 		    !intel_context_is_barrier(ce)) {
-			GEM_TRACE_ERR("Inactive context:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Inactive context:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			ok = false;
@@ -1694,7 +1711,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		}
 
 		if (!i915_vma_is_pinned(ce->state)) {
-			GEM_TRACE_ERR("Unpinned context:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Unpinned context:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			ok = false;
@@ -1702,7 +1720,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		}
 
 		if (!i915_vma_is_pinned(ce->ring->vma)) {
-			GEM_TRACE_ERR("Unpinned ring:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Unpinned ring:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			ok = false;
-- 
2.20.1

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

[Intel-gfx] [PATCH 2/2] drm/i915/execlists: Verify we don't submit two identical CCIDs

[Chris Wilson]

Check that we do not submit two contexts into ELSP with the same CCID
[upper portion of the descriptor].

References: https://gitlab.freedesktop.org/drm/intel/-/issues/1793
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 37 ++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 7d56207276d5..a69809e7d1d8 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1597,9 +1597,12 @@ static __maybe_unused bool
 assert_pending_valid(const struct intel_engine_execlists *execlists,
 		     const char *msg)
 {
+	struct intel_engine_cs *engine =
+		container_of(execlists, typeof(*engine), execlists);
 	struct i915_request * const *port, *rq;
 	struct intel_context *ce = NULL;
 	bool sentinel = false;
+	u32 ccid = -1;
 
 	trace_ports(execlists, msg, execlists->pending);
 
@@ -1608,13 +1611,14 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		return true;
 
 	if (!execlists->pending[0]) {
-		GEM_TRACE_ERR("Nothing pending for promotion!\n");
+		GEM_TRACE_ERR("%s: Nothing pending for promotion!\n",
+			      engine->name);
 		return false;
 	}
 
 	if (execlists->pending[execlists_num_ports(execlists)]) {
-		GEM_TRACE_ERR("Excess pending[%d] for promotion!\n",
-			      execlists_num_ports(execlists));
+		GEM_TRACE_ERR("%s: Excess pending[%d] for promotion!\n",
+			      engine->name, execlists_num_ports(execlists));
 		return false;
 	}
 
@@ -1626,20 +1630,31 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		GEM_BUG_ON(!i915_request_is_active(rq));
 
 		if (ce == rq->context) {
-			GEM_TRACE_ERR("Dup context:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Dup context:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			return false;
 		}
 		ce = rq->context;
 
+		if (ccid == ce->lrc.ccid) {
+			GEM_TRACE_ERR("%s: Dup ccid:%x context:%llx in pending[%zd]\n",
+				      engine->name,
+				      ccid, ce->timeline->fence_context,
+				      port - execlists->pending);
+			return false;
+		}
+		ccid = ce->lrc.ccid;
+
 		/*
 		 * Sentinels are supposed to be lonely so they flush the
 		 * current exection off the HW. Check that they are the
 		 * only request in the pending submission.
 		 */
 		if (sentinel) {
-			GEM_TRACE_ERR("context:%llx after sentinel in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: context:%llx after sentinel in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			return false;
@@ -1647,7 +1662,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 
 		sentinel = i915_request_has_sentinel(rq);
 		if (sentinel && port != execlists->pending) {
-			GEM_TRACE_ERR("sentinel context:%llx not in prime position[%zd]\n",
+			GEM_TRACE_ERR("%s: sentinel context:%llx not in prime position[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			return false;
@@ -1662,7 +1678,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 
 		if (i915_active_is_idle(&ce->active) &&
 		    !intel_context_is_barrier(ce)) {
-			GEM_TRACE_ERR("Inactive context:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Inactive context:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			ok = false;
@@ -1670,7 +1687,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		}
 
 		if (!i915_vma_is_pinned(ce->state)) {
-			GEM_TRACE_ERR("Unpinned context:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Unpinned context:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			ok = false;
@@ -1678,7 +1696,8 @@ assert_pending_valid(const struct intel_engine_execlists *execlists,
 		}
 
 		if (!i915_vma_is_pinned(ce->ring->vma)) {
-			GEM_TRACE_ERR("Unpinned ring:%llx in pending[%zd]\n",
+			GEM_TRACE_ERR("%s: Unpinned ring:%llx in pending[%zd]\n",
+				      engine->name,
 				      ce->timeline->fence_context,
 				      port - execlists->pending);
 			ok = false;
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 2935ed5339c4 ("drm/i915: Remove logical HW ID").

The bot has tested the following trees: v5.6.7.

v5.6.7: Failed to apply! Possible dependencies:
    03d0ed8a8e93 ("drm/i915: Skip capturing errors from internal contexts")
    1883a0a4658e ("drm/i915: Track hw reported context runtime")
    6f280b133dc2 ("drm/i915/perf: Fix OA context id overlap with idle context id")
    70a76a9b8e9d ("drm/i915/gt: Hook up CS_MASTER_ERROR_INTERRUPT")
    ff3d4ff6c9e6 ("drm/i915/gt: Tidy repetition in declaring gen8+ interrupts")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

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

Re: [PATCH] drm/i915/execlists: Avoid reusing the same logical CC_ID

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 2935ed5339c4 ("drm/i915: Remove logical HW ID").

The bot has tested the following trees: v5.6.7.

v5.6.7: Failed to apply! Possible dependencies:
    03d0ed8a8e93 ("drm/i915: Skip capturing errors from internal contexts")
    1883a0a4658e ("drm/i915: Track hw reported context runtime")
    6f280b133dc2 ("drm/i915/perf: Fix OA context id overlap with idle context id")
    70a76a9b8e9d ("drm/i915/gt: Hook up CS_MASTER_ERROR_INTERRUPT")
    ff3d4ff6c9e6 ("drm/i915/gt: Tidy repetition in declaring gen8+ interrupts")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha