From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id E574C6E064 for ; Fri, 18 Jun 2021 07:17:39 +0000 (UTC) Date: Fri, 18 Jun 2021 00:17:38 -0700 Message-ID: <87fsxfej59.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20210617191256.577244-35-jason@jlekstrand.net> References: <20210617191256.577244-1-jason@jlekstrand.net> <20210617191256.577244-35-jason@jlekstrand.net> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") 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: Jason Ekstrand Cc: igt-dev@lists.freedesktop.org List-ID: 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. > -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 :-/ > > - 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? > + > /* > * 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). > @@ -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. _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev