| Message ID | 225f88d1044053674cbd632998c69c0c677a530e.1579731227.git.dale.b.stimson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915/gem_ctx_isolation: __for_each_physical_engine + engine mapping | expand |
Quoting Dale B Stimson (2020-01-22 23:26:57) > Switch from simple iteration over all potential engines to using > macro __for_each_physical_engine which only returns engines that are > actually present. > > For each context (as it is created) call gem_context_set_all_engines > so that execbuf will interpret the engine specification in the new way. > > Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com> > --- > tests/i915/gem_ctx_isolation.c | 41 ++++++++++++++++++++++++++-------- > 1 file changed, 32 insertions(+), 9 deletions(-) > > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c > index 25113b054..31a20ed3a 100644 > --- a/tests/i915/gem_ctx_isolation.c > +++ b/tests/i915/gem_ctx_isolation.c > @@ -240,6 +240,25 @@ static bool ignore_register(uint32_t offset) > return false; > } > > +/* > + * context_create_plus_all_engines - Same as gem_context_create plus setup. > + * > + * This is a convenience function that may be called instead of the sequence > + * of gem_context_create followed by gem_context_set_all_engines. > + * If gem_has_engine_topology(), then function gem_context_set_all_engines > + * indicates that future execbuf calls for this context should interpret the > + * engine specification in a gem_engine_topology-compatible way. > + */ > +static uint32_t context_create_plus_all_engines(int fd) > +{ > + uint32_t ctx; > + > + ctx = gem_context_create(fd); > + gem_context_set_all_engines(fd, ctx); > + > + return ctx; > +} gem_context_clone_with_engines() so we can stop assuming that all-engines is the right answer, because that depends on the conditions set up by the iterator on the first context. -Chris
On 23/01/2020 09:09, Chris Wilson wrote: > Quoting Dale B Stimson (2020-01-22 23:26:57) >> Switch from simple iteration over all potential engines to using >> macro __for_each_physical_engine which only returns engines that are >> actually present. >> >> For each context (as it is created) call gem_context_set_all_engines >> so that execbuf will interpret the engine specification in the new way. >> >> Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com> >> --- >> tests/i915/gem_ctx_isolation.c | 41 ++++++++++++++++++++++++++-------- >> 1 file changed, 32 insertions(+), 9 deletions(-) >> >> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c >> index 25113b054..31a20ed3a 100644 >> --- a/tests/i915/gem_ctx_isolation.c >> +++ b/tests/i915/gem_ctx_isolation.c >> @@ -240,6 +240,25 @@ static bool ignore_register(uint32_t offset) >> return false; >> } >> >> +/* >> + * context_create_plus_all_engines - Same as gem_context_create plus setup. >> + * >> + * This is a convenience function that may be called instead of the sequence >> + * of gem_context_create followed by gem_context_set_all_engines. >> + * If gem_has_engine_topology(), then function gem_context_set_all_engines >> + * indicates that future execbuf calls for this context should interpret the >> + * engine specification in a gem_engine_topology-compatible way. >> + */ >> +static uint32_t context_create_plus_all_engines(int fd) >> +{ >> + uint32_t ctx; >> + >> + ctx = gem_context_create(fd); >> + gem_context_set_all_engines(fd, ctx); >> + >> + return ctx; >> +} > > gem_context_clone_with_engines() so we can stop assuming that > all-engines is the right answer, because that depends on the conditions > set up by the iterator on the first context. gem_context_clone_with_engines was agreed upon in principle some time ago but never implemented. I have now posted this as https://patchwork.freedesktop.org/series/72464/ and plan to merge it once it passes CI. Dale, Arjun, Krishnaiah and Sreedhar - you have in progress patches which use gem_context_set_all_engines which will be gone and you will need to adjust your work accordingly. Sreedhar specifically for your change in gem_exec_parallel we will need to add a new helper which transfers the engine map from one fd/context to another. I will copy you on a patch which will add it. Regards, Tvrtko
On 2020-01-23 15:59:33, Tvrtko Ursulin wrote: > gem_context_clone_with_engines was agreed upon in principle some time ago > but never implemented. I have now posted this as > https://patchwork.freedesktop.org/series/72464/ and plan to merge it once it > passes CI. > > Dale, Arjun, Krishnaiah and Sreedhar - you have in progress patches which > use gem_context_set_all_engines which will be gone and you will need to > adjust your work accordingly. > > Sreedhar specifically for your change in gem_exec_parallel we will need to > add a new helper which transfers the engine map from one fd/context to > another. I will copy you on a patch which will add it. I have a new iteration of the gem_ctx_isolation patches (on top of your patch and using gem_context_clone_with_engines) which I will submit to the ML as soon as your patch merges. Thanks, Dale
diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c index 25113b054..31a20ed3a 100644 --- a/tests/i915/gem_ctx_isolation.c +++ b/tests/i915/gem_ctx_isolation.c @@ -240,6 +240,25 @@ static bool ignore_register(uint32_t offset) return false; } +/* + * context_create_plus_all_engines - Same as gem_context_create plus setup. + * + * This is a convenience function that may be called instead of the sequence + * of gem_context_create followed by gem_context_set_all_engines. + * If gem_has_engine_topology(), then function gem_context_set_all_engines + * indicates that future execbuf calls for this context should interpret the + * engine specification in a gem_engine_topology-compatible way. + */ +static uint32_t context_create_plus_all_engines(int fd) +{ + uint32_t ctx; + + ctx = gem_context_create(fd); + gem_context_set_all_engines(fd, ctx); + + return ctx; +} + static void tmpl_regs(int fd, uint32_t ctx, const struct intel_execution_engine2 *e, @@ -586,7 +605,8 @@ static void nonpriv(int fd, igt_spin_t *spin = NULL; uint32_t ctx, regs[2], tmpl; - ctx = gem_context_create(fd); + ctx = context_create_plus_all_engines(fd); + tmpl = read_regs(fd, ctx, e, flags); regs[0] = read_regs(fd, ctx, e, flags); @@ -599,7 +619,7 @@ static void nonpriv(int fd, write_regs(fd, ctx, e, flags, values[v]); if (flags & DIRTY2) { - uint32_t sw = gem_context_create(fd); + uint32_t sw = context_create_plus_all_engines(fd); igt_spin_t *syncpt, *dirt; /* Explicit sync to keep the switch between write/read */ @@ -668,7 +688,7 @@ static void isolation(int fd, igt_spin_t *spin = NULL; uint32_t ctx[2], regs[2], tmp; - ctx[0] = gem_context_create(fd); + ctx[0] = context_create_plus_all_engines(fd); regs[0] = read_regs(fd, ctx[0], e, flags); spin = igt_spin_new(fd, .ctx = ctx[0], .engine = e->flags); @@ -687,7 +707,7 @@ static void isolation(int fd, * the default values from this context, but if goes badly we * see the corruption from the previous context instead! */ - ctx[1] = gem_context_create(fd); + ctx[1] = context_create_plus_all_engines(fd); regs[1] = read_regs(fd, ctx[1], e, flags); if (flags & DIRTY2) { @@ -727,7 +747,7 @@ static void isolation(int fd, static void inject_reset_context(int fd, const struct intel_execution_engine2 *e) { struct igt_spin_factory opts = { - .ctx = gem_context_create(fd), + .ctx = context_create_plus_all_engines(fd), .engine = e->flags, .flags = IGT_SPIN_FAST, }; @@ -775,11 +795,11 @@ static void preservation(int fd, gem_quiescent_gpu(fd); - ctx[num_values] = gem_context_create(fd); + ctx[num_values] = context_create_plus_all_engines(fd); spin = igt_spin_new(fd, .ctx = ctx[num_values], .engine = e->flags); regs[num_values][0] = read_regs(fd, ctx[num_values], e, flags); for (int v = 0; v < num_values; v++) { - ctx[v] = gem_context_create(fd); + ctx[v] = context_create_plus_all_engines(fd); write_regs(fd, ctx[v], e, flags, values[v]); regs[v][0] = read_regs(fd, ctx[v], e, flags); @@ -854,6 +874,7 @@ static unsigned int __has_context_isolation(int fd) igt_main { unsigned int has_context_isolation = 0; + const struct intel_execution_engine2 *e; int fd = -1; igt_fixture { @@ -871,10 +892,12 @@ igt_main igt_warn_on_f(gen > LAST_KNOWN_GEN, "GEN not recognized! Test needs to be updated to run.\n"); igt_skip_on(gen > LAST_KNOWN_GEN); + + /* Context 0 is created on device open. */ + gem_context_set_all_engines(fd, 0); } - for (const struct intel_execution_engine2 *e = intel_execution_engines2; - e->name; e++) { + __for_each_physical_engine(fd, e) { igt_subtest_group { igt_fixture { igt_require(has_context_isolation & (1 << e->class));
Switch from simple iteration over all potential engines to using macro __for_each_physical_engine which only returns engines that are actually present. For each context (as it is created) call gem_context_set_all_engines so that execbuf will interpret the engine specification in the new way. Signed-off-by: Dale B Stimson <dale.b.stimson@intel.com> --- tests/i915/gem_ctx_isolation.c | 41 ++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 9 deletions(-)