[v5] tests/i915/gem_exec_async: Update with engine discovery

Submitted by Chris Wilson on July 31, 2019, 8:05 a.m.

Details

Message ID 156456032572.6373.13078860351376372466@skylake-alporthouse-com
State New
Headers show
Series "tests/i915/gem_exec_async: Update with engine discovery" ( rev: 2 ) in IGT (deprecated)

Not browsing as part of any series.

Commit Message

Chris Wilson July 31, 2019, 8:05 a.m.
Quoting Tvrtko Ursulin (2019-07-31 07:12:35)
> 
> On 30/07/2019 18:07, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
> >>
> >> On 30/07/2019 09:04, Ramalingam C wrote:
> >>> On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>> On 30/07/2019 04:58, Ramalingam C wrote:
> >>>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)
> >>>>
> >>>> I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.
> >>>>
> >>>> And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)
> >>> For me current shape looks good enough :) I will use the uint64_t for
> >>> parameter types.
> >>
> >> Okay but please add some documentation for the helper (we've been very
> >> bad in this work in this respect so far) and also filter out non-engine
> >> selection bits from the flags before doing the checks.
> >>
> >>>>
> >>>>> +{
> >>>>> +   if (eb_flag1 == eb_flag2)
> >>>>> +           return false;
> >>>>> +
> >>>>> +   /* DEFAULT stands for RENDER in legacy uAPI*/
> >>>>> +   if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> >>>>> +        (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> >>>>> +           return false;
> >>>>> +
> >>>>> +   /*
> >>>>> +    * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> >>>>> +    * same engine.
> >>>>> +    */
> >>>>> +   if ((eb_flag1 == I915_EXEC_BSD) &&
> >>>>> +       (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>>>> +                   return false;
> >>>>> +
> >>>>> +   if ((eb_flag2 == I915_EXEC_BSD) &&
> >>>>> +       (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>>>> +                   return false;
> >>>>
> >>>> I think this works.
> >>>>
> >>>> I've also come up with something to merge the two checks, not 100% it's correct or more readable:
> >>>>
> >>>> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
> >>>>       !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
> >>>>       (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
> >>>>       return false;
> >>>>
> >>>> Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.
> >>> readability perspective, we could stick to the original version. If we
> >>> want to go ahead we need to do below ops:
> >>
> >> Stick with your version I think.
> >>
> >> Chris is being quiet BTW. Either we are below his radar and he'll scream
> >> later, or we managed to approach something he finds passable. ;)
> > 
> > Just waiting until I have to use it in anger :)
> > 
> > Gut feeling is that I won't have to use it, in that if I have two
> > different timelines pointing to the same physical engine, do I really
> > care? The situations where I would have distinct engine mappings strike
> > me as being cases where I'm testing timelines; otherwise I would create
> > contexts with the same ctx->engines[] map.
> 
> I do dislike this complication of testing uapi flags but figuring out 
> the physical engines under the covers. Do you think it would be clearer 
> do drop this helper and instead use two contexts in this test? It would 
> make it dependent on contexts though..

Going back to look at the use case...

 tests/i915/gem_exec_async.c | 81 ++++++++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 18 deletions(-)

(Fill in the gaps for the new structs!)

Is more of what I was expecting. (Eventually no one will notice the BSD
aliasing bit rotting and we can remove it from the uABI.)
-Chris

Patch hide | download patch | download mbox

diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
index 9a06af7e2..fafca98ad 100644
--- a/tests/i915/gem_exec_async.c
+++ b/tests/i915/gem_exec_async.c
@@ -80,7 +80,10 @@  static void store_dword(int fd, unsigned ring,
 	gem_close(fd, obj[1].handle);
 }
 
-static void one(int fd, unsigned ring, uint32_t flags)
+static void one(int fd,
+		unsigned int idx,
+		const struct intel_execution_engine2 *engines,
+		unsigned int num_engines)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
 	struct drm_i915_gem_exec_object2 obj[2];
@@ -88,7 +91,6 @@  static void one(int fd, unsigned ring, uint32_t flags)
 #define BATCH 1
 	struct drm_i915_gem_relocation_entry reloc;
 	struct drm_i915_gem_execbuffer2 execbuf;
-	unsigned int other;
 	uint32_t *batch;
 	int i;
 
@@ -138,19 +140,17 @@  static void one(int fd, unsigned ring, uint32_t flags)
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.buffer_count = 2;
-	execbuf.flags = ring | flags;
+	execbuf.flags = engines[idx].flags;
 	igt_require(__gem_execbuf(fd, &execbuf) == 0);
 	gem_close(fd, obj[BATCH].handle);
 
 	i = 0;
-	for_each_physical_engine(fd, other) {
-		if (other == ring)
+	for (unsigned int other = 0; other < num_engines; other++) {
+		if (other == idx)
 			continue;
 
-		if (!gem_can_store_dword(fd, other))
-			continue;
-
-		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
+		store_dword(fd, engines[other].flags,
+			    obj[SCRATCH].handle, 4*i, i);
 		i++;
 	}
 
@@ -185,7 +185,6 @@  static bool has_async_execbuf(int fd)
 
 igt_main
 {
-	const struct intel_execution_engine *e;
 	int fd = -1;
 
 	igt_skip_on_simulation();
@@ -199,15 +198,61 @@  igt_main
 		igt_fork_hang_detector(fd);
 	}
 
-	for (e = intel_execution_engines; e->name; e++) {
-		/* default exec-id is purely symbolic */
-		if (e->exec_id == 0)
-			continue;
+	/* First up, check the legacy engine selector ABI for independence */
+	igt_subtest_group {
+		struct intel_execution_engine2 engines[64];
+		unsigned int num_engines = 0;
+
+		igt_fixture {
+			const struct intel_execution_engine *e;
+
+			for (e = intel_execution_engines; e->name; e++) {
+				if (!gem_ring_has_physical_engine(fd, e->exec_id | e->flags))
+					continue;
+
+				/* Must be unique, no unknowable BSD aliases! */
+				engines[num_engines] =
+					gem_eb_flags_to_engine(e->exec_id | e->flags);
+				if (engines[num_engines].flags != -1)
+					continue;
+
+				if (!gem_can_store_dword(fd, engines[num_engines].flags))
+					continue;
+
+				num_engines++;
+				if (num_engines == ARRAY_SIZE(engines))
+					break;
+			}
+		}
+
+		for (unsigned int n = 0; n < num_engines; n++) {
+			igt_subtest_f("legacy-concurrent-writes-%s",
+				      engines[n].name)
+				one(fd, n, engines, num_engines);
+		}
+	}
+
+	/* Same again, but using a ctx->engine[] map and indices */
+	igt_subtest_group {
+		struct intel_execution_engine2 engines[64];
+		unsigned int num_engines = 0;
+
+		igt_fixture {
+			const struct intel_execution_engine2 *e;
+
+			__for_each_physical_engine(fd, e) {
+				if (!gem_class_can_store_dword(fd, e->class))
+					continue;
+
+				engines[num_engines++] = *e;
+				if (num_engines == ARRAY_SIZE(engines))
+					break;
+			}
+		}
 
-		igt_subtest_f("concurrent-writes-%s", e->name) {
-			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
-			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
-			one(fd, e->exec_id, e->flags);
+		for (unsigned int n = 0; n < num_engines; n++) {
+			igt_subtest_f("concurrent-writes-%s", engines[n].name)
+				one(fd, n, engines, num_engines);
 		}
 	}


Comments

Quoting Chris Wilson (2019-07-31 09:05:25)
> +       /* First up, check the legacy engine selector ABI for independence */
> +       igt_subtest_group {
> +               struct intel_execution_engine2 engines[64];
> +               unsigned int num_engines = 0;
> +
> +               igt_fixture {
> +                       const struct intel_execution_engine *e;
> +
> +                       for (e = intel_execution_engines; e->name; e++) {
> +                               if (!gem_ring_has_physical_engine(fd, e->exec_id | e->flags))
> +                                       continue;
> +
> +                               /* Must be unique, no unknowable BSD aliases! */
> +                               engines[num_engines] =
> +                                       gem_eb_flags_to_engine(e->exec_id | e->flags);
> +                               if (engines[num_engines].flags != -1)
> +                                       continue;
> +
> +                               if (!gem_can_store_dword(fd, engines[num_engines].flags))
> +                                       continue;
> +
> +                               num_engines++;
> +                               if (num_engines == ARRAY_SIZE(engines))
> +                                       break;
> +                       }
> +               }

I also expect this to be repeated enough to merit something like
num_engines = gem_engines_get_legacy(fd,
				     engines, ARRAY_SIZE(engines),
				     LEGACY_PHYSICAL | LEGACY_CAN_STORE_DWORD);
-Chris