From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x733.google.com (mail-qk1-x733.google.com [IPv6:2607:f8b0:4864:20::733]) by gabe.freedesktop.org (Postfix) with ESMTPS id 73DCB6EA3F for ; Fri, 18 Jun 2021 16:22:42 +0000 (UTC) Received: by mail-qk1-x733.google.com with SMTP id g4so11035478qkl.1 for ; Fri, 18 Jun 2021 09:22:42 -0700 (PDT) MIME-Version: 1.0 References: <20210617191256.577244-1-jason@jlekstrand.net> <20210617191256.577244-35-jason@jlekstrand.net> <87fsxfej59.wl-ashutosh.dixit@intel.com> In-Reply-To: <87fsxfej59.wl-ashutosh.dixit@intel.com> From: Jason Ekstrand Date: Fri, 18 Jun 2021 11:22:30 -0500 Message-ID: Subject: Re: [igt-dev] [PATCH i-g-t 34/79] tests/i915/gem_watchdog: Convert to intel_ctx_t (v2) List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: "Dixit, Ashutosh" Cc: IGT GPU Tools List-ID: On Fri, Jun 18, 2021 at 2:17 AM Dixit, Ashutosh 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 > > A few comments below. Please fix as needed, after fixing this is: > > Reviewed-by: Ashutosh Dixit > > > -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. _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev