[igt-dev] [PATCH i-g-t 0/2] i915/gem_ctx_isolation: __for_each_physical_engine + engine mapping

[igt-dev] [PATCH i-g-t 0/2] i915/gem_ctx_isolation: __for_each_physical_engine + engine mapping

[Dale B Stimson]

i915/gem_ctx_isolation: __for_each_physical_engine and gem_engine_topology

Use __for_each_physical_engine with gem_engine_topology.

Iterates through the definitive list of all engines
present.  All contexts have their engine mapping set via
gem_context_set_all_engines().

Patch 1 was originally posted by Ramalingam C <ramalingam.c@intel.com>.
Since then, slight modifications have been done to it due to
upstream changes.  Patch 1 is being kept separate from patch 2 in
order to assure proper attribution to the author.

Dale B Stimson (1):
  DBS: tests/i915/gem_ctx_isolation: use the gem_engine_topology
    library, part 2

Ramalingam C (1):
  tests/i915/gem_ctx_isolation: use the gem_engine_topology library,
    part 1

 tests/i915/gem_ctx_isolation.c | 66 ++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 23 deletions(-)

-- 
2.25.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 1/2] tests/i915/gem_ctx_isolation: use the gem_engine_topology library, part 1

[Dale B Stimson]

From: Ramalingam C <ramalingam.c@intel.com>

Call function gem_class_can_store_dword instead of legacy function
gem_can_store_dword.  This requires that e->class be available in the
calling function.

Instead of passing "engine" (== "e->flags") to functions, pass "e".
This makes e->class available where it is needed.

This commit is being kept separate from "part 2" in order to assure
proper attribution to the author.  The code associated with this
commit was written by Ramalingam C <ramalingam.c@intel.com>.
Since then, slight modifications have been done due to upstream
changes.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>.
Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com>
---
 tests/i915/gem_ctx_isolation.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index 6aa27133c..25113b054 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -575,7 +575,6 @@ static void nonpriv(int fd,
 		0x0505c0c0,
 		0xdeadbeef
 	};
-	unsigned int engine = e->flags;
 	unsigned int num_values = ARRAY_SIZE(values);
 
 	/* Sigh -- hsw: we need cmdparser access to our own registers! */
@@ -593,7 +592,7 @@ static void nonpriv(int fd,
 
 		tmpl_regs(fd, ctx, e, tmpl, values[v]);
 
-		spin = igt_spin_new(fd, .ctx = ctx, .engine = engine);
+		spin = igt_spin_new(fd, .ctx = ctx, .engine = e->flags);
 
 		igt_debug("%s[%d]: Setting all registers to 0x%08x\n",
 			  __func__, v, values[v]);
@@ -606,12 +605,12 @@ static void nonpriv(int fd,
 			/* Explicit sync to keep the switch between write/read */
 			syncpt = igt_spin_new(fd,
 					      .ctx = ctx,
-					      .engine = engine,
+					      .engine = e->flags,
 					      .flags = IGT_SPIN_FENCE_OUT);
 
 			dirt = igt_spin_new(fd,
 					    .ctx = sw,
-					    .engine = engine,
+					    .engine = e->flags,
 					    .fence = syncpt->out_fence,
 					    .flags = (IGT_SPIN_FENCE_IN |
 						      IGT_SPIN_FENCE_OUT));
@@ -619,7 +618,7 @@ static void nonpriv(int fd,
 
 			syncpt = igt_spin_new(fd,
 					      .ctx = ctx,
-					      .engine = engine,
+					      .engine = e->flags,
 					      .fence = dirt->out_fence,
 					      .flags = IGT_SPIN_FENCE_IN);
 			igt_spin_free(fd, dirt);
@@ -660,7 +659,6 @@ static void isolation(int fd,
 		0xaaaaaaaa,
 		0xdeadbeef
 	};
-	unsigned int engine = e->flags;
 	unsigned int num_values =
 		flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
 
@@ -673,7 +671,7 @@ static void isolation(int fd,
 		ctx[0] = gem_context_create(fd);
 		regs[0] = read_regs(fd, ctx[0], e, flags);
 
-		spin = igt_spin_new(fd, .ctx = ctx[0], .engine = engine);
+		spin = igt_spin_new(fd, .ctx = ctx[0], .engine = e->flags);
 
 		if (flags & DIRTY1) {
 			igt_debug("%s[%d]: Setting all registers of ctx 0 to 0x%08x\n",
@@ -726,11 +724,11 @@ static void isolation(int fd,
 #define S4 (4 << 8)
 #define SLEEP_MASK (0xf << 8)
 
-static void inject_reset_context(int fd, unsigned int engine)
+static void inject_reset_context(int fd, const struct intel_execution_engine2 *e)
 {
 	struct igt_spin_factory opts = {
 		.ctx = gem_context_create(fd),
-		.engine = engine,
+		.engine = e->flags,
 		.flags = IGT_SPIN_FAST,
 	};
 	igt_spin_t *spin;
@@ -741,7 +739,7 @@ static void inject_reset_context(int fd, unsigned int engine)
 	 * HW for screwing up if the context was already broken.
 	 */
 
-	if (gem_can_store_dword(fd, engine))
+	if (gem_class_can_store_dword(fd, e->class))
 		opts.flags |= IGT_SPIN_POLL_RUN;
 
 	spin = __igt_spin_factory(fd, &opts);
@@ -771,7 +769,6 @@ static void preservation(int fd,
 		0xdeadbeef
 	};
 	const unsigned int num_values = ARRAY_SIZE(values);
-	unsigned int engine = e->flags;
 	uint32_t ctx[num_values +1 ];
 	uint32_t regs[num_values + 1][2];
 	igt_spin_t *spin;
@@ -779,7 +776,7 @@ static void preservation(int fd,
 	gem_quiescent_gpu(fd);
 
 	ctx[num_values] = gem_context_create(fd);
-	spin = igt_spin_new(fd, .ctx = ctx[num_values], .engine = engine);
+	spin = igt_spin_new(fd, .ctx = ctx[num_values], .engine = e->flags);
 	regs[num_values][0] = read_regs(fd, ctx[num_values], e, flags);
 	for (int v = 0; v < num_values; v++) {
 		ctx[v] = gem_context_create(fd);
@@ -792,7 +789,7 @@ static void preservation(int fd,
 	igt_spin_free(fd, spin);
 
 	if (flags & RESET)
-		inject_reset_context(fd, engine);
+		inject_reset_context(fd, e);
 
 	switch (flags & SLEEP_MASK) {
 	case NOSLEEP:
@@ -819,7 +816,7 @@ static void preservation(int fd,
 		break;
 	}
 
-	spin = igt_spin_new(fd, .ctx = ctx[num_values], .engine = engine);
+	spin = igt_spin_new(fd, .ctx = ctx[num_values], .engine = e->flags);
 	for (int v = 0; v < num_values; v++)
 		regs[v][1] = read_regs(fd, ctx[v], e, flags);
 	regs[num_values][1] = read_regs(fd, ctx[num_values], e, flags);
-- 
2.25.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 2/2] DBS: tests/i915/gem_ctx_isolation: use the gem_engine_topology library, part 2

[Dale B Stimson]

Switch from simple iteration over all potential engines to using
macro __for_each_physical_engine which only returns engines that are
actually present.

For each context (as it is created) call gem_context_set_all_engines
so that execbuf will interpret the engine specification in the new way.

Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com>
---
 tests/i915/gem_ctx_isolation.c | 41 ++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index 25113b054..31a20ed3a 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -240,6 +240,25 @@ static bool ignore_register(uint32_t offset)
 	return false;
 }
 
+/*
+ * context_create_plus_all_engines - Same as gem_context_create plus setup.
+ *
+ * This is a convenience function that may be called instead of the sequence
+ * of gem_context_create followed by gem_context_set_all_engines.
+ * If gem_has_engine_topology(), then function gem_context_set_all_engines
+ * indicates that future execbuf calls for this context should interpret the
+ * engine specification in a gem_engine_topology-compatible way.
+ */
+static uint32_t context_create_plus_all_engines(int fd)
+{
+	uint32_t ctx;
+
+	ctx = gem_context_create(fd);
+	gem_context_set_all_engines(fd, ctx);
+
+	return ctx;
+}
+
 static void tmpl_regs(int fd,
 		      uint32_t ctx,
 		      const struct intel_execution_engine2 *e,
@@ -586,7 +605,8 @@ static void nonpriv(int fd,
 		igt_spin_t *spin = NULL;
 		uint32_t ctx, regs[2], tmpl;
 
-		ctx = gem_context_create(fd);
+		ctx = context_create_plus_all_engines(fd);
+
 		tmpl = read_regs(fd, ctx, e, flags);
 		regs[0] = read_regs(fd, ctx, e, flags);
 
@@ -599,7 +619,7 @@ static void nonpriv(int fd,
 		write_regs(fd, ctx, e, flags, values[v]);
 
 		if (flags & DIRTY2) {
-			uint32_t sw = gem_context_create(fd);
+			uint32_t sw = context_create_plus_all_engines(fd);
 			igt_spin_t *syncpt, *dirt;
 
 			/* Explicit sync to keep the switch between write/read */
@@ -668,7 +688,7 @@ static void isolation(int fd,
 		igt_spin_t *spin = NULL;
 		uint32_t ctx[2], regs[2], tmp;
 
-		ctx[0] = gem_context_create(fd);
+		ctx[0] = context_create_plus_all_engines(fd);
 		regs[0] = read_regs(fd, ctx[0], e, flags);
 
 		spin = igt_spin_new(fd, .ctx = ctx[0], .engine = e->flags);
@@ -687,7 +707,7 @@ static void isolation(int fd,
 		 * the default values from this context, but if goes badly we
 		 * see the corruption from the previous context instead!
 		 */
-		ctx[1] = gem_context_create(fd);
+		ctx[1] = context_create_plus_all_engines(fd);
 		regs[1] = read_regs(fd, ctx[1], e, flags);
 
 		if (flags & DIRTY2) {
@@ -727,7 +747,7 @@ static void isolation(int fd,
 static void inject_reset_context(int fd, const struct intel_execution_engine2 *e)
 {
 	struct igt_spin_factory opts = {
-		.ctx = gem_context_create(fd),
+		.ctx = context_create_plus_all_engines(fd),
 		.engine = e->flags,
 		.flags = IGT_SPIN_FAST,
 	};
@@ -775,11 +795,11 @@ static void preservation(int fd,
 
 	gem_quiescent_gpu(fd);
 
-	ctx[num_values] = gem_context_create(fd);
+	ctx[num_values] = context_create_plus_all_engines(fd);
 	spin = igt_spin_new(fd, .ctx = ctx[num_values], .engine = e->flags);
 	regs[num_values][0] = read_regs(fd, ctx[num_values], e, flags);
 	for (int v = 0; v < num_values; v++) {
-		ctx[v] = gem_context_create(fd);
+		ctx[v] = context_create_plus_all_engines(fd);
 		write_regs(fd, ctx[v], e, flags, values[v]);
 
 		regs[v][0] = read_regs(fd, ctx[v], e, flags);
@@ -854,6 +874,7 @@ static unsigned int __has_context_isolation(int fd)
 igt_main
 {
 	unsigned int has_context_isolation = 0;
+	const struct intel_execution_engine2 *e;
 	int fd = -1;
 
 	igt_fixture {
@@ -871,10 +892,12 @@ igt_main
 		igt_warn_on_f(gen > LAST_KNOWN_GEN,
 			      "GEN not recognized! Test needs to be updated to run.\n");
 		igt_skip_on(gen > LAST_KNOWN_GEN);
+
+		/* Context 0 is created on device open. */
+		gem_context_set_all_engines(fd, 0);
 	}
 
-	for (const struct intel_execution_engine2 *e = intel_execution_engines2;
-	     e->name; e++) {
+	__for_each_physical_engine(fd, e) {
 		igt_subtest_group {
 			igt_fixture {
 				igt_require(has_context_isolation & (1 << e->class));
-- 
2.25.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/2] DBS: tests/i915/gem_ctx_isolation: use the gem_engine_topology library, part 2

[Chris Wilson]

Quoting Dale B Stimson (2020-01-22 23:26:57)
> Switch from simple iteration over all potential engines to using
> macro __for_each_physical_engine which only returns engines that are
> actually present.
> 
> For each context (as it is created) call gem_context_set_all_engines
> so that execbuf will interpret the engine specification in the new way.
> 
> Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com>
> ---
>  tests/i915/gem_ctx_isolation.c | 41 ++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index 25113b054..31a20ed3a 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -240,6 +240,25 @@ static bool ignore_register(uint32_t offset)
>         return false;
>  }
>  
> +/*
> + * context_create_plus_all_engines - Same as gem_context_create plus setup.
> + *
> + * This is a convenience function that may be called instead of the sequence
> + * of gem_context_create followed by gem_context_set_all_engines.
> + * If gem_has_engine_topology(), then function gem_context_set_all_engines
> + * indicates that future execbuf calls for this context should interpret the
> + * engine specification in a gem_engine_topology-compatible way.
> + */
> +static uint32_t context_create_plus_all_engines(int fd)
> +{
> +       uint32_t ctx;
> +
> +       ctx = gem_context_create(fd);
> +       gem_context_set_all_engines(fd, ctx);
> +
> +       return ctx;
> +}

gem_context_clone_with_engines() so we can stop assuming that
all-engines is the right answer, because that depends on the conditions
set up by the iterator on the first context.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/2] DBS: tests/i915/gem_ctx_isolation: use the gem_engine_topology library, part 2

[Tvrtko Ursulin]

On 23/01/2020 09:09, Chris Wilson wrote:
> Quoting Dale B Stimson (2020-01-22 23:26:57)
>> Switch from simple iteration over all potential engines to using
>> macro __for_each_physical_engine which only returns engines that are
>> actually present.
>>
>> For each context (as it is created) call gem_context_set_all_engines
>> so that execbuf will interpret the engine specification in the new way.
>>
>> Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com>
>> ---
>>   tests/i915/gem_ctx_isolation.c | 41 ++++++++++++++++++++++++++--------
>>   1 file changed, 32 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
>> index 25113b054..31a20ed3a 100644
>> --- a/tests/i915/gem_ctx_isolation.c
>> +++ b/tests/i915/gem_ctx_isolation.c
>> @@ -240,6 +240,25 @@ static bool ignore_register(uint32_t offset)
>>          return false;
>>   }
>>   
>> +/*
>> + * context_create_plus_all_engines - Same as gem_context_create plus setup.
>> + *
>> + * This is a convenience function that may be called instead of the sequence
>> + * of gem_context_create followed by gem_context_set_all_engines.
>> + * If gem_has_engine_topology(), then function gem_context_set_all_engines
>> + * indicates that future execbuf calls for this context should interpret the
>> + * engine specification in a gem_engine_topology-compatible way.
>> + */
>> +static uint32_t context_create_plus_all_engines(int fd)
>> +{
>> +       uint32_t ctx;
>> +
>> +       ctx = gem_context_create(fd);
>> +       gem_context_set_all_engines(fd, ctx);
>> +
>> +       return ctx;
>> +}
> 
> gem_context_clone_with_engines() so we can stop assuming that
> all-engines is the right answer, because that depends on the conditions
> set up by the iterator on the first context.

gem_context_clone_with_engines was agreed upon in principle some time 
ago but never implemented. I have now posted this as 
https://patchwork.freedesktop.org/series/72464/ and plan to merge it 
once it passes CI.

Dale, Arjun, Krishnaiah and Sreedhar - you have in progress patches 
which use gem_context_set_all_engines which will be gone and you will 
need to adjust your work accordingly.

Sreedhar specifically for your change in gem_exec_parallel we will need 
to add a new helper which transfers the engine map from one fd/context 
to another. I will copy you on a patch which will add it.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 2/2] DBS: tests/i915/gem_ctx_isolation: use the gem_engine_topology library, part 2

[Dale B Stimson]

On 2020-01-23 15:59:33, Tvrtko Ursulin wrote:
> gem_context_clone_with_engines was agreed upon in principle some time ago
> but never implemented. I have now posted this as
> https://patchwork.freedesktop.org/series/72464/ and plan to merge it once it
> passes CI.
> 
> Dale, Arjun, Krishnaiah and Sreedhar - you have in progress patches which
> use gem_context_set_all_engines which will be gone and you will need to
> adjust your work accordingly.
> 
> Sreedhar specifically for your change in gem_exec_parallel we will need to
> add a new helper which transfers the engine map from one fd/context to
> another. I will copy you on a patch which will add it.

I have a new iteration of the gem_ctx_isolation patches (on top of your
patch and using gem_context_clone_with_engines) which I will submit to the
ML as soon as your patch merges.

Thanks,
Dale
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev