[19/19] i915/gem_exec_balancer: Exercise bonded pairs

Submitted by Chris Wilson on March 8, 2019, 6:11 p.m.

Details

Message ID 20190308181129.15562-19-chris@chris-wilson.co.uk
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in IGT (deprecated)

Not browsing as part of any series.

Commit Message

Chris Wilson March 8, 2019, 6:11 p.m.
The submit-fence + load_balancing apis allow for us to execute a named
pair of engines in parallel; that this by submitting a request to one
engine, we can then use the generated submit-fence to submit a second
request to another engine and have it execute at the same time.
Furthermore, by specifying bonded pairs, we can direct the virtual
engine to use a particular engine in parallel to the first request.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_balancer.c | 174 +++++++++++++++++++++++++++++++--
 1 file changed, 165 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/i915/gem_exec_balancer.c b/tests/i915/gem_exec_balancer.c
index d9fdffe67..e727090e4 100644
--- a/tests/i915/gem_exec_balancer.c
+++ b/tests/i915/gem_exec_balancer.c
@@ -102,12 +102,44 @@  list_engines(int i915, uint32_t class_mask, unsigned int *out)
 	return engines;
 }
 
+static int __set_engines(int i915, uint32_t ctx,
+			 const struct class_instance *ci,
+			 unsigned int count)
+{
+	struct engines {
+		uint64_t extension;
+		uint64_t class_instance[count];
+	} engines;
+	struct drm_i915_gem_context_param p = {
+		.ctx_id = ctx,
+		.param = I915_CONTEXT_PARAM_ENGINES,
+		.size = sizeof(engines),
+		.value = to_user_pointer(&engines)
+	};
+
+	engines.extension = 0;
+	memcpy(engines.class_instance, ci, sizeof(engines.class_instance));
+
+	return __gem_context_set_param(i915, &p);
+}
+
+static void set_engines(int i915, uint32_t ctx,
+			const struct class_instance *ci,
+			unsigned int count)
+{
+	igt_assert_eq(__set_engines(i915, ctx, ci, count), 0);
+}
+
 static int __set_load_balancer(int i915, uint32_t ctx,
 			       const struct class_instance *ci,
-			       unsigned int count)
+			       unsigned int count,
+			       void *ext)
 {
 	struct i915_context_engines_load_balance balancer = {
-		{ .name = I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE },
+		{ 
+			.next_extension = to_user_pointer(ext),
+			.name = I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE
+	       	},
 		.engines_mask = ~0ull,
 	};
 	I915_DEFINE_CONTEXT_PARAM_ENGINES(engines, count + 1);
@@ -128,9 +160,10 @@  static int __set_load_balancer(int i915, uint32_t ctx,
 
 static void set_load_balancer(int i915, uint32_t ctx,
 			      const struct class_instance *ci,
-			      unsigned int count)
+			      unsigned int count,
+			      void *ext)
 {
-	igt_assert_eq(__set_load_balancer(i915, ctx, ci, count), 0);
+	igt_assert_eq(__set_load_balancer(i915, ctx, ci, count, ext), 0);
 }
 
 static uint32_t load_balancer_create(int i915,
@@ -140,7 +173,7 @@  static uint32_t load_balancer_create(int i915,
 	uint32_t ctx;
 
 	ctx = gem_context_create(i915);
-	set_load_balancer(i915, ctx, ci, count);
+	set_load_balancer(i915, ctx, ci, count, NULL);
 
 	return ctx;
 }
@@ -254,7 +287,7 @@  static void individual(int i915)
 
 		for (int pass = 0; pass < count; pass++) { /* approx. count! */
 			igt_permute_array(ci, count, igt_exchange_int64);
-			set_load_balancer(i915, ctx, ci, count);
+			set_load_balancer(i915, ctx, ci, count, NULL);
 			for (unsigned int n = 0; n < count; n++)
 				check_individual_engine(i915, ctx, ci, n);
 		}
@@ -265,6 +298,123 @@  static void individual(int i915)
 	gem_context_destroy(i915, ctx);
 }
 
+static void bonded(int i915, unsigned int flags)
+#define CORK 0x1
+{
+	struct class_instance *master_engines;
+	uint32_t master;
+	struct bond {
+		uint64_t next_extension;
+		uint64_t name;
+
+		uint16_t class;
+		uint16_t inst;
+		uint32_t flags;
+		uint64_t mask;
+	} bonds[16];
+
+	/*
+	 * I915_CONTEXT_PARAM_ENGINE provides an extension that allows us
+	 * to specify which engine(s) to pair with a parallel (EXEC_SUBMIT)
+	 * request submitted to another engine.
+	 */
+
+	master = gem_queue_create(i915);
+
+	memset(bonds, 0, sizeof(bonds));
+	for (int n = 0; n < ARRAY_SIZE(bonds); n++) {
+		bonds[n].name = I915_CONTEXT_ENGINES_EXT_BOND;
+		bonds[n].next_extension =
+			n ? to_user_pointer(&bonds[n - 1]) : 0;
+		bonds[n].mask = 1 << n;
+	}
+
+	for (int mask = 0; mask < 32; mask++) {
+		unsigned int count, limit;
+		struct class_instance *siblings;
+		uint32_t ctx;
+		int n;
+
+		siblings = list_engines(i915, 1u << mask, &count);
+		if (!siblings)
+			continue;
+
+		if (count < 2) {
+			free(siblings);
+			continue;
+		}
+
+		igt_debug("Found %d engines of class %d\n", count, mask);
+
+		master_engines = list_engines(i915, ~(1u << mask), &limit);
+		set_engines(i915, master, master_engines, limit);
+
+		limit = min(count, limit);
+		for (n = 0; n < limit; n++) {
+			bonds[n].class = master_engines[n].class;
+			bonds[n].inst = master_engines[n].instance;
+		}
+
+		ctx = gem_context_clone(i915,
+				       	master, I915_CONTEXT_CLONE_VM,
+					I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE);
+		set_load_balancer(i915, ctx, siblings, count, &bonds[limit - 1]);
+
+		for (n = 0; n < limit; n++) {
+			struct drm_i915_gem_execbuffer2 eb;
+			IGT_CORK_HANDLE(cork);
+			igt_spin_t *spin, *plug;
+			double load;
+			int pmu;
+
+			igt_assert(siblings[n].class != master_engines[n].class);
+
+			pmu = perf_i915_open(I915_PMU_ENGINE_BUSY(siblings[n].class,
+								  siblings[n].instance));
+
+			plug = NULL;
+			if (flags & CORK) {
+				plug = __igt_spin_batch_new(i915,
+							    .ctx = master,
+							    .engine = n + 1,
+							    .dependency = igt_cork_plug(&cork, i915));
+			}
+
+			spin = __igt_spin_batch_new(i915,
+						    .ctx = master,
+						    .engine = n + 1,
+						    .flags = IGT_SPIN_FENCE_OUT);
+
+			eb = spin->execbuf;
+			eb.rsvd1 = ctx;
+			eb.rsvd2 = spin->out_fence;
+			eb.flags = I915_EXEC_FENCE_SUBMIT;
+			gem_execbuf(i915, &eb);
+
+			if (plug) {
+				igt_cork_unplug(&cork);
+				igt_spin_batch_free(i915, plug);
+			}
+
+			load = measure_load(pmu, 10000);
+			igt_spin_batch_free(i915, spin);
+
+			close(pmu);
+
+			igt_assert_f(load > 0.90,
+				     "engine %d (class:instance %d:%d) was found to be only %.1f%% busy\n",
+				     n, siblings[n].class, siblings[n].instance,
+				     load*100);
+		}
+
+		gem_context_destroy(i915, ctx);
+		free(master_engines);
+		free(siblings);
+	}
+
+	gem_context_destroy(i915, master);
+}
+
 static int add_pmu(int pmu, const struct class_instance *ci)
 {
 	return perf_i915_open_group(I915_PMU_ENGINE_BUSY(ci->class,
@@ -447,7 +597,7 @@  static void semaphore(int i915)
 		count = ARRAY_SIZE(block);
 
 		for (int i = 0; i < count; i++) {
-			set_load_balancer(i915, block[i], ci, count);
+			set_load_balancer(i915, block[i], ci, count, NULL);
 			spin[i] = __igt_spin_batch_new(i915,
 						       .ctx = block[i],
 						       .dependency = scratch);
@@ -458,7 +608,7 @@  static void semaphore(int i915)
 		 * or we let the vip through. If not, we hang.
 		 */
 		vip = gem_context_create(i915);
-		set_load_balancer(i915, vip, ci, count);
+		set_load_balancer(i915, vip, ci, count, NULL);
 		ping(i915, vip, 0);
 		gem_context_destroy(i915, vip);
 
@@ -573,7 +723,7 @@  static bool has_load_balancer(int i915)
 	int err;
 
 	ctx = gem_context_create(i915);
-	err = __set_load_balancer(i915, ctx, &ci, 1);
+	err = __set_load_balancer(i915, ctx, &ci, 1, NULL);
 	gem_context_destroy(i915, ctx);
 
 	return err == 0;
@@ -621,6 +771,12 @@  igt_main
 	igt_subtest("smoke")
 		smoketest(i915, 20);
 
+	igt_subtest("bonded-imm")
+		bonded(i915, 0);
+
+	igt_subtest("bonded-cork")
+		bonded(i915, CORK);
+
 	igt_fixture {
 		igt_stop_hang_detector();
 	}

Comments

Hi Chris,

just a nitpick, a warning that came out when I applied the
patches.

>  	struct i915_context_engines_load_balance balancer = {
> -		{ .name = I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE },
> +		{ 
                 ^
trailing whitespace

> +			.next_extension = to_user_pointer(ext),
> +			.name = I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE
> +	       	},
         ^^^^^^|
               +-> tab

spaces before tab

Andi
Hi again,

I was thinking...

> +static void bonded(int i915, unsigned int flags)
> +#define CORK 0x1
> +{
> +	struct class_instance *master_engines;

... shall we just make 'struct class_instance' generic in the
uapi? I do not expect every test that uses class and instance to
define its own structure.

Besides we can avoid a good load of 'offsetof' and make the whole
thing more readable.

Andi
Quoting Andi Shyti (2019-03-11 10:30:23)
> Hi again,
> 
> I was thinking...
> 
> > +static void bonded(int i915, unsigned int flags)
> > +#define CORK 0x1
> > +{
> > +     struct class_instance *master_engines;
> 
> ... shall we just make 'struct class_instance' generic in the
> uapi? I do not expect every test that uses class and instance to
> define its own structure.

But this test is defining its own struct ;)

> Besides we can avoid a good load of 'offsetof' and make the whole
> thing more readable.

Maybe stop using uAPI structs for library structs -- it's not a good
strategy long term. (We only need the uAPI for interfacing, but for the
library we tend to want other details such as mmio_base, name, etc)

Using offsetof to define the size is reasonably common practice for
variable length arrays (except unless you are paranoid about overflow in
every step of the computation).
-Chris