On 17/05/2019 20:35, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-05-17 12:25:13)From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Support new i915 uAPI for configuring contexts with engine maps. Please refer to the README file for more detailed explanation. v2: * Allow defining engine maps by class. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> --- benchmarks/gem_wsim.c | 211 +++++++++++++++++++++++++++++++++++------ benchmarks/wsim/README | 25 ++++- 2 files changed, 204 insertions(+), 32 deletions(-) diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c index 60b7d32e22d4..e5b12e37490e 100644 --- a/benchmarks/gem_wsim.c +++ b/benchmarks/gem_wsim.c @@ -57,6 +57,7 @@ #include "ewma.h"enum intel_engine_id {+ DEFAULT, RCS, BCS, VCS, @@ -81,7 +82,8 @@ enum w_type SW_FENCE, SW_FENCE_SIGNAL, CTX_PRIORITY, - PREEMPTION + PREEMPTION, + ENGINE_MAP };struct deps@@ -115,6 +117,10 @@ struct w_step int throttle; int fence_signal; int priority; + struct { + unsigned int engine_map_count; + enum intel_engine_id *engine_map; + }; };/* Implementation details */@@ -142,6 +148,8 @@ DECLARE_EWMA(uint64_t, rt, 4, 2) struct ctx { uint32_t id; int priority; + unsigned int engine_map_count; + enum intel_engine_id *engine_map; bool targets_instance; bool wants_balance; unsigned int static_vcs; @@ -200,10 +208,10 @@ struct workload int fd; bool first; unsigned int num_engines; - unsigned int engine_map[5]; + unsigned int engine_map[NUM_ENGINES]; uint64_t t_prev; - uint64_t prev[5]; - double busy[5]; + uint64_t prev[NUM_ENGINES]; + double busy[NUM_ENGINES]; } busy_balancer; };@@ -234,6 +242,7 @@ static int fd;#define REG(x) (volatile uint32_t *)((volatile char *)igt_global_mmio + x)static const char *ring_str_map[NUM_ENGINES] = {+ [DEFAULT] = "DEFAULT", [RCS] = "RCS", [BCS] = "BCS", [VCS] = "VCS", @@ -330,6 +339,43 @@ static int str_to_engine(const char *str) return -1; }+static int parse_engine_map(struct w_step *step, const char *_str)+{ + char *token, *tctx = NULL, *tstart = (char *)_str; + + while ((token = strtok_r(tstart, "|", &tctx))) { + enum intel_engine_id engine; + unsigned int add; + + tstart = NULL; + + if (!strcmp(token, "DEFAULT")) + return -1; + + engine = str_to_engine(token); + if ((int)engine < 0) + return -1; + + if (engine != VCS && engine != VCS1 && engine != VCS2) + return -1; /* TODO */Still a little concerned that the map is VCS only. It just doesn't fit my expectations of what the map will be.
I think I could update this now that load_balance takes a list.
+ + add = engine == VCS ? 2 : 1;Will we not every ask what happens if we had millions of engines at our disposal. But that's a tommorrow problem, ok.
This is improved in a later patch. It felt easier to generalize at the end in this instance instead of trying to rebase the whole series.
+ step->engine_map_count += add; + step->engine_map = realloc(step->engine_map, + step->engine_map_count * + sizeof(step->engine_map[0])); + + if (engine != VCS) { + step->engine_map[step->engine_map_count - 1] = engine; + } else { + step->engine_map[step->engine_map_count - 2] = VCS1; + step->engine_map[step->engine_map_count - 1] = VCS2; + } + } + + return 0; +} + static struct workload * parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w) { @@ -448,6 +494,33 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w) } else if (!strcmp(field, "f")) { step.type = SW_FENCE; goto add_step; + } else if (!strcmp(field, "M")) { + unsigned int nr = 0; + while ((field = strtok_r(fstart, ".", &fctx)) != + NULL) { + tmp = atoi(field); + check_arg(nr == 0 && tmp <= 0, + "Invalid context at step %u!\n", + nr_steps); + check_arg(nr > 1, + "Invalid engine map format at step %u!\n", + nr_steps); + + if (nr == 0) { + step.context = tmp; + } else { + tmp = parse_engine_map(&step, + field); + check_arg(tmp < 0, + "Invalid engine map list at step %u!\n", + nr_steps); + } + + nr++; + } + + step.type = ENGINE_MAP; + goto add_step; } else if (!strcmp(field, "X")) { unsigned int nr = 0; while ((field = strtok_r(fstart, ".", &fctx)) != @@ -774,6 +847,7 @@ terminate_bb(struct w_step *w, unsigned int flags) }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, @@ -796,11 +870,36 @@ eb_set_engine(struct drm_i915_gem_execbuffer2 *eb, eb->flags = eb_engine_map[engine]; }+static unsigned int+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) + return i + 1; + } + + igt_assert(0); + return 0;No balancer in the map at this point?
Correct, only in one of the later patches.
+} + +static struct ctx * +__get_ctx(struct workload *wrk, struct w_step *w) +{ + return &wrk->ctx_list[w->context * 2]; +} + static void -eb_update_flags(struct w_step *w, enum intel_engine_id engine, - unsigned int flags) +eb_update_flags(struct workload *wrk, struct w_step *w, + enum intel_engine_id engine, unsigned int flags) { - eb_set_engine(&w->eb, engine, flags); + struct ctx *ctx = __get_ctx(wrk, w); + + if (ctx->engine_map) + w->eb.flags = find_engine_in_map(ctx, engine); + else + eb_set_engine(&w->eb, engine, flags);w->eb.flags |= I915_EXEC_HANDLE_LUT;w->eb.flags |= I915_EXEC_NO_RELOC; @@ -819,12 +918,6 @@ get_status_objects(struct workload *wrk) return wrk->status_object; }-static struct ctx *-__get_ctx(struct workload *wrk, struct w_step *w) -{ - return &wrk->ctx_list[w->context * 2]; -} - static uint32_t get_ctxid(struct workload *wrk, struct w_step *w) { @@ -894,7 +987,7 @@ alloc_step_batch(struct workload *wrk, struct w_step *w, unsigned int flags) engine = VCS2; else if (flags & SWAPVCS && engine == VCS2) engine = VCS1; - eb_update_flags(w, engine, flags); + eb_update_flags(wrk, w, engine, flags); #ifdef DEBUG printf("%u: %u:|", w->idx, w->eb.buffer_count); for (i = 0; i <= j; i++) @@ -936,7 +1029,7 @@ static void vm_destroy(int i915, uint32_t vm_id) igt_assert_eq(__vm_destroy(i915, vm_id), 0); }-static void+static int prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) { unsigned int ctx_vcs; @@ -999,30 +1092,53 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) /* * Identify if contexts target specific engine instances and if they * want to be balanced. + * + * Transfer over engine map configuration from the workload step. */ for (j = 0; j < wrk->nr_ctxs; j += 2) { bool targets = false; bool balance = false;for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {- if (w->type != BATCH) - continue; - if (w->context != (j / 2)) continue;- if (w->engine == VCS)- balance = true; - else - targets = true; + if (w->type == BATCH) { + if (w->engine == VCS) + balance = true; + else + targets = true; + } else if (w->type == ENGINE_MAP) { + wrk->ctx_list[j].engine_map = w->engine_map; + wrk->ctx_list[j].engine_map_count = + w->engine_map_count; + } }- if (flags & I915) {- wrk->ctx_list[j].targets_instance = targets; + wrk->ctx_list[j].targets_instance = targets; + if (flags & I915) wrk->ctx_list[j].wants_balance = balance; + } + + /* + * Ensure VCS is not allowed with engine map contexts. + */ + for (j = 0; j < wrk->nr_ctxs; j += 2) { + for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) { + if (w->context != (j / 2)) + continue; + + if (w->type != BATCH) + continue; + + if (wrk->ctx_list[j].engine_map && w->engine == VCS) {But wouldn't VCS still be meaning use the balancer and not a specific engine??? I'm not understanding how you are using maps in the .wsim :(
Batch sent to VCS means any VCS if not a context with a map, but VCS mentioned in the map now auto-expands to all present VCS instances.
VCS as engine specifier at execbuf time could be allowed if code would then check if there is a load balancer built of vcs engines in this context.
But what use case you think is not covered? We got legacy wsim files which implicitly create a map by doing: 1.VCS.1000.0.0 -> submit a batch to any vcs And then after this series you can also do: M.1.VCS B.1 1.DEFAULT.1000.0.0 Which would have the same effect. You would seem want: M.1.VCS B.1 1.VCS.1000.0.0 ? But I don't see what it gains? Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
