From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id D3F43112082 for ; Fri, 22 Apr 2022 06:12:43 +0000 (UTC) Date: Fri, 22 Apr 2022 08:12:39 +0200 From: Zbigniew =?utf-8?Q?Kempczy=C5=84ski?= To: Kamil Konieczny , igt-dev@lists.freedesktop.org Message-ID: References: <20220421060955.21016-1-zbigniew.kempczynski@intel.com> <20220421060955.21016-3-zbigniew.kempczynski@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [igt-dev] [PATCH i-g-t 2/3] tests/i915/gem_exec_schedule: Don't use default context List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On Thu, Apr 21, 2022 at 09:51:05PM +0200, Kamil Konieczny wrote: > Hi Zbigniew, > > On 2022-04-21 at 08:09:54 +0200, Zbigniew Kempczyński wrote: > > Config created on top of all physical devices may provide more devices > > than default context contains. Using engine id from such "richer" > > context is wrong when default context will be chosen for this engine. > > Add dedicated context to handle all engines covered by context cfg. > > > > Signed-off-by: Zbigniew Kempczyński > > Cc: Kamil Konieczny > > Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/5444 (timeslicing) > > --- > > tests/i915/gem_exec_schedule.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c > > index caeac255de..dfcff849c8 100644 > > --- a/tests/i915/gem_exec_schedule.c > > +++ b/tests/i915/gem_exec_schedule.c > > @@ -568,7 +568,7 @@ static void timeslice(int i915, const intel_ctx_cfg_t *cfg, > > .buffers_ptr = to_user_pointer(&obj), > > .buffer_count = 1, > > }; > > - const intel_ctx_t *ctx; > > + const intel_ctx_t *ctx[2]; > > This looks redundant, see below. > > > uint32_t *result; > > int out; > > > > @@ -582,22 +582,25 @@ static void timeslice(int i915, const intel_ctx_cfg_t *cfg, > > igt_require(gem_scheduler_has_timeslicing(i915)); > > igt_require(intel_gen(intel_get_drm_devid(i915)) >= 8); > > > > + ctx[0] = intel_ctx_create(i915, cfg); > > obj.handle = timeslicing_batches(i915, &offset); > > result = gem_mmap__device_coherent(i915, obj.handle, 0, 4096, PROT_READ); > > > > execbuf.flags = engine | I915_EXEC_FENCE_OUT; > > execbuf.batch_start_offset = 0; > > + execbuf.rsvd1 = ctx[0]->id; > > gem_execbuf_wr(i915, &execbuf); > > + intel_ctx_destroy(i915, ctx[0]); > > Here you destroy this and later you create other context, so you > can just reuse ctx pointer. Yes, I know I can reuse. I created indexed array to easier track in which context execbufs are executed. I mean when you're looking at the code and you see execbuf.rsvd1 = ctx->id; you need to track the above code what context is in use. Using ctx[n]->id increases readability imo. I will change to single ctx if you insist. -- Zbigniew > > > > /* No coupling between requests; free to timeslice */ > > > > - ctx = intel_ctx_create(i915, cfg); > > - execbuf.rsvd1 = ctx->id; > > + ctx[1] = intel_ctx_create(i915, cfg); > > + execbuf.rsvd1 = ctx[1]->id; > > execbuf.rsvd2 >>= 32; > > execbuf.flags = engine | I915_EXEC_FENCE_OUT; > > execbuf.batch_start_offset = offset; > > gem_execbuf_wr(i915, &execbuf); > > - intel_ctx_destroy(i915, ctx); > > + intel_ctx_destroy(i915, ctx[1]); > > Maybe here is place for intel_ctx_destroy(i915, ctx[0]); ? > > > > > gem_sync(i915, obj.handle); > > gem_close(i915, obj.handle); > > -- > > 2.32.0 > > > Regards, > Kamil