[igt-dev] [PATCH i-g-t 34/79] tests/i915/gem_watchdog: Convert to intel_ctx_t (v2)
Jason Ekstrand
jason at jlekstrand.net
Fri Jun 18 16:22:30 UTC 2021
On Fri, Jun 18, 2021 at 2:17 AM Dixit, Ashutosh
<ashutosh.dixit at intel.com> wrote:
>
> On Thu, 17 Jun 2021 12:12:41 -0700, Jason Ekstrand wrote:
> >
> > v2 (Jason Ekstrand):
> > - Rebase on Tvrtko's changes
> > - Fix an issue in default-virtual with the set-once rule for engines
> >
> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
>
> A few comments below. Please fix as needed, after fixing this is:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
>
> > -static void physical(int i915)
> > +static void physical(int i915, const intel_ctx_t *ctx)
> > {
> > const unsigned int wait_us = default_timeout_wait_s * USEC_PER_SEC;
> > - unsigned int num_engines = __engines__->num_engines, i, count;
> > + unsigned int num_engines, i, count;
> > const struct intel_execution_engine2 *e;
> > - unsigned int expect = num_engines;
> > - igt_spin_t *spin[num_engines];
> > + igt_spin_t *spin[GEM_MAX_ENGINES];
> >
> > i = 0;
> > - __for_each_physical_engine(i915, e) {
> > - spin[i] = igt_spin_new(i915,
> > + for_each_ctx_engine(i915, ctx, e) {
> > + spin[i] = igt_spin_new(i915, .ctx = ctx,
> > .engine = e->flags,
> > .flags = spin_flags());
> > i++;
> > }
> > + num_engines = i;
>
> Don't even need this, num_engines = ctx->cfg->num_engines.
It isn't if we're using the old ring API.
> > -static void virtual(int i915)
> > +static void virtual(int i915, const intel_ctx_cfg_t *base_cfg)
> > {
> > const unsigned int wait_us = default_timeout_wait_s * USEC_PER_SEC;
> > - unsigned int num_engines = __engines__->num_engines, i, count;
> > + unsigned int num_engines = base_cfg->num_engines, i, count;
> > igt_spin_t *spin[num_engines];
> > unsigned int expect = num_engines;
> > - uint32_t ctx[num_engines];
> > - uint32_t vm;
> > + intel_ctx_cfg_t cfg = {};
> > + const intel_ctx_t *ctx[num_engines];
> >
> > igt_require(gem_has_execlists(i915));
> >
> > igt_debug("%u virtual engines\n", num_engines);
> > igt_require(num_engines);
> >
> > + cfg.vm = gem_vm_create(i915);
> > +
> > i = 0;
> > for (int class = 0; class < 32; class++) {
> > struct i915_engine_class_instance *ci;
> >
> > - ci = list_engines(class, &count);
> > + ci = list_engines(base_cfg, class, &count);
> > if (!ci)
> > continue;
> >
> > @@ -296,17 +238,12 @@ static void virtual(int i915)
> >
> > igt_assert(i < num_engines);
> >
> > - ctx[i] = gem_context_create(i915);
> > + ctx[i] = intel_ctx_create(i915, &cfg);
>
> I would have expected this to be base_cfg, since cfg only has legacy
> engines. But even the original code has only legacy engines for the load
> balancer so this "somehow works" in a manner unknown to me :-/
That's because set_load_balancer sets a whole new engine set. :-(
Eventually, we'll probably want to add load balancing to the context
config but this works for now and keeps this test using roughly the
same pattern as the media driver.
> >
> > - if (!i)
> > - vm = ctx_get_vm(i915, ctx[i]);
> > - else
> > - ctx_set_vm(i915, ctx[i], vm);
> > -
> > - set_load_balancer(i915, ctx[i], ci, count, NULL);
> > + set_load_balancer(i915, ctx[i]->id, ci, count, NULL);
> >
> > spin[i] = igt_spin_new(i915,
> > - .ctx_id = ctx[i],
> > + .ctx = ctx[i],
> > .flags = spin_flags());
> > i++;
> > }
> > @@ -317,8 +254,8 @@ static void virtual(int i915)
> > count = wait_timeout(i915, spin, num_engines, wait_us, expect);
> >
> > for (i = 0; i < num_engines && spin[i]; i++) {
> > - gem_context_destroy(i915, ctx[i]);
> > igt_spin_free(i915, spin[i]);
> > + intel_ctx_destroy(i915, ctx[i]);
> > }
> >
> > igt_assert_eq(count, expect);
> > @@ -499,17 +436,6 @@ delay_create(int i915, uint32_t ctx,
> > return obj;
> > }
> >
> > -static uint32_t vm_clone(int i915)
> > -{
> > - uint32_t ctx = 0;
> > - __gem_context_clone(i915, 0,
> > - I915_CONTEXT_CLONE_VM |
> > - I915_CONTEXT_CLONE_ENGINES,
> > - I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE,
> > - &ctx);
> > - return ctx;
> > -}
> > -
> > static int __execbuf(int i915, struct drm_i915_gem_execbuffer2 *execbuf)
> > {
> > int err;
> > @@ -526,6 +452,7 @@ static int __execbuf(int i915, struct drm_i915_gem_execbuffer2 *execbuf)
> >
> > static uint32_t
> > far_delay(int i915, unsigned long delay, unsigned int target,
> > + const intel_ctx_t *ctx,
> > const struct intel_execution_engine2 *e, int *fence)
> > {
> > struct drm_i915_gem_exec_object2 obj = delay_create(i915, 0, e, delay);
> > @@ -540,6 +467,7 @@ far_delay(int i915, unsigned long delay, unsigned int target,
> > .buffer_count = 2,
> > .flags = e->flags,
> > };
> > + intel_ctx_cfg_t cfg = ctx->cfg;
> > uint32_t handle = gem_create(i915, 4096);
> > unsigned long count, submit;
> >
> > @@ -552,23 +480,27 @@ far_delay(int i915, unsigned long delay, unsigned int target,
> > submit *= NSEC_PER_SEC;
> > submit /= 2 * delay;
> >
> > + if (gem_has_vm(i915))
> > + cfg.vm = gem_vm_create(i915);
>
> Might as well add I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE too if needed?
Sure. Why not? Keeps it closer to the original.
> > +
> > /*
> > * Submit a few long chains of individually short pieces of work
> > * against a shared object.
> > */
> > for (count = 0; count < submit;) {
> > - execbuf.rsvd1 = vm_clone(i915);
> > + const intel_ctx_t *tmp_ctx = intel_ctx_create(i915, &cfg);
> > + execbuf.rsvd1 = tmp_ctx->id;
> > if (!execbuf.rsvd1)
>
> This check is always false now (intel_ctx_create will assert on error).
Yes, but only because this test requires gen 8+ and therefore real
context support. I've added an igt_assert(tmp_ctx->id);
> > @@ -652,32 +585,22 @@ igt_main
> > }
> >
> > i915 = gem_reopen_driver(i915); /* Apply modparam. */
> > -
> > - __engines__ = malloc(4096);
> > - igt_assert(__engines__);
> > - memset(__engines__, 0, 4096);
> > - memset(&item, 0, sizeof(item));
> > - item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> > - item.data_ptr = to_user_pointer(__engines__);
> > - item.length = 4096;
> > - i915_query_items(i915, &item, 1);
> > - igt_assert(item.length >= 0);
> > - igt_assert(item.length <= 4096);
> > - igt_assert(__engines__->num_engines > 0);
> > + ctx = intel_ctx_create_all_physical(i915);
> > }
> >
> > igt_subtest_group {
> > igt_subtest("default-physical")
> > - physical(i915);
> > + physical(i915, ctx);
> >
> > igt_subtest("default-virtual")
> > - virtual(i915);
> > + virtual(i915, &ctx->cfg);
> > }
> >
> > igt_subtest_with_dynamic("far-fence") {
> > - __for_each_physical_engine(i915, e) {
> > + for_each_ctx_engine(i915, ctx, e) {
> > igt_dynamic_f("%s", e->name)
> > - far_fence(i915, default_timeout_wait_s * 3, e);
> > + far_fence(i915, default_timeout_wait_s * 3,
> > + ctx, e);
> > }
> > }
>
> Missing intel_ctx_destroy.
Fixed.
More information about the igt-dev
mailing list