[PATCH v2 i-g-t 0/6] benchmarks/gem_wsim: Extend engine selection syntax

[PATCH v2 i-g-t 0/6] benchmarks/gem_wsim: Extend engine selection syntax

[Marcin Bernatowicz]

Introduces significant changes to the engine selection syntax:
- Dynamically generates the list of available physical engines.
- Identifies engines using [class:instance:gt] tuples.
- Allows specifying engine instance and gt as 
  `engine_class[<engine_instance>-<gt_id>]`.
- Adds support for compute engine class (CCS).
- Maintains 1-based engine instance ids for compatibility 
  with existing workload definitions.
- Enhances `w_step` with `engine_idx`
  and `request_idx` for throttling functionality
  (both populated during prepare workload phase).

Adds command line option (-l) to list physical engines.

v2: Split into multiple patches for easier review (Tvrtko)

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>

Marcin Bernatowicz (6):
  benchmarks/gem_wsim: Introduce intel_engines structure
  benchmarks/gem_wsim: Unify bond handling
  benchmarks/gem_wsim: Introduce engine_idx to streamline engine
    selection
  benchmarks/gem_wsim: Update request_idx in prepare phase
  benchmarks/gem_wsim: Extend engine selection syntax
  benchmarks/gem_wsim: Option to list physical engines

 benchmarks/gem_wsim.c | 835 +++++++++++++++++++++++-------------------
 1 file changed, 449 insertions(+), 386 deletions(-)

-- 
2.31.1

[PATCH v2 i-g-t 1/6] benchmarks/gem_wsim: Introduce intel_engines structure

[Marcin Bernatowicz]

Introduction of struct intel_engines, which encapsulates the number of
engines (nr_engines) and a pointer to an array of engine IDs (engines).
This structural refactor replaces the previous ad-hoc approach of managing
engine maps across various structures (w_step, ctx, etc.)
This change is part of a series of preparatory steps for upcoming
patches.

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
---
 benchmarks/gem_wsim.c | 72 ++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index fce57b894..e98624221 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -126,6 +126,11 @@ struct w_arg {
 	bool sseu;
 };
 
+struct intel_engines {
+	unsigned int nr_engines;
+	enum intel_engine_id *engines;
+};
+
 struct bond {
 	uint64_t mask;
 	enum intel_engine_id master;
@@ -165,10 +170,7 @@ struct w_step {
 		int target;
 		int throttle;
 		int priority;
-		struct {
-			unsigned int engine_map_count;
-			enum intel_engine_id *engine_map;
-		};
+		struct intel_engines engine_map;
 		bool load_balance;
 		struct {
 			uint64_t bond_mask;
@@ -220,8 +222,7 @@ struct xe_exec_queue {
 struct ctx {
 	uint32_t id;
 	int priority;
-	unsigned int engine_map_count;
-	enum intel_engine_id *engine_map;
+	struct intel_engines engine_map;
 	unsigned int bond_count;
 	struct bond *bonds;
 	bool load_balance;
@@ -722,15 +723,17 @@ static int parse_engine_map(struct w_step *step, const char *_str)
 			return -1; /* TODO */
 
 		add = engine == VCS ? num_engines_in_class(VCS) : 1;
-		step->engine_map_count += add;
-		step->engine_map = realloc(step->engine_map,
-					   step->engine_map_count *
-					   sizeof(step->engine_map[0]));
+		step->engine_map.nr_engines += add;
+		step->engine_map.engines = realloc(step->engine_map.engines,
+						step->engine_map.nr_engines *
+						sizeof(step->engine_map.engines[0]));
 
 		if (engine != VCS)
-			step->engine_map[step->engine_map_count - add] = engine;
+			step->engine_map.engines[step->engine_map.nr_engines - add] = engine;
 		else
-			fill_engines_id_class(&step->engine_map[step->engine_map_count - add], VCS);
+			fill_engines_id_class(
+				&step->engine_map.engines[step->engine_map.nr_engines - add],
+				VCS);
 	}
 
 	return 0;
@@ -1608,8 +1611,8 @@ find_engine_in_map(struct ctx *ctx, enum intel_engine_id engine)
 {
 	unsigned int i;
 
-	for (i = 0; i < ctx->engine_map_count; i++) {
-		if (ctx->engine_map[i] == engine)
+	for (i = 0; i < ctx->engine_map.nr_engines; i++) {
+		if (ctx->engine_map.engines[i] == engine)
 			return i + 1;
 	}
 
@@ -1623,7 +1626,7 @@ eb_update_flags(struct workload *wrk, struct w_step *w,
 {
 	struct ctx *ctx = __get_ctx(wrk, w);
 
-	if (ctx->engine_map)
+	if (ctx->engine_map.nr_engines)
 		w->i915.eb.flags = find_engine_in_map(ctx, engine);
 	else
 		eb_set_engine(&w->i915.eb, engine);
@@ -1648,7 +1651,7 @@ xe_get_eq(struct workload *wrk, const struct w_step *w)
 	struct ctx *ctx = __get_ctx(wrk, w);
 	struct xe_exec_queue *eq;
 
-	if (ctx->engine_map) {
+	if (ctx->engine_map.nr_engines) {
 		igt_assert_eq(ctx->xe.nr_queues, 1);
 		igt_assert(ctx->xe.queue_list[0].id);
 		eq = &ctx->xe.queue_list[0];
@@ -1941,7 +1944,7 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask)
 	if (slice_mask == -1)
 		slice_mask = device_sseu.slice_mask;
 
-	if (ctx->engine_map && ctx->load_balance) {
+	if (ctx->engine_map.nr_engines && ctx->load_balance) {
 		sseu.flags = I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX;
 		sseu.engine.engine_class = I915_ENGINE_CLASS_INVALID;
 		sseu.engine.engine_instance = 0;
@@ -2151,9 +2154,8 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 
 			if (w->type == ENGINE_MAP) {
 				ctx->engine_map = w->engine_map;
-				ctx->engine_map_count = w->engine_map_count;
 			} else if (w->type == LOAD_BALANCE) {
-				if (!ctx->engine_map) {
+				if (!ctx->engine_map.nr_engines) {
 					wsim_err("Load balancing needs an engine map!\n");
 					return 1;
 				}
@@ -2232,15 +2234,15 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 
 		__configure_context(ctx_id, wrk->prio);
 
-		if (ctx->engine_map) {
+		if (ctx->engine_map.nr_engines) {
 			struct i915_context_param_engines *set_engines =
-				alloca0(sizeof_param_engines(ctx->engine_map_count + 1));
+				alloca0(sizeof_param_engines(ctx->engine_map.nr_engines + 1));
 			struct i915_context_engines_load_balance *load_balance =
-				alloca0(sizeof_load_balance(ctx->engine_map_count));
+				alloca0(sizeof_load_balance(ctx->engine_map.nr_engines));
 			struct drm_i915_gem_context_param param = {
 				.ctx_id = ctx_id,
 				.param = I915_CONTEXT_PARAM_ENGINES,
-				.size = sizeof_param_engines(ctx->engine_map_count + 1),
+				.size = sizeof_param_engines(ctx->engine_map.nr_engines + 1),
 				.value = to_user_pointer(set_engines),
 			};
 			struct i915_context_engines_bond *last = NULL;
@@ -2252,11 +2254,11 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 				load_balance->base.name =
 					I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE;
 				load_balance->num_siblings =
-					ctx->engine_map_count;
+					ctx->engine_map.nr_engines;
 
-				for (j = 0; j < ctx->engine_map_count; j++)
+				for (j = 0; j < ctx->engine_map.nr_engines; j++)
 					load_balance->engines[j] =
-						get_engine(ctx->engine_map[j]);
+						get_engine(ctx->engine_map.engines[j]);
 			}
 
 			/* Reserve slot for virtual engine. */
@@ -2265,9 +2267,9 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 			set_engines->engines[0].engine_instance =
 				I915_ENGINE_CLASS_INVALID_NONE;
 
-			for (j = 1; j <= ctx->engine_map_count; j++)
+			for (j = 1; j <= ctx->engine_map.nr_engines; j++)
 				set_engines->engines[j] =
-					get_engine(ctx->engine_map[j - 1]);
+					get_engine(ctx->engine_map.engines[j - 1]);
 
 			last = NULL;
 			for (j = 0; j < ctx->bond_count; j++) {
@@ -2289,7 +2291,7 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 						continue;
 
 					idx = find_engine(&set_engines->engines[1],
-							  ctx->engine_map_count,
+							  ctx->engine_map.nr_engines,
 							  e);
 					bond->engines[b++] =
 						set_engines->engines[1 + idx];
@@ -2338,9 +2340,8 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 				continue;
 			if (w->type == ENGINE_MAP) {
 				ctx->engine_map = w->engine_map;
-				ctx->engine_map_count = w->engine_map_count;
 			} else if (w->type == LOAD_BALANCE) {
-				if (!ctx->engine_map) {
+				if (!ctx->engine_map.nr_engines) {
 					wsim_err("Load balancing needs an engine map!\n");
 					return 1;
 				}
@@ -2349,15 +2350,15 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 		}
 
 		/* create exec queue for each referenced engine */
-		if (ctx->engine_map) {
+		if (ctx->engine_map.nr_engines) {
 			ctx->xe.nr_queues = 1;
 			ctx->xe.queue_list = calloc(ctx->xe.nr_queues, sizeof(*ctx->xe.queue_list));
 			igt_assert(ctx->xe.queue_list);
 			eq = &ctx->xe.queue_list[ctx->xe.nr_queues - 1];
-			eq->nr_hwes = ctx->engine_map_count;
+			eq->nr_hwes = ctx->engine_map.nr_engines;
 			eq->hwe_list = calloc(eq->nr_hwes, sizeof(*eq->hwe_list));
 			for (i = 0; i < eq->nr_hwes; ++i) {
-				eq->hwe_list[i] = xe_get_engine(ctx->engine_map[i]);
+				eq->hwe_list[i] = xe_get_engine(ctx->engine_map.engines[i]);
 
 				/* check no mixing classes and no duplicates */
 				for (int j = 0; j < i; ++j) {
@@ -2380,7 +2381,8 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 
 				if (verbose > 3)
 					printf("%u ctx[%d] %s [%u:%u:%u]\n",
-						id, ctx_idx, ring_str_map[ctx->engine_map[i]],
+						id, ctx_idx,
+						ring_str_map[ctx->engine_map.engines[i]],
 						eq->hwe_list[i].engine_class,
 						eq->hwe_list[i].engine_instance,
 						eq->hwe_list[i].gt_id);
-- 
2.31.1

[PATCH v2 i-g-t 2/6] benchmarks/gem_wsim: Unify bond handling

[Marcin Bernatowicz]

This change brings the handling of bonding information in line with
other parts of the code that use structured approaches for similar data,
enhancing overall consistency. Prepares code to use struct intel_engines as
bond.mask.

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
---
 benchmarks/gem_wsim.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index e98624221..98e8fe55c 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -172,10 +172,7 @@ struct w_step {
 		int priority;
 		struct intel_engines engine_map;
 		bool load_balance;
-		struct {
-			uint64_t bond_mask;
-			enum intel_engine_id bond_master;
-		};
+		struct bond bond;
 		int sseu;
 		struct working_set working_set;
 	};
@@ -1146,8 +1143,8 @@ parse_workload(struct w_arg *arg, unsigned int flags, double scale_dur,
 							  "Invalid context at step %u!\n",
 							  nr_steps);
 					} else if (nr == 1) {
-						step.bond_mask = engine_list_mask(field);
-						check_arg(step.bond_mask == 0,
+						step.bond.mask = engine_list_mask(field);
+						check_arg(step.bond.mask == 0,
 							"Invalid siblings list at step %u!\n",
 							nr_steps);
 					} else if (nr == 2) {
@@ -1157,7 +1154,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, double scale_dur,
 							  tmp == DEFAULT,
 							  "Invalid master engine at step %u!\n",
 							  nr_steps);
-						step.bond_master = tmp;
+						step.bond.master = tmp;
 					}
 
 					nr++;
@@ -2174,10 +2171,7 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 						     ctx->bond_count *
 						     sizeof(struct bond));
 				igt_assert(ctx->bonds);
-				ctx->bonds[ctx->bond_count - 1].mask =
-					w->bond_mask;
-				ctx->bonds[ctx->bond_count - 1].master =
-					w->bond_master;
+				ctx->bonds[ctx->bond_count - 1] = w->bond;
 			}
 		}
 	}
-- 
2.31.1

[PATCH v2 i-g-t 3/6] benchmarks/gem_wsim: Introduce engine_idx to streamline engine selection

[Marcin Bernatowicz]

This patch introduces a new member, engine_idx, to the w_step structure.
This index is populated during the workload preparation phase and is
designed to reference an engine within the context's dynamic engine map
or legacy/static engine list.

The introduction of engine_idx significantly simplifies the engine
selection process during the run phase of the benchmark. By directly
associating each workload step with a specific engine index, the patch
eliminates the need for engine identification and mapping logic that was
previously required. This change not only streamlines the execution flow
but also lays the groundwork for supporting dynamic engine lists in
coming patches.

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
---
 benchmarks/gem_wsim.c | 140 +++++++++++++++++++++++++-----------------
 1 file changed, 82 insertions(+), 58 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 98e8fe55c..b50de78ec 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -159,6 +159,7 @@ struct w_step {
 	enum w_type type;
 	unsigned int context;
 	unsigned int engine;
+	unsigned int engine_idx;
 	struct duration duration;
 	struct deps data_deps;
 	struct deps fence_deps;
@@ -529,6 +530,41 @@ static int str_to_engine(const char *str)
 	return -1;
 }
 
+static unsigned int
+engine_to_i915_legacy_ring(const enum intel_engine_id *engine)
+{
+	static const unsigned int eb_engine_map[NUM_ENGINES] = {
+		[DEFAULT] = I915_EXEC_DEFAULT,
+		[RCS] = I915_EXEC_RENDER,
+		[BCS] = I915_EXEC_BLT,
+		[VCS] = I915_EXEC_BSD,
+		[VCS1] = I915_EXEC_BSD | I915_EXEC_BSD_RING1,
+		[VCS2] = I915_EXEC_BSD | I915_EXEC_BSD_RING2,
+		[VECS] = I915_EXEC_VEBOX
+	};
+
+	return eb_engine_map[*engine];
+}
+
+static bool are_equal_engines(const enum intel_engine_id *e1,
+			      const enum intel_engine_id *e2)
+{
+	return *e1 == *e2;
+}
+
+static bool find_engine_in_map(const enum intel_engine_id *engine,
+			       struct intel_engines *engines, unsigned int *idx)
+{
+	igt_assert(idx);
+	for (unsigned int i = 0; i < engines->nr_engines; ++i)
+		if (are_equal_engines(engine, &engines->engines[i])) {
+			*idx = i;
+			return true;
+		}
+
+	return false;
+}
+
 static struct intel_engine_data *query_engines(void)
 {
 	static struct intel_engine_data engines = {};
@@ -1587,47 +1623,10 @@ static unsigned int create_bb(struct w_step *w, int self)
 	return r;
 }
 
-static const unsigned int eb_engine_map[NUM_ENGINES] = {
-	[DEFAULT] = I915_EXEC_DEFAULT,
-	[RCS] = I915_EXEC_RENDER,
-	[BCS] = I915_EXEC_BLT,
-	[VCS] = I915_EXEC_BSD,
-	[VCS1] = I915_EXEC_BSD | I915_EXEC_BSD_RING1,
-	[VCS2] = I915_EXEC_BSD | I915_EXEC_BSD_RING2,
-	[VECS] = I915_EXEC_VEBOX
-};
-
 static void
-eb_set_engine(struct drm_i915_gem_execbuffer2 *eb, enum intel_engine_id engine)
-{
-	eb->flags = eb_engine_map[engine];
-}
-
-static unsigned int
-find_engine_in_map(struct ctx *ctx, enum intel_engine_id engine)
+eb_update_flags(struct workload *wrk, struct w_step *w)
 {
-	unsigned int i;
-
-	for (i = 0; i < ctx->engine_map.nr_engines; i++) {
-		if (ctx->engine_map.engines[i] == engine)
-			return i + 1;
-	}
-
-	igt_assert(ctx->load_balance);
-	return 0;
-}
-
-static void
-eb_update_flags(struct workload *wrk, struct w_step *w,
-		enum intel_engine_id engine)
-{
-	struct ctx *ctx = __get_ctx(wrk, w);
-
-	if (ctx->engine_map.nr_engines)
-		w->i915.eb.flags = find_engine_in_map(ctx, engine);
-	else
-		eb_set_engine(&w->i915.eb, engine);
-
+	w->i915.eb.flags = w->engine_idx;
 	w->i915.eb.flags |= I915_EXEC_HANDLE_LUT;
 	w->i915.eb.flags |= I915_EXEC_NO_RELOC;
 
@@ -1646,19 +1645,9 @@ static struct xe_exec_queue *
 xe_get_eq(struct workload *wrk, const struct w_step *w)
 {
 	struct ctx *ctx = __get_ctx(wrk, w);
-	struct xe_exec_queue *eq;
 
-	if (ctx->engine_map.nr_engines) {
-		igt_assert_eq(ctx->xe.nr_queues, 1);
-		igt_assert(ctx->xe.queue_list[0].id);
-		eq = &ctx->xe.queue_list[0];
-	} else {
-		igt_assert(w->engine >= 0 && w->engine < ctx->xe.nr_queues);
-		igt_assert(ctx->xe.queue_list[w->engine].id);
-		eq = &ctx->xe.queue_list[w->engine];
-	}
-
-	return eq;
+	igt_assert_lt(w->engine_idx, ctx->xe.nr_queues);
+	return &ctx->xe.queue_list[w->engine_idx];
 }
 
 static struct vm *
@@ -1686,7 +1675,6 @@ static uint32_t alloc_bo(int i915, unsigned long *size)
 static void
 alloc_step_batch(struct workload *wrk, struct w_step *w)
 {
-	enum intel_engine_id engine = w->engine;
 	struct dep_entry *dep;
 	unsigned int j = 0;
 	unsigned int nr_obj = 2 + w->data_deps.nr;
@@ -1774,7 +1762,7 @@ alloc_step_batch(struct workload *wrk, struct w_step *w)
 	w->i915.eb.buffer_count = j + 1;
 	w->i915.eb.rsvd1 = get_ctxid(wrk, w);
 
-	eb_update_flags(wrk, w, engine);
+	eb_update_flags(wrk, w);
 #ifdef DEBUG
 	printf("%u: %u:|", w->idx, w->i915.eb.buffer_count);
 	for (i = 0; i <= j; i++)
@@ -2179,7 +2167,7 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 	/*
 	 * Create and configure contexts.
 	 */
-	for_each_ctx(ctx, wrk) {
+	__for_each_ctx(ctx, wrk, ctx_idx) {
 		struct drm_i915_gem_context_create_ext_setparam ext = {
 			.base.name = I915_CONTEXT_CREATE_EXT_SETPARAM,
 			.param.param = I915_CONTEXT_PARAM_VM,
@@ -2241,6 +2229,22 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 			};
 			struct i915_context_engines_bond *last = NULL;
 
+			/* update engine_idx */
+			for_each_w_step(w, wrk) {
+				if (w->context != ctx_idx)
+					continue;
+				if (w->type == BATCH) {
+					unsigned int map_idx = 0;
+
+					if (find_engine_in_map(&w->engine, &ctx->engine_map,
+								&map_idx))
+						/* 0 is virtual, map indexes are shifted by one */
+						w->engine_idx = map_idx + 1;
+					else
+						igt_assert(ctx->load_balance);
+				}
+			}
+
 			if (ctx->load_balance) {
 				set_engines->extensions =
 					to_user_pointer(load_balance);
@@ -2296,6 +2300,15 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 			load_balance->base.next_extension = to_user_pointer(last);
 
 			gem_context_set_param(fd, &param);
+		} else {
+			/* update engine_idx */
+			for_each_w_step(w, wrk) {
+				if (w->context != ctx_idx)
+					continue;
+				if (w->type == BATCH) {
+					w->engine_idx = engine_to_i915_legacy_ring(&w->engine);
+				}
+			}
 		}
 
 		if (wrk->sseu) {
@@ -2382,6 +2395,14 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 						eq->hwe_list[i].gt_id);
 			}
 
+			/* update engine_idx */
+			for_each_w_step(w, wrk) {
+				if (w->context != ctx_idx)
+					continue;
+				if (w->type == BATCH)
+					w->engine_idx = 0;
+			}
+
 			xe_exec_queue_create_(ctx, eq);
 		} else {
 			int engine_classes[NUM_ENGINES] = {};
@@ -2392,8 +2413,11 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 			for_each_w_step(w, wrk) {
 				if (w->context != ctx_idx)
 					continue;
-				if (w->type == BATCH)
+				if (w->type == BATCH) {
 					engine_classes[w->engine]++;
+					/* update engine_idx */
+					w->engine_idx = w->engine;
+				}
 			}
 
 			for (i = 0; i < NUM_ENGINES; i++) {
@@ -2630,12 +2654,12 @@ static void do_xe_exec(struct workload *wrk, struct w_step *w)
 }
 
 static void
-do_eb(struct workload *wrk, struct w_step *w, enum intel_engine_id engine)
+do_eb(struct workload *wrk, struct w_step *w)
 {
 	struct dep_entry *dep;
 	unsigned int i;
 
-	eb_update_flags(wrk, w, engine);
+	eb_update_flags(wrk, w);
 	update_bb_start(wrk, w);
 
 	for_each_dep(dep, w->fence_deps) {
@@ -2828,7 +2852,7 @@ static void *run_workload(void *data)
 			if (is_xe)
 				do_xe_exec(wrk, w);
 			else
-				do_eb(wrk, w, engine);
+				do_eb(wrk, w);
 
 			if (w->request != -1) {
 				igt_list_del(&w->rq_link);
-- 
2.31.1

[PATCH v2 i-g-t 4/6] benchmarks/gem_wsim: Update request_idx in prepare phase

[Marcin Bernatowicz]

Renamed `request` to `request_idx` for consistency with `engine_idx`.
The index references an element of device's physical engines array.
Similar to engine_idx the field is initialized in workload's preparation
phase. Field is used for throttling functionality, enabling control over
the rate of requests on a given engine.

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
---
 benchmarks/gem_wsim.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index b50de78ec..821746feb 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -181,7 +181,7 @@ struct w_step {
 	/* Implementation details */
 	unsigned int idx;
 	struct igt_list_head rq_link;
-	unsigned int request;
+	unsigned int request_idx;
 	unsigned int preempt_us;
 
 	union {
@@ -1300,7 +1300,7 @@ add_step:
 			step.delay = __duration(step.delay, scale_time);
 
 		step.idx = nr_steps++;
-		step.request = -1;
+		step.rq_link.next = step.rq_link.prev = NULL;
 		steps = realloc(steps, sizeof(step) * nr_steps);
 		igt_assert(steps);
 
@@ -2229,7 +2229,7 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 			};
 			struct i915_context_engines_bond *last = NULL;
 
-			/* update engine_idx */
+			/* update engine_idx and request_idx */
 			for_each_w_step(w, wrk) {
 				if (w->context != ctx_idx)
 					continue;
@@ -2242,6 +2242,8 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 						w->engine_idx = map_idx + 1;
 					else
 						igt_assert(ctx->load_balance);
+
+					w->request_idx = ctx->engine_map.engines[map_idx];
 				}
 			}
 
@@ -2301,12 +2303,13 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 
 			gem_context_set_param(fd, &param);
 		} else {
-			/* update engine_idx */
+			/* update engine_idx and request_idx */
 			for_each_w_step(w, wrk) {
 				if (w->context != ctx_idx)
 					continue;
 				if (w->type == BATCH) {
 					w->engine_idx = engine_to_i915_legacy_ring(&w->engine);
+					w->request_idx = w->engine;
 				}
 			}
 		}
@@ -2399,8 +2402,10 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 			for_each_w_step(w, wrk) {
 				if (w->context != ctx_idx)
 					continue;
-				if (w->type == BATCH)
+				if (w->type == BATCH) {
 					w->engine_idx = 0;
+					w->request_idx = ctx->engine_map.engines[w->engine_idx];
+				}
 			}
 
 			xe_exec_queue_create_(ctx, eq);
@@ -2415,8 +2420,9 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 					continue;
 				if (w->type == BATCH) {
 					engine_classes[w->engine]++;
-					/* update engine_idx */
+					/* update engine_idx and request_idx */
 					w->engine_idx = w->engine;
+					w->request_idx = w->engine;
 				}
 			}
 
@@ -2733,7 +2739,6 @@ static void *run_workload(void *data)
 		clock_gettime(CLOCK_MONOTONIC, &repeat_start);
 
 		for_each_w_step(w, wrk) {
-			enum intel_engine_id engine = w->engine;
 			int do_sleep = 0;
 
 			if (!wrk->run)
@@ -2854,13 +2859,12 @@ static void *run_workload(void *data)
 			else
 				do_eb(wrk, w);
 
-			if (w->request != -1) {
+			if (w->rq_link.next) {
 				igt_list_del(&w->rq_link);
-				wrk->nrequest[w->request]--;
+				wrk->nrequest[w->request_idx]--;
 			}
-			w->request = engine;
-			igt_list_add_tail(&w->rq_link, &wrk->requests[engine]);
-			wrk->nrequest[engine]++;
+			igt_list_add_tail(&w->rq_link, &wrk->requests[w->request_idx]);
+			wrk->nrequest[w->request_idx]++;
 
 			if (!wrk->run)
 				break;
@@ -2869,17 +2873,16 @@ static void *run_workload(void *data)
 				w_step_sync(w);
 
 			if (qd_throttle > 0) {
-				while (wrk->nrequest[engine] > qd_throttle) {
+				while (wrk->nrequest[w->request_idx] > qd_throttle) {
 					struct w_step *s;
 
-					s = igt_list_first_entry(&wrk->requests[engine],
+					s = igt_list_first_entry(&wrk->requests[w->request_idx],
 								 s, rq_link);
 
 					w_step_sync(s);
 
-					s->request = -1;
 					igt_list_del(&s->rq_link);
-					wrk->nrequest[engine]--;
+					wrk->nrequest[w->request_idx]--;
 				}
 			}
 		}
-- 
2.31.1

[PATCH v2 i-g-t 5/6] benchmarks/gem_wsim: Extend engine selection syntax

[Marcin Bernatowicz]

This patch introduces a more flexible and detailed approach to
specifying engines.
The tool now dynamically generates a list of available physical engines
by querying the device, moving away from a static enumeration of engine
IDs.
Engines are now identified using [class:instance:gt] tuples.
This approach accommodates the specification of engine instances in the
format `engine_class[<engine_instance>-<gt_id>]`, enhancing the
granularity of engine selection, ex. First VCS engine may be specified
as VCS, VCS1, and VCS1-0.
The patch adds support for the compute engine class (CCS).
To maintain compatibility with existing workload definitions, the patch
ensures that 1-based engine instance IDs are preserved.

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
---
 benchmarks/gem_wsim.c | 641 +++++++++++++++++++++---------------------
 1 file changed, 320 insertions(+), 321 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 821746feb..effe8a479 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -68,17 +68,44 @@
 #include "xe/xe_ioctl.h"
 #include "xe/xe_spin.h"
 
-enum intel_engine_id {
-	DEFAULT,
+enum intel_engine_class {
 	RCS,
 	BCS,
 	VCS,
-	VCS1,
-	VCS2,
 	VECS,
-	NUM_ENGINES
+	CCS,
+	NUM_ENGINE_CLASSES,
 };
 
+_Static_assert(RCS == DRM_XE_ENGINE_CLASS_RENDER, "mismatch");
+_Static_assert(BCS == DRM_XE_ENGINE_CLASS_COPY, "mismatch");
+_Static_assert(VCS == DRM_XE_ENGINE_CLASS_VIDEO_DECODE, "mismatch");
+_Static_assert(VECS == DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE, "mismatch");
+_Static_assert(CCS == DRM_XE_ENGINE_CLASS_COMPUTE, "mismatch");
+_Static_assert((int)RCS == (int)I915_ENGINE_CLASS_RENDER, "mismatch");
+_Static_assert((int)BCS == (int)I915_ENGINE_CLASS_COPY, "mismatch");
+_Static_assert((int)VCS == (int)I915_ENGINE_CLASS_VIDEO, "mismatch");
+_Static_assert((int)VECS == (int)I915_ENGINE_CLASS_VIDEO_ENHANCE, "mismatch");
+_Static_assert((int)CCS == (int)I915_ENGINE_CLASS_COMPUTE, "mismatch");
+
+static const char *intel_engine_class_string(uint16_t engine_class)
+{
+	switch (engine_class) {
+	case RCS:
+		return "RCS";
+	case BCS:
+		return "BCS";
+	case VCS:
+		return "VCS";
+	case VECS:
+		return "VECS";
+	case CCS:
+		return "CCS";
+	default:
+		igt_assert(0);
+	}
+}
+
 struct duration {
 	unsigned int min, max;
 	bool unbound;
@@ -126,14 +153,19 @@ struct w_arg {
 	bool sseu;
 };
 
+#define INVALID_ID ((uint16_t)-2)
+#define DEFAULT_ID ((uint16_t)-1)
+
+typedef struct drm_xe_engine_class_instance intel_engine_t;
+
 struct intel_engines {
 	unsigned int nr_engines;
-	enum intel_engine_id *engines;
+	intel_engine_t *engines;
 };
 
 struct bond {
-	uint64_t mask;
-	enum intel_engine_id master;
+	struct intel_engines mask;
+	intel_engine_t master;
 };
 
 struct work_buffer_size {
@@ -158,7 +190,7 @@ struct w_step {
 	/* Workload step metadata */
 	enum w_type type;
 	unsigned int context;
-	unsigned int engine;
+	intel_engine_t engine;
 	unsigned int engine_idx;
 	struct duration duration;
 	struct deps data_deps;
@@ -264,8 +296,8 @@ struct workload {
 	int sync_timeline;
 	uint32_t sync_seqno;
 
-	struct igt_list_head requests[NUM_ENGINES];
-	unsigned int nrequest[NUM_ENGINES];
+	struct igt_list_head *requests;
+	unsigned int *nrequest;
 };
 
 #define __for_each_ctx(__ctx, __wrk, __ctx_idx) \
@@ -293,16 +325,6 @@ static struct drm_i915_gem_context_param_sseu device_sseu = {
 #define FLAG_DEPSYNC		(1<<2)
 #define FLAG_SSEU		(1<<3)
 
-static const char *ring_str_map[NUM_ENGINES] = {
-	[DEFAULT] = "DEFAULT",
-	[RCS] = "RCS",
-	[BCS] = "BCS",
-	[VCS] = "VCS",
-	[VCS1] = "VCS1",
-	[VCS2] = "VCS2",
-	[VECS] = "VECS",
-};
-
 static void w_step_sync(struct w_step *w)
 {
 	if (is_xe)
@@ -518,41 +540,100 @@ out:
 	} \
 }
 
-static int str_to_engine(const char *str)
-{
-	unsigned int i;
+/* engine_class[<engine_instance>-<gt_id>] */
+static intel_engine_t str_to_engine(const char *str)
+{
+	intel_engine_t e = {INVALID_ID, DEFAULT_ID, DEFAULT_ID};
+	size_t pos;
+
+	if (!strcasecmp("DEFAULT", str)) {
+		e.engine_class = DEFAULT_ID;
+		return e;
+	} else if (!strncasecmp("RCS", str, 3)) {
+		e.engine_class = RCS;
+		pos = 3;
+	} else if (!strncasecmp("BCS", str, 3)) {
+		e.engine_class = BCS;
+		pos = 3;
+	} else if (!strncasecmp("VCS", str, 3)) {
+		e.engine_class = VCS;
+		pos = 3;
+	} else if (!strncasecmp("VECS", str, 4)) {
+		e.engine_class = VECS;
+		pos = 4;
+	} else if (!strncasecmp("CCS", str, 3)) {
+		e.engine_class = CCS;
+		pos = 3;
+	} else
+		return (intel_engine_t){INVALID_ID};
+
+	if (str[pos]) {
+		char *s = strchr(&str[pos], '-');
+		char *endptr = NULL;
+		long id;
+
+		if (!s || (s && *s != str[pos])) {
+			id = strtol(&str[pos], &endptr, 10);
+			if (endptr == &str[pos] || id < 1 || id >= INVALID_ID)
+				return (intel_engine_t){INVALID_ID};
+			e.engine_instance = id - 1;
+		}
 
-	for (i = 0; i < ARRAY_SIZE(ring_str_map); i++) {
-		if (!strcasecmp(str, ring_str_map[i]))
-			return i;
+		if (s && *(++s)) {
+			id = strtol(s, &endptr, 10);
+			if (endptr == s || id < 0 || id >= INVALID_ID)
+				return (intel_engine_t){INVALID_ID};
+			e.gt_id = id;
+		}
+
+		if (endptr && endptr != (str + strlen(str)))
+			return (intel_engine_t){INVALID_ID};
 	}
 
-	return -1;
+	return e;
+}
+
+static struct i915_engine_class_instance
+engine_to_i915_engine_class(const intel_engine_t *engine)
+{
+	return (struct i915_engine_class_instance){ engine->engine_class,
+						    engine->engine_instance };
 }
 
 static unsigned int
-engine_to_i915_legacy_ring(const enum intel_engine_id *engine)
-{
-	static const unsigned int eb_engine_map[NUM_ENGINES] = {
-		[DEFAULT] = I915_EXEC_DEFAULT,
-		[RCS] = I915_EXEC_RENDER,
-		[BCS] = I915_EXEC_BLT,
-		[VCS] = I915_EXEC_BSD,
-		[VCS1] = I915_EXEC_BSD | I915_EXEC_BSD_RING1,
-		[VCS2] = I915_EXEC_BSD | I915_EXEC_BSD_RING2,
-		[VECS] = I915_EXEC_VEBOX
+engine_to_i915_legacy_ring(const intel_engine_t *engine)
+{
+	switch (engine->engine_class) {
+	case DEFAULT_ID:
+		return I915_EXEC_DEFAULT;
+	case RCS:
+		return I915_EXEC_RENDER;
+	case BCS:
+		return I915_EXEC_BLT;
+	case VCS:
+		if (engine->engine_instance == DEFAULT_ID)
+			return I915_EXEC_BSD;
+		else if (engine->engine_instance == 0)
+			return I915_EXEC_BSD | I915_EXEC_BSD_RING1;
+		else if (engine->engine_instance == 1)
+			return I915_EXEC_BSD | I915_EXEC_BSD_RING2;
+		break;
+	case VECS:
+		return I915_EXEC_VEBOX;
 	};
 
-	return eb_engine_map[*engine];
+	igt_assert(0);
 }
 
-static bool are_equal_engines(const enum intel_engine_id *e1,
-			      const enum intel_engine_id *e2)
+static bool are_equal_engines(const intel_engine_t *e1,
+			      const intel_engine_t *e2)
 {
-	return *e1 == *e2;
+	return e1->engine_class == e2->engine_class &&
+	       e1->engine_instance == e2->engine_instance &&
+	       e1->gt_id == e2->gt_id;
 }
 
-static bool find_engine_in_map(const enum intel_engine_id *engine,
+static bool find_engine_in_map(const intel_engine_t *engine,
 			       struct intel_engines *engines, unsigned int *idx)
 {
 	igt_assert(idx);
@@ -565,208 +646,156 @@ static bool find_engine_in_map(const enum intel_engine_id *engine,
 	return false;
 }
 
-static struct intel_engine_data *query_engines(void)
+static struct intel_engines *query_engines(void)
 {
-	static struct intel_engine_data engines = {};
+	static struct intel_engines engines = {};
 
-	if (engines.nengines)
+	if (engines.nr_engines)
 		return &engines;
 
 	if (is_xe) {
 		struct drm_xe_engine_class_instance *hwe;
 
-		xe_for_each_engine(fd, hwe) {
-			engines.engines[engines.nengines].class = hwe->engine_class;
-			engines.engines[engines.nengines].instance = hwe->engine_instance;
-			engines.nengines++;
+		engines.engines = calloc(xe_number_engines(fd), sizeof(intel_engine_t));
+		igt_assert(engines.engines);
+		engines.nr_engines = 0;
+		xe_for_each_engine(fd, hwe)
+			engines.engines[engines.nr_engines++] = *hwe;
+		igt_assert(engines.nr_engines);
+	} else {
+		struct intel_engine_data ed = {};
+
+		ed = intel_engine_list_of_physical(fd);
+		igt_assert(ed.nengines);
+		engines.nr_engines = ed.nengines;
+		engines.engines = calloc(engines.nr_engines, sizeof(intel_engine_t));
+		igt_assert(engines.engines);
+		for (int i = 0; i < ed.nengines; ++i) {
+			engines.engines[i].engine_class = ed.engines[i].class;
+			engines.engines[i].engine_instance = ed.engines[i].instance;
+			engines.engines[i].gt_id = DEFAULT_ID;
 		}
-	} else
-		engines = intel_engine_list_of_physical(fd);
+	}
 
-	igt_assert(engines.nengines);
 	return &engines;
 }
 
-static unsigned int num_engines_in_class(enum intel_engine_id class)
+static bool is_valid_engine(const intel_engine_t *engine)
 {
-	const struct intel_engine_data *engines = query_engines();
-	unsigned int i, count = 0;
-
-	igt_assert(class == VCS);
-
-	for (i = 0; i < engines->nengines; i++) {
-		if (engines->engines[i].class == I915_ENGINE_CLASS_VIDEO)
-			count++;
-	}
-
-	igt_assert(count);
-	return count;
+	return engine->engine_class != INVALID_ID;
 }
 
-static void
-fill_engines_id_class(enum intel_engine_id *list,
-		      enum intel_engine_id class)
+static bool is_default_engine(const intel_engine_t *engine)
 {
-	const struct intel_engine_data *engines = query_engines();
-	enum intel_engine_id engine = VCS1;
-	unsigned int i, j = 0;
-
-	igt_assert(class == VCS);
-	igt_assert(num_engines_in_class(VCS) <= 2);
-
-	for (i = 0; i < engines->nengines; i++) {
-		if (engines->engines[i].class != I915_ENGINE_CLASS_VIDEO)
-			continue;
+	return engine->engine_class == DEFAULT_ID &&
+	       engine->engine_instance == DEFAULT_ID &&
+	       engine->gt_id == DEFAULT_ID;
+}
 
-		list[j++] = engine++;
-	}
+static bool engine_matches_filter(const intel_engine_t *engine, const intel_engine_t *filter)
+{
+	return (filter->engine_class == DEFAULT_ID ||
+		filter->engine_class == engine->engine_class) &&
+	       (filter->engine_instance == DEFAULT_ID ||
+		filter->engine_instance == engine->engine_instance) &&
+	       (filter->gt_id == DEFAULT_ID ||
+		filter->gt_id == engine->gt_id);
 }
 
+#define for_each_matching_engine(__engine, __filter, __engines) \
+	for (unsigned int __i = 0; __i < __engines->nr_engines && \
+	     (__engine = &__engines->engines[__i]); __i++) \
+		for_if(engine_matches_filter(__engine, __filter))
+
 static unsigned int
-find_physical_instance(enum intel_engine_id class, unsigned int logical)
+append_matching_engines(const intel_engine_t *filter, struct intel_engines *engines)
 {
-	const struct intel_engine_data *engines = query_engines();
-	unsigned int i, j = 0;
+	unsigned int prev_nr_engines;
+	struct intel_engines *all = query_engines();
+	intel_engine_t *engine;
 
-	igt_assert(class == VCS);
-
-	for (i = 0; i < engines->nengines; i++) {
-		if (engines->engines[i].class != I915_ENGINE_CLASS_VIDEO)
-			continue;
+	igt_assert(engines);
+	prev_nr_engines = engines->nr_engines;
 
-		/* Map logical to physical instances. */
-		if (logical == j++)
-			return engines->engines[i].instance;
+	for_each_matching_engine(engine, filter, all) {
+		engines->nr_engines++;
+		engines->engines = realloc(engines->engines,
+					   engines->nr_engines * sizeof(intel_engine_t));
+		igt_assert(engines->engines);
+		engines->engines[engines->nr_engines - 1] = *engine;
 	}
 
-	igt_assert(0);
-	return 0;
+	return engines->nr_engines - prev_nr_engines;
 }
 
-static struct i915_engine_class_instance
-get_engine(enum intel_engine_id engine)
+static intel_engine_t get_default_engine(void)
 {
-	struct i915_engine_class_instance ci;
+	struct intel_engines *all_engines = query_engines();
+	const intel_engine_t filters[] = {
+		{RCS, DEFAULT_ID, DEFAULT_ID},
+		{CCS, DEFAULT_ID, DEFAULT_ID},
+		{DEFAULT_ID, DEFAULT_ID, DEFAULT_ID},
+		{INVALID_ID}
+	}, *filter, *default_engine;
 
-	query_engines();
-
-	switch (engine) {
-	case RCS:
-		ci.engine_class = I915_ENGINE_CLASS_RENDER;
-		ci.engine_instance = 0;
-		break;
-	case BCS:
-		ci.engine_class = I915_ENGINE_CLASS_COPY;
-		ci.engine_instance = 0;
-		break;
-	case VCS1:
-	case VCS2:
-		ci.engine_class = I915_ENGINE_CLASS_VIDEO;
-		ci.engine_instance = find_physical_instance(VCS, engine - VCS1);
-		break;
-	case VECS:
-		ci.engine_class = I915_ENGINE_CLASS_VIDEO_ENHANCE;
-		ci.engine_instance = 0;
-		break;
-	default:
-		igt_assert(0);
-	};
+	for (filter = filters; is_valid_engine(filter); filter++)
+		for_each_matching_engine(default_engine, filter, all_engines)
+			return *default_engine;
 
-	return ci;
+	igt_assert(0);
 }
 
-static struct drm_xe_engine_class_instance
-xe_get_engine(enum intel_engine_id engine)
+static intel_engine_t resolve_to_physical_engine_(const intel_engine_t *engine)
 {
-	struct drm_xe_engine_class_instance hwe = {}, *hwe1;
-	bool found_physical = false;
+	struct intel_engines *all_engines = query_engines();
+	intel_engine_t *resolved;
 
-	switch (engine) {
-	case RCS:
-		hwe.engine_class = DRM_XE_ENGINE_CLASS_RENDER;
-		break;
-	case BCS:
-		hwe.engine_class = DRM_XE_ENGINE_CLASS_COPY;
-		break;
-	case VCS1:
-		hwe.engine_class = DRM_XE_ENGINE_CLASS_VIDEO_DECODE;
-		break;
-	case VCS2:
-		hwe.engine_class = DRM_XE_ENGINE_CLASS_VIDEO_DECODE;
-		hwe.engine_instance = 1;
-		break;
-	case VECS:
-		hwe.engine_class = DRM_XE_ENGINE_CLASS_VIDEO_ENHANCE;
-		break;
-	default:
-		igt_assert(0);
-	};
+	igt_assert(engine);
+	if (is_default_engine(engine))
+		return get_default_engine();
 
-	xe_for_each_engine(fd, hwe1) {
-		if (hwe.engine_class == hwe1->engine_class &&
-		    hwe.engine_instance  == hwe1->engine_instance) {
-			hwe = *hwe1;
-			found_physical = true;
-			break;
-		}
-	}
+	for_each_matching_engine(resolved, engine, all_engines)
+		return *resolved;
 
-	igt_assert(found_physical);
-	return hwe;
+	return (intel_engine_t){INVALID_ID};
 }
 
-static struct drm_xe_engine_class_instance
-xe_get_default_engine(void)
+static void resolve_to_physical_engine(intel_engine_t *engine)
 {
-	struct drm_xe_engine_class_instance default_hwe, *hwe;
-
-	/* select RCS0 | CCS0 or first available engine */
-	default_hwe = xe_engine(fd, 0)->instance;
-	xe_for_each_engine(fd, hwe) {
-		if ((hwe->engine_class == DRM_XE_ENGINE_CLASS_RENDER ||
-		     hwe->engine_class == DRM_XE_ENGINE_CLASS_COMPUTE) &&
-		    hwe->engine_instance == 0) {
-			default_hwe = *hwe;
-			break;
-		}
-	}
-
-	return default_hwe;
+	*engine = resolve_to_physical_engine_(engine);
+	igt_assert(is_valid_engine(engine));
 }
 
 static int parse_engine_map(struct w_step *step, const char *_str)
 {
 	char *token, *tctx = NULL, *tstart = (char *)_str;
+	intel_engine_t engine;
 
 	while ((token = strtok_r(tstart, "|", &tctx))) {
-		enum intel_engine_id engine;
-		unsigned int add;
-
 		tstart = NULL;
 
-		if (!strcmp(token, "DEFAULT"))
+		engine = str_to_engine(token);
+		if (!is_valid_engine(&engine) || is_default_engine(&engine))
 			return -1;
 
-		engine = str_to_engine(token);
-		if ((int)engine < 0)
+		if (!append_matching_engines(&engine, &step->engine_map))
 			return -1;
+	}
 
-		if (engine != VCS && engine != VCS1 && engine != VCS2 &&
-		    engine != RCS)
-			return -1; /* TODO */
+	return 0;
+}
 
-		add = engine == VCS ? num_engines_in_class(VCS) : 1;
-		step->engine_map.nr_engines += add;
-		step->engine_map.engines = realloc(step->engine_map.engines,
-						step->engine_map.nr_engines *
-						sizeof(step->engine_map.engines[0]));
+static int parse_bond_engines(struct w_step *step, const char *_str)
+{
+	char *token, *tctx = NULL, *tstart = (char *)_str;
+	intel_engine_t engine;
 
-		if (engine != VCS)
-			step->engine_map.engines[step->engine_map.nr_engines - add] = engine;
-		else
-			fill_engines_id_class(
-				&step->engine_map.engines[step->engine_map.nr_engines - add],
-				VCS);
+	while ((token = strtok_r(tstart, "|", &tctx))) {
+		tstart = NULL;
+
+		engine = str_to_engine(token);
+		if (append_matching_engines(&engine, &step->bond.mask) != 1)
+			return -1;
 	}
 
 	return 0;
@@ -888,26 +917,6 @@ static int parse_working_set(struct working_set *set, char *str)
 	return 0;
 }
 
-static uint64_t engine_list_mask(const char *_str)
-{
-	uint64_t mask = 0;
-
-	char *token, *tctx = NULL, *tstart = (char *)_str;
-
-	while ((token = strtok_r(tstart, "|", &tctx))) {
-		enum intel_engine_id engine = str_to_engine(token);
-
-		if ((int)engine < 0 || engine == DEFAULT || engine == VCS)
-			return 0;
-
-		mask |= 1 << engine;
-
-		tstart = NULL;
-	}
-
-	return mask;
-}
-
 static unsigned long
 allocate_working_set(struct workload *wrk, struct working_set *set);
 
@@ -1179,18 +1188,19 @@ parse_workload(struct w_arg *arg, unsigned int flags, double scale_dur,
 							  "Invalid context at step %u!\n",
 							  nr_steps);
 					} else if (nr == 1) {
-						step.bond.mask = engine_list_mask(field);
-						check_arg(step.bond.mask == 0,
-							"Invalid siblings list at step %u!\n",
-							nr_steps);
+						tmp = parse_bond_engines(&step, field);
+						check_arg(tmp < 0,
+							  "Invalid siblings list at step %u!\n",
+							  nr_steps);
 					} else if (nr == 2) {
-						tmp = str_to_engine(field);
-						check_arg(tmp <= 0 ||
-							  tmp == VCS ||
-							  tmp == DEFAULT,
+						struct intel_engines engines;
+
+						step.bond.master = str_to_engine(field);
+						check_arg(append_matching_engines(&step.bond.master,
+										  &engines) != 1,
 							  "Invalid master engine at step %u!\n",
 							  nr_steps);
-						step.bond.master = tmp;
+						free(engines.engines);
 					}
 
 					nr++;
@@ -1248,13 +1258,11 @@ parse_workload(struct w_arg *arg, unsigned int flags, double scale_dur,
 		if (field) {
 			fstart = NULL;
 
-			i = str_to_engine(field);
-			check_arg(i < 0,
+			step.engine = str_to_engine(field);
+			check_arg(!is_valid_engine(&step.engine),
 				  "Invalid engine id at step %u!\n", nr_steps);
 
 			valid++;
-
-			step.engine = i;
 		}
 
 		field = strtok_r(fstart, ".", &fctx);
@@ -1420,9 +1428,9 @@ add_step:
 static struct workload *
 clone_workload(struct workload *_wrk)
 {
+	int nr_engines = query_engines()->nr_engines;
 	struct workload *wrk;
 	struct w_step *w;
-	int i;
 
 	wrk = malloc(sizeof(*wrk));
 	igt_assert(wrk);
@@ -1457,8 +1465,12 @@ clone_workload(struct workload *_wrk)
 		}
 	}
 
-	for (i = 0; i < NUM_ENGINES; i++)
-		IGT_INIT_LIST_HEAD(&wrk->requests[i]);
+	wrk->requests = calloc(nr_engines, sizeof(*wrk->requests));
+	igt_assert(wrk->requests);
+	wrk->nrequest = calloc(nr_engines, sizeof(*wrk->nrequest));
+	igt_assert(wrk->nrequest);
+	while (--nr_engines >= 0)
+		IGT_INIT_LIST_HEAD(&wrk->requests[nr_engines]);
 
 	return wrk;
 }
@@ -1485,37 +1497,32 @@ __get_ctx(struct workload *wrk, const struct w_step *w)
 	return &wrk->ctx_list[w->context];
 }
 
-static uint32_t mmio_base(int i915, enum intel_engine_id engine, int gen)
+static uint32_t mmio_base(int i915, const intel_engine_t *engine, int gen)
 {
-	const char *name;
+	char name[16];
 
 	if (gen >= 11)
 		return 0;
 
-	switch (engine) {
-	case NUM_ENGINES:
+	switch (engine->engine_class) {
 	default:
 		return 0;
 
-	case DEFAULT:
+	case DEFAULT_ID:
 	case RCS:
-		name = "rcs0";
+		snprintf(name, sizeof(name), "rcs%u", engine->engine_instance);
 		break;
-
 	case BCS:
-		name = "bcs0";
+		snprintf(name, sizeof(name), "bcs%u", engine->engine_instance);
 		break;
-
 	case VCS:
-	case VCS1:
-		name = "vcs0";
-		break;
-	case VCS2:
-		name = "vcs1";
+		snprintf(name, sizeof(name), "vcs%u", engine->engine_instance);
 		break;
-
 	case VECS:
-		name = "vecs0";
+		snprintf(name, sizeof(name), "vecs%u", engine->engine_instance);
+		break;
+	case CCS:
+		snprintf(name, sizeof(name), "ccs%u", engine->engine_instance);
 		break;
 	}
 
@@ -1525,7 +1532,7 @@ static uint32_t mmio_base(int i915, enum intel_engine_id engine, int gen)
 static unsigned int create_bb(struct w_step *w, int self)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
-	const uint32_t base = mmio_base(fd, w->engine, gen);
+	const uint32_t base = mmio_base(fd, &w->engine, gen);
 #define CS_GPR(x) (base + 0x600 + 8 * (x))
 #define TIMESTAMP (base + 0x3a8)
 	const int use_64b = gen >= 8;
@@ -1890,22 +1897,6 @@ static void vm_destroy(int i915, uint32_t vm_id)
 	igt_assert_eq(__vm_destroy(i915, vm_id), 0);
 }
 
-static unsigned int
-find_engine(struct i915_engine_class_instance *ci, unsigned int count,
-	    enum intel_engine_id engine)
-{
-	struct i915_engine_class_instance e = get_engine(engine);
-	unsigned int i;
-
-	for (i = 0; i < count; i++, ci++) {
-		if (!memcmp(&e, ci, sizeof(*ci)))
-			return i;
-	}
-
-	igt_assert(0);
-	return 0;
-}
-
 static struct drm_i915_gem_context_param_sseu get_device_sseu(void)
 {
 	struct drm_i915_gem_context_param param = { };
@@ -2243,7 +2234,10 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 					else
 						igt_assert(ctx->load_balance);
 
-					w->request_idx = ctx->engine_map.engines[map_idx];
+					igt_assert(find_engine_in_map(
+						&ctx->engine_map.engines[map_idx],
+						query_engines(),
+						&w->request_idx));
 				}
 			}
 
@@ -2258,7 +2252,8 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 
 				for (j = 0; j < ctx->engine_map.nr_engines; j++)
 					load_balance->engines[j] =
-						get_engine(ctx->engine_map.engines[j]);
+						engine_to_i915_engine_class(
+							&ctx->engine_map.engines[j]);
 			}
 
 			/* Reserve slot for virtual engine. */
@@ -2269,32 +2264,30 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 
 			for (j = 1; j <= ctx->engine_map.nr_engines; j++)
 				set_engines->engines[j] =
-					get_engine(ctx->engine_map.engines[j - 1]);
+					engine_to_i915_engine_class(
+						&ctx->engine_map.engines[j - 1]);
 
 			last = NULL;
 			for (j = 0; j < ctx->bond_count; j++) {
-				unsigned long mask = ctx->bonds[j].mask;
+				struct intel_engines *mask = &ctx->bonds[j].mask;
 				struct i915_context_engines_bond *bond =
-					alloca0(sizeof_engines_bond(__builtin_popcount(mask)));
+					alloca0(sizeof_engines_bond(mask->nr_engines));
 				unsigned int b, e;
 
 				bond->base.next_extension = to_user_pointer(last);
 				bond->base.name = I915_CONTEXT_ENGINES_EXT_BOND;
 
 				bond->virtual_index = 0;
-				bond->master = get_engine(ctx->bonds[j].master);
+				bond->master = engine_to_i915_engine_class(&ctx->bonds[j].master);
 
-				for (b = 0, e = 0; mask; e++, mask >>= 1) {
+				for (b = 0, e = 0; e < mask->nr_engines; e++) {
 					unsigned int idx;
 
-					if (!(mask & 1))
-						continue;
+					igt_assert(find_engine_in_map(&mask->engines[e],
+								      &ctx->engine_map,
+								      &idx));
 
-					idx = find_engine(&set_engines->engines[1],
-							  ctx->engine_map.nr_engines,
-							  e);
-					bond->engines[b++] =
-						set_engines->engines[1 + idx];
+					bond->engines[b++] = set_engines->engines[1 + idx];
 				}
 
 				last = bond;
@@ -2309,7 +2302,11 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
 					continue;
 				if (w->type == BATCH) {
 					w->engine_idx = engine_to_i915_legacy_ring(&w->engine);
-					w->request_idx = w->engine;
+					resolve_to_physical_engine(&w->engine);
+					igt_assert(find_engine_in_map(
+							&w->engine,
+							query_engines(),
+							&w->request_idx));
 				}
 			}
 		}
@@ -2368,7 +2365,7 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 			eq->nr_hwes = ctx->engine_map.nr_engines;
 			eq->hwe_list = calloc(eq->nr_hwes, sizeof(*eq->hwe_list));
 			for (i = 0; i < eq->nr_hwes; ++i) {
-				eq->hwe_list[i] = xe_get_engine(ctx->engine_map.engines[i]);
+				eq->hwe_list[i] = ctx->engine_map.engines[i];
 
 				/* check no mixing classes and no duplicates */
 				for (int j = 0; j < i; ++j) {
@@ -2390,65 +2387,67 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
 				}
 
 				if (verbose > 3)
-					printf("%u ctx[%d] %s [%u:%u:%u]\n",
-						id, ctx_idx,
-						ring_str_map[ctx->engine_map.engines[i]],
-						eq->hwe_list[i].engine_class,
-						eq->hwe_list[i].engine_instance,
-						eq->hwe_list[i].gt_id);
+					printf("%u ctx[%d] %s [%u:%u:%u]\n", id,
+					       ctx_idx,
+					       intel_engine_class_string(
+						       ctx->engine_map.engines[i].engine_class),
+					       eq->hwe_list[i].engine_class,
+					       eq->hwe_list[i].engine_instance,
+					       eq->hwe_list[i].gt_id);
 			}
 
-			/* update engine_idx */
+			xe_exec_queue_create_(ctx, eq);
+		} else {
+			/* create engine_map, update engine_idx */
 			for_each_w_step(w, wrk) {
 				if (w->context != ctx_idx)
 					continue;
 				if (w->type == BATCH) {
-					w->engine_idx = 0;
-					w->request_idx = ctx->engine_map.engines[w->engine_idx];
+					resolve_to_physical_engine(&w->engine);
+					if (!find_engine_in_map(&w->engine, &ctx->engine_map,
+								&w->engine_idx)) {
+						igt_assert_eq(1, append_matching_engines(&w->engine,
+										&ctx->engine_map));
+						w->engine_idx = ctx->engine_map.nr_engines - 1;
+					}
 				}
 			}
 
-			xe_exec_queue_create_(ctx, eq);
-		} else {
-			int engine_classes[NUM_ENGINES] = {};
+			/* skip not referenced context */
+			if (!ctx->engine_map.nr_engines)
+				continue;
 
-			ctx->xe.nr_queues = NUM_ENGINES;
+			ctx->xe.nr_queues = ctx->engine_map.nr_engines;
 			ctx->xe.queue_list = calloc(ctx->xe.nr_queues, sizeof(*ctx->xe.queue_list));
 
-			for_each_w_step(w, wrk) {
-				if (w->context != ctx_idx)
-					continue;
-				if (w->type == BATCH) {
-					engine_classes[w->engine]++;
-					/* update engine_idx and request_idx */
-					w->engine_idx = w->engine;
-					w->request_idx = w->engine;
-				}
-			}
-
-			for (i = 0; i < NUM_ENGINES; i++) {
-				if (engine_classes[i]) {
-					eq = &ctx->xe.queue_list[i];
-					eq->nr_hwes = 1;
-					eq->hwe_list = calloc(1, sizeof(*eq->hwe_list));
+			for (i = 0; i < ctx->xe.nr_queues; i++) {
+				eq = &ctx->xe.queue_list[i];
+				eq->nr_hwes = 1;
+				eq->hwe_list = calloc(1, sizeof(*eq->hwe_list));
+				eq->hwe_list[0] = ctx->engine_map.engines[i];
 
-					if (i == DEFAULT)
-						eq->hwe_list[0] = xe_get_default_engine();
-					else if (i == VCS)
-						eq->hwe_list[0] = xe_get_engine(VCS1);
-					else
-						eq->hwe_list[0] = xe_get_engine(i);
+				if (verbose > 3)
+					printf("%u ctx[%d] %s [%d:%d:%d]\n",
+						id, ctx_idx,
+						intel_engine_class_string(
+							ctx->engine_map.engines[i].engine_class),
+						eq->hwe_list[0].engine_class,
+						eq->hwe_list[0].engine_instance,
+						eq->hwe_list[0].gt_id);
 
-					if (verbose > 3)
-						printf("%u ctx[%d] %s [%u:%u:%u]\n",
-							id, ctx_idx, ring_str_map[i],
-							eq->hwe_list[0].engine_class,
-							eq->hwe_list[0].engine_instance,
-							eq->hwe_list[0].gt_id);
+				xe_exec_queue_create_(ctx, eq);
+			}
+		}
 
-					xe_exec_queue_create_(ctx, eq);
-				}
-				engine_classes[i] = 0;
+		/* update request_idx */
+		for_each_w_step(w, wrk) {
+			if (w->context != ctx_idx)
+				continue;
+			if (w->type == BATCH) {
+				igt_assert(find_engine_in_map(
+						&ctx->engine_map.engines[w->engine_idx],
+						query_engines(),
+						&w->request_idx));
 			}
 		}
 	}
@@ -2911,7 +2910,7 @@ static void *run_workload(void *data)
 		}
 	}
 
-	for (int i = 0; i < NUM_ENGINES; i++) {
+	for (int i = query_engines()->nr_engines; --i >= 0;) {
 		if (!wrk->nrequest[i])
 			continue;
 
-- 
2.31.1

[PATCH v2 i-g-t 6/6] benchmarks/gem_wsim: Option to list physical engines

[Marcin Bernatowicz]

Added command line option (-l) to list physical engines.

Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
---
 benchmarks/gem_wsim.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index effe8a479..ae4e507ce 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -2987,6 +2987,7 @@ static void print_help(void)
 "  -f <scale>        Scale factor for batch durations.\n"
 "  -F <scale>        Scale factor for delays.\n"
 "  -L                List GPUs.\n"
+"  -l                List physical engines.\n"
 "  -D <gpu>          One of the GPUs from -L.\n"
 	);
 }
@@ -3046,10 +3047,42 @@ add_workload_arg(struct w_arg *w_args, unsigned int nr_args, char *w_arg,
 	return w_args;
 }
 
+static void list_engines(void)
+{
+	struct intel_engines *engines = query_engines();
+	int engine_class_count[NUM_ENGINE_CLASSES] = {};
+	unsigned int i;
+
+	for (i = 0; i < engines->nr_engines; ++i) {
+		igt_assert_lt(engines->engines[i].engine_class, NUM_ENGINE_CLASSES);
+		engine_class_count[engines->engines[i].engine_class]++;
+	}
+
+	for (i = 0; i < engines->nr_engines; ++i) {
+		if (engine_class_count[engines->engines[i].engine_class] > 1)
+			printf("%s%u",
+			       intel_engine_class_string(engines->engines[i].engine_class),
+			       engines->engines[i].engine_instance + 1);
+		else
+			printf("%s",
+			       intel_engine_class_string(engines->engines[i].engine_class));
+
+		if (is_xe && engines->engines[i].gt_id)
+			printf("-%u", engines->engines[i].gt_id);
+
+		if (verbose > 3)
+			printf(" [%d:%d:%d]", engines->engines[i].engine_class,
+			       engines->engines[i].engine_instance,
+			       engines->engines[i].gt_id);
+		printf("\n");
+	}
+}
+
 int main(int argc, char **argv)
 {
 	struct igt_device_card card = { };
 	bool list_devices_arg = false;
+	bool list_engines_arg = false;
 	unsigned int repeat = 1;
 	unsigned int clients = 1;
 	unsigned int flags = 0;
@@ -3072,11 +3105,14 @@ int main(int argc, char **argv)
 	master_prng = time(NULL);
 
 	while ((c = getopt(argc, argv,
-			   "LhqvsSdc:r:w:W:a:p:I:f:F:D:")) != -1) {
+			   "LlhqvsSdc:r:w:W:a:p:I:f:F:D:")) != -1) {
 		switch (c) {
 		case 'L':
 			list_devices_arg = true;
 			break;
+		case 'l':
+			list_engines_arg = true;
+			break;
 		case 'D':
 			device_arg = strdup(optarg);
 			break;
@@ -3197,6 +3233,11 @@ int main(int argc, char **argv)
 	if (is_xe)
 		xe_device_get(fd);
 
+	if (list_engines_arg) {
+		list_engines();
+		goto out;
+	}
+
 	if (!nr_w_args) {
 		wsim_err("No workload descriptor(s)!\n");
 		goto err;
-- 
2.31.1

Re: [PATCH v2 i-g-t 1/6] benchmarks/gem_wsim: Introduce intel_engines structure

[Kamil Konieczny]

Hi Marcin,
On 2024-04-23 at 10:56:41 +0200, Marcin Bernatowicz wrote:
> Introduction of struct intel_engines, which encapsulates the number of
> engines (nr_engines) and a pointer to an array of engine IDs (engines).
> This structural refactor replaces the previous ad-hoc approach of managing
> engine maps across various structures (w_step, ctx, etc.)
> This change is part of a series of preparatory steps for upcoming
> patches.

+Cc: Tvrtko Ursulin <tursulin@igalia.com>

Please rebase your patchseries and send again, overall it looks
good so

Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>

See also small nit below.

> 
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> ---
>  benchmarks/gem_wsim.c | 72 ++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index fce57b894..e98624221 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -126,6 +126,11 @@ struct w_arg {
>  	bool sseu;
>  };
>  
> +struct intel_engines {
> +	unsigned int nr_engines;
> +	enum intel_engine_id *engines;
> +};
> +
>  struct bond {
>  	uint64_t mask;
>  	enum intel_engine_id master;
> @@ -165,10 +170,7 @@ struct w_step {
>  		int target;
>  		int throttle;
>  		int priority;
> -		struct {
> -			unsigned int engine_map_count;
> -			enum intel_engine_id *engine_map;
> -		};
> +		struct intel_engines engine_map;
>  		bool load_balance;
>  		struct {
>  			uint64_t bond_mask;
> @@ -220,8 +222,7 @@ struct xe_exec_queue {
>  struct ctx {
>  	uint32_t id;
>  	int priority;
> -	unsigned int engine_map_count;
> -	enum intel_engine_id *engine_map;
> +	struct intel_engines engine_map;
>  	unsigned int bond_count;
>  	struct bond *bonds;
>  	bool load_balance;
> @@ -722,15 +723,17 @@ static int parse_engine_map(struct w_step *step, const char *_str)
>  			return -1; /* TODO */
>  
>  		add = engine == VCS ? num_engines_in_class(VCS) : 1;
> -		step->engine_map_count += add;
> -		step->engine_map = realloc(step->engine_map,
> -					   step->engine_map_count *
> -					   sizeof(step->engine_map[0]));
> +		step->engine_map.nr_engines += add;
> +		step->engine_map.engines = realloc(step->engine_map.engines,
> +						step->engine_map.nr_engines *
> +						sizeof(step->engine_map.engines[0]));
>  
>  		if (engine != VCS)
> -			step->engine_map[step->engine_map_count - add] = engine;
> +			step->engine_map.engines[step->engine_map.nr_engines - add] = engine;
>  		else
> -			fill_engines_id_class(&step->engine_map[step->engine_map_count - add], VCS);
> +			fill_engines_id_class(
> +				&step->engine_map.engines[step->engine_map.nr_engines - add],
> +				VCS);
>  	}
>  
>  	return 0;
> @@ -1608,8 +1611,8 @@ find_engine_in_map(struct ctx *ctx, enum intel_engine_id engine)
>  {
>  	unsigned int i;
>  
> -	for (i = 0; i < ctx->engine_map_count; i++) {
> -		if (ctx->engine_map[i] == engine)
> +	for (i = 0; i < ctx->engine_map.nr_engines; i++) {
> +		if (ctx->engine_map.engines[i] == engine)
>  			return i + 1;
>  	}
>  
> @@ -1623,7 +1626,7 @@ eb_update_flags(struct workload *wrk, struct w_step *w,
>  {
>  	struct ctx *ctx = __get_ctx(wrk, w);
>  
> -	if (ctx->engine_map)
> +	if (ctx->engine_map.nr_engines)
>  		w->i915.eb.flags = find_engine_in_map(ctx, engine);
>  	else
>  		eb_set_engine(&w->i915.eb, engine);
> @@ -1648,7 +1651,7 @@ xe_get_eq(struct workload *wrk, const struct w_step *w)
>  	struct ctx *ctx = __get_ctx(wrk, w);
>  	struct xe_exec_queue *eq;
>  
> -	if (ctx->engine_map) {
> +	if (ctx->engine_map.nr_engines) {
>  		igt_assert_eq(ctx->xe.nr_queues, 1);
>  		igt_assert(ctx->xe.queue_list[0].id);
>  		eq = &ctx->xe.queue_list[0];
> @@ -1941,7 +1944,7 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask)
>  	if (slice_mask == -1)
>  		slice_mask = device_sseu.slice_mask;
>  
> -	if (ctx->engine_map && ctx->load_balance) {
> +	if (ctx->engine_map.nr_engines && ctx->load_balance) {
>  		sseu.flags = I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX;
>  		sseu.engine.engine_class = I915_ENGINE_CLASS_INVALID;
>  		sseu.engine.engine_instance = 0;
> @@ -2151,9 +2154,8 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>  
>  			if (w->type == ENGINE_MAP) {
>  				ctx->engine_map = w->engine_map;
> -				ctx->engine_map_count = w->engine_map_count;
>  			} else if (w->type == LOAD_BALANCE) {
> -				if (!ctx->engine_map) {
> +				if (!ctx->engine_map.nr_engines) {
>  					wsim_err("Load balancing needs an engine map!\n");
>  					return 1;
>  				}
> @@ -2232,15 +2234,15 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>  
>  		__configure_context(ctx_id, wrk->prio);
>  
> -		if (ctx->engine_map) {
> +		if (ctx->engine_map.nr_engines) {
>  			struct i915_context_param_engines *set_engines =
> -				alloca0(sizeof_param_engines(ctx->engine_map_count + 1));
> +				alloca0(sizeof_param_engines(ctx->engine_map.nr_engines + 1));
>  			struct i915_context_engines_load_balance *load_balance =
> -				alloca0(sizeof_load_balance(ctx->engine_map_count));
> +				alloca0(sizeof_load_balance(ctx->engine_map.nr_engines));
>  			struct drm_i915_gem_context_param param = {
>  				.ctx_id = ctx_id,
>  				.param = I915_CONTEXT_PARAM_ENGINES,
> -				.size = sizeof_param_engines(ctx->engine_map_count + 1),
> +				.size = sizeof_param_engines(ctx->engine_map.nr_engines + 1),
>  				.value = to_user_pointer(set_engines),
>  			};
>  			struct i915_context_engines_bond *last = NULL;
> @@ -2252,11 +2254,11 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>  				load_balance->base.name =
>  					I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE;
>  				load_balance->num_siblings =
> -					ctx->engine_map_count;
> +					ctx->engine_map.nr_engines;
>  
> -				for (j = 0; j < ctx->engine_map_count; j++)
> +				for (j = 0; j < ctx->engine_map.nr_engines; j++)
>  					load_balance->engines[j] =
> -						get_engine(ctx->engine_map[j]);
> +						get_engine(ctx->engine_map.engines[j]);

Why not:
	get_engine(ctx, j)
or
	get_engine_from_map(ctx, j)

Regards,
Kamil

>  			}
>  
>  			/* Reserve slot for virtual engine. */
> @@ -2265,9 +2267,9 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>  			set_engines->engines[0].engine_instance =
>  				I915_ENGINE_CLASS_INVALID_NONE;
>  
> -			for (j = 1; j <= ctx->engine_map_count; j++)
> +			for (j = 1; j <= ctx->engine_map.nr_engines; j++)
>  				set_engines->engines[j] =
> -					get_engine(ctx->engine_map[j - 1]);
> +					get_engine(ctx->engine_map.engines[j - 1]);
>  
>  			last = NULL;
>  			for (j = 0; j < ctx->bond_count; j++) {
> @@ -2289,7 +2291,7 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>  						continue;
>  
>  					idx = find_engine(&set_engines->engines[1],
> -							  ctx->engine_map_count,
> +							  ctx->engine_map.nr_engines,
>  							  e);
>  					bond->engines[b++] =
>  						set_engines->engines[1 + idx];
> @@ -2338,9 +2340,8 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
>  				continue;
>  			if (w->type == ENGINE_MAP) {
>  				ctx->engine_map = w->engine_map;
> -				ctx->engine_map_count = w->engine_map_count;
>  			} else if (w->type == LOAD_BALANCE) {
> -				if (!ctx->engine_map) {
> +				if (!ctx->engine_map.nr_engines) {
>  					wsim_err("Load balancing needs an engine map!\n");
>  					return 1;
>  				}
> @@ -2349,15 +2350,15 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
>  		}
>  
>  		/* create exec queue for each referenced engine */
> -		if (ctx->engine_map) {
> +		if (ctx->engine_map.nr_engines) {
>  			ctx->xe.nr_queues = 1;
>  			ctx->xe.queue_list = calloc(ctx->xe.nr_queues, sizeof(*ctx->xe.queue_list));
>  			igt_assert(ctx->xe.queue_list);
>  			eq = &ctx->xe.queue_list[ctx->xe.nr_queues - 1];
> -			eq->nr_hwes = ctx->engine_map_count;
> +			eq->nr_hwes = ctx->engine_map.nr_engines;
>  			eq->hwe_list = calloc(eq->nr_hwes, sizeof(*eq->hwe_list));
>  			for (i = 0; i < eq->nr_hwes; ++i) {
> -				eq->hwe_list[i] = xe_get_engine(ctx->engine_map[i]);
> +				eq->hwe_list[i] = xe_get_engine(ctx->engine_map.engines[i]);
>  
>  				/* check no mixing classes and no duplicates */
>  				for (int j = 0; j < i; ++j) {
> @@ -2380,7 +2381,8 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
>  
>  				if (verbose > 3)
>  					printf("%u ctx[%d] %s [%u:%u:%u]\n",
> -						id, ctx_idx, ring_str_map[ctx->engine_map[i]],
> +						id, ctx_idx,
> +						ring_str_map[ctx->engine_map.engines[i]],
>  						eq->hwe_list[i].engine_class,
>  						eq->hwe_list[i].engine_instance,
>  						eq->hwe_list[i].gt_id);
> -- 
> 2.31.1
> 

Re: [PATCH v2 i-g-t 1/6] benchmarks/gem_wsim: Introduce intel_engines structure

[Bernatowicz, Marcin]

Hi,

On 7/23/2024 1:01 PM, Kamil Konieczny wrote:
> Hi Marcin,
> On 2024-04-23 at 10:56:41 +0200, Marcin Bernatowicz wrote:
>> Introduction of struct intel_engines, which encapsulates the number of
>> engines (nr_engines) and a pointer to an array of engine IDs (engines).
>> This structural refactor replaces the previous ad-hoc approach of managing
>> engine maps across various structures (w_step, ctx, etc.)
>> This change is part of a series of preparatory steps for upcoming
>> patches.
> 
> +Cc: Tvrtko Ursulin <tursulin@igalia.com>
> 
> Please rebase your patchseries and send again, overall it looks
> good so

Done, send v3 with additional indentation corrections.

> 
> Acked-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> 
> See also small nit below.
> 
>>
>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>> ---
>>   benchmarks/gem_wsim.c | 72 ++++++++++++++++++++++---------------------
>>   1 file changed, 37 insertions(+), 35 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index fce57b894..e98624221 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -126,6 +126,11 @@ struct w_arg {
>>   	bool sseu;
>>   };
>>   
>> +struct intel_engines {
>> +	unsigned int nr_engines;
>> +	enum intel_engine_id *engines;
>> +};
>> +
>>   struct bond {
>>   	uint64_t mask;
>>   	enum intel_engine_id master;
>> @@ -165,10 +170,7 @@ struct w_step {
>>   		int target;
>>   		int throttle;
>>   		int priority;
>> -		struct {
>> -			unsigned int engine_map_count;
>> -			enum intel_engine_id *engine_map;
>> -		};
>> +		struct intel_engines engine_map;
>>   		bool load_balance;
>>   		struct {
>>   			uint64_t bond_mask;
>> @@ -220,8 +222,7 @@ struct xe_exec_queue {
>>   struct ctx {
>>   	uint32_t id;
>>   	int priority;
>> -	unsigned int engine_map_count;
>> -	enum intel_engine_id *engine_map;
>> +	struct intel_engines engine_map;
>>   	unsigned int bond_count;
>>   	struct bond *bonds;
>>   	bool load_balance;
>> @@ -722,15 +723,17 @@ static int parse_engine_map(struct w_step *step, const char *_str)
>>   			return -1; /* TODO */
>>   
>>   		add = engine == VCS ? num_engines_in_class(VCS) : 1;
>> -		step->engine_map_count += add;
>> -		step->engine_map = realloc(step->engine_map,
>> -					   step->engine_map_count *
>> -					   sizeof(step->engine_map[0]));
>> +		step->engine_map.nr_engines += add;
>> +		step->engine_map.engines = realloc(step->engine_map.engines,
>> +						step->engine_map.nr_engines *
>> +						sizeof(step->engine_map.engines[0]));
>>   
>>   		if (engine != VCS)
>> -			step->engine_map[step->engine_map_count - add] = engine;
>> +			step->engine_map.engines[step->engine_map.nr_engines - add] = engine;
>>   		else
>> -			fill_engines_id_class(&step->engine_map[step->engine_map_count - add], VCS);
>> +			fill_engines_id_class(
>> +				&step->engine_map.engines[step->engine_map.nr_engines - add],
>> +				VCS);
>>   	}
>>   
>>   	return 0;
>> @@ -1608,8 +1611,8 @@ find_engine_in_map(struct ctx *ctx, enum intel_engine_id engine)
>>   {
>>   	unsigned int i;
>>   
>> -	for (i = 0; i < ctx->engine_map_count; i++) {
>> -		if (ctx->engine_map[i] == engine)
>> +	for (i = 0; i < ctx->engine_map.nr_engines; i++) {
>> +		if (ctx->engine_map.engines[i] == engine)
>>   			return i + 1;
>>   	}
>>   
>> @@ -1623,7 +1626,7 @@ eb_update_flags(struct workload *wrk, struct w_step *w,
>>   {
>>   	struct ctx *ctx = __get_ctx(wrk, w);
>>   
>> -	if (ctx->engine_map)
>> +	if (ctx->engine_map.nr_engines)
>>   		w->i915.eb.flags = find_engine_in_map(ctx, engine);
>>   	else
>>   		eb_set_engine(&w->i915.eb, engine);
>> @@ -1648,7 +1651,7 @@ xe_get_eq(struct workload *wrk, const struct w_step *w)
>>   	struct ctx *ctx = __get_ctx(wrk, w);
>>   	struct xe_exec_queue *eq;
>>   
>> -	if (ctx->engine_map) {
>> +	if (ctx->engine_map.nr_engines) {
>>   		igt_assert_eq(ctx->xe.nr_queues, 1);
>>   		igt_assert(ctx->xe.queue_list[0].id);
>>   		eq = &ctx->xe.queue_list[0];
>> @@ -1941,7 +1944,7 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask)
>>   	if (slice_mask == -1)
>>   		slice_mask = device_sseu.slice_mask;
>>   
>> -	if (ctx->engine_map && ctx->load_balance) {
>> +	if (ctx->engine_map.nr_engines && ctx->load_balance) {
>>   		sseu.flags = I915_CONTEXT_SSEU_FLAG_ENGINE_INDEX;
>>   		sseu.engine.engine_class = I915_ENGINE_CLASS_INVALID;
>>   		sseu.engine.engine_instance = 0;
>> @@ -2151,9 +2154,8 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>>   
>>   			if (w->type == ENGINE_MAP) {
>>   				ctx->engine_map = w->engine_map;
>> -				ctx->engine_map_count = w->engine_map_count;
>>   			} else if (w->type == LOAD_BALANCE) {
>> -				if (!ctx->engine_map) {
>> +				if (!ctx->engine_map.nr_engines) {
>>   					wsim_err("Load balancing needs an engine map!\n");
>>   					return 1;
>>   				}
>> @@ -2232,15 +2234,15 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>>   
>>   		__configure_context(ctx_id, wrk->prio);
>>   
>> -		if (ctx->engine_map) {
>> +		if (ctx->engine_map.nr_engines) {
>>   			struct i915_context_param_engines *set_engines =
>> -				alloca0(sizeof_param_engines(ctx->engine_map_count + 1));
>> +				alloca0(sizeof_param_engines(ctx->engine_map.nr_engines + 1));
>>   			struct i915_context_engines_load_balance *load_balance =
>> -				alloca0(sizeof_load_balance(ctx->engine_map_count));
>> +				alloca0(sizeof_load_balance(ctx->engine_map.nr_engines));
>>   			struct drm_i915_gem_context_param param = {
>>   				.ctx_id = ctx_id,
>>   				.param = I915_CONTEXT_PARAM_ENGINES,
>> -				.size = sizeof_param_engines(ctx->engine_map_count + 1),
>> +				.size = sizeof_param_engines(ctx->engine_map.nr_engines + 1),
>>   				.value = to_user_pointer(set_engines),
>>   			};
>>   			struct i915_context_engines_bond *last = NULL;
>> @@ -2252,11 +2254,11 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>>   				load_balance->base.name =
>>   					I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE;
>>   				load_balance->num_siblings =
>> -					ctx->engine_map_count;
>> +					ctx->engine_map.nr_engines;
>>   
>> -				for (j = 0; j < ctx->engine_map_count; j++)
>> +				for (j = 0; j < ctx->engine_map.nr_engines; j++)
>>   					load_balance->engines[j] =
>> -						get_engine(ctx->engine_map[j]);
>> +						get_engine(ctx->engine_map.engines[j]);
> 
> Why not:
> 	get_engine(ctx, j)
> or
> 	get_engine_from_map(ctx, j)

get_engine does not exist after the whole patch series is applied.

--
marcin
> 
> Regards,
> Kamil
> 
>>   			}
>>   
>>   			/* Reserve slot for virtual engine. */
>> @@ -2265,9 +2267,9 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>>   			set_engines->engines[0].engine_instance =
>>   				I915_ENGINE_CLASS_INVALID_NONE;
>>   
>> -			for (j = 1; j <= ctx->engine_map_count; j++)
>> +			for (j = 1; j <= ctx->engine_map.nr_engines; j++)
>>   				set_engines->engines[j] =
>> -					get_engine(ctx->engine_map[j - 1]);
>> +					get_engine(ctx->engine_map.engines[j - 1]);
>>   
>>   			last = NULL;
>>   			for (j = 0; j < ctx->bond_count; j++) {
>> @@ -2289,7 +2291,7 @@ static int prepare_contexts(unsigned int id, struct workload *wrk)
>>   						continue;
>>   
>>   					idx = find_engine(&set_engines->engines[1],
>> -							  ctx->engine_map_count,
>> +							  ctx->engine_map.nr_engines,
>>   							  e);
>>   					bond->engines[b++] =
>>   						set_engines->engines[1 + idx];
>> @@ -2338,9 +2340,8 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
>>   				continue;
>>   			if (w->type == ENGINE_MAP) {
>>   				ctx->engine_map = w->engine_map;
>> -				ctx->engine_map_count = w->engine_map_count;
>>   			} else if (w->type == LOAD_BALANCE) {
>> -				if (!ctx->engine_map) {
>> +				if (!ctx->engine_map.nr_engines) {
>>   					wsim_err("Load balancing needs an engine map!\n");
>>   					return 1;
>>   				}
>> @@ -2349,15 +2350,15 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
>>   		}
>>   
>>   		/* create exec queue for each referenced engine */
>> -		if (ctx->engine_map) {
>> +		if (ctx->engine_map.nr_engines) {
>>   			ctx->xe.nr_queues = 1;
>>   			ctx->xe.queue_list = calloc(ctx->xe.nr_queues, sizeof(*ctx->xe.queue_list));
>>   			igt_assert(ctx->xe.queue_list);
>>   			eq = &ctx->xe.queue_list[ctx->xe.nr_queues - 1];
>> -			eq->nr_hwes = ctx->engine_map_count;
>> +			eq->nr_hwes = ctx->engine_map.nr_engines;
>>   			eq->hwe_list = calloc(eq->nr_hwes, sizeof(*eq->hwe_list));
>>   			for (i = 0; i < eq->nr_hwes; ++i) {
>> -				eq->hwe_list[i] = xe_get_engine(ctx->engine_map[i]);
>> +				eq->hwe_list[i] = xe_get_engine(ctx->engine_map.engines[i]);
>>   
>>   				/* check no mixing classes and no duplicates */
>>   				for (int j = 0; j < i; ++j) {
>> @@ -2380,7 +2381,8 @@ static int xe_prepare_contexts(unsigned int id, struct workload *wrk)
>>   
>>   				if (verbose > 3)
>>   					printf("%u ctx[%d] %s [%u:%u:%u]\n",
>> -						id, ctx_idx, ring_str_map[ctx->engine_map[i]],
>> +						id, ctx_idx,
>> +						ring_str_map[ctx->engine_map.engines[i]],
>>   						eq->hwe_list[i].engine_class,
>>   						eq->hwe_list[i].engine_instance,
>>   						eq->hwe_list[i].gt_id);
>> -- 
>> 2.31.1
>>