From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 9824B6E07B for ; Tue, 22 Jun 2021 01:51:57 +0000 (UTC) Date: Mon, 21 Jun 2021 18:51:56 -0700 Message-ID: <87czse8y4j.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20210617191256.577244-44-jason@jlekstrand.net> References: <20210617191256.577244-1-jason@jlekstrand.net> <20210617191256.577244-44-jason@jlekstrand.net> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Subject: Re: [igt-dev] [PATCH i-g-t 43/79] tests/i915/gem_ctx_persistence: Convert to intel_ctx_t 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:50 -0700, Jason Ekstrand wrote: > > Signed-off-by: Jason Ekstrand A couple of questions below just in case, otherwise this is: Reviewed-by: Ashutosh Dixit > diff --git a/lib/intel_ctx.h b/lib/intel_ctx.h > index 054fecc4a..e34cefc14 100644 > --- a/lib/intel_ctx.h > +++ b/lib/intel_ctx.h > @@ -16,6 +16,7 @@ > * intel_ctx_cfg_t: > * @flags: Context create flags > * @vm: VM to inherit or 0 for using a per-context VM > + * @nopersist: set I915_CONTEXT_PARAM_PERSISTENCE to 0 > * @num_engines: Number of client-specified engines or 0 for legacy mode > * @engines: Client-specified engines > * > @@ -42,6 +43,7 @@ > typedef struct intel_ctx_cfg { > uint32_t flags; > uint32_t vm; > + bool nopersist; To avoid the negative, wondering if we could have had a 'persist' field here rather than 'nopersist'? For regular contexts 'persist' seems fine. When this field is introduced we would default it to true but call the context setparam ioctl only if 'persist' were false. I do understand that contexts are persistent by default so 'nopersist' is really where something extra needs to be done. Is this why 'nopersist' was chosen here? Also, how are legacy contexts handled (since they really don't have a cfg)? Are they always persistent (or can they ever be non-persistent)? That would be another reason for having 'nopersist' I guess. > @@ -460,14 +467,16 @@ static void test_noheartbeat_many(int i915, int count, unsigned int flags) > igt_assert(set_heartbeat(i915, e->full_name, 500)); > > for (int n = 0; n < ARRAY_SIZE(spin); n++) { > - uint32_t ctx; > + const intel_ctx_t *ctx; > + > + ctx = intel_ctx_create(i915, NULL); > > - ctx = gem_context_create(i915); > - spin[n] = igt_spin_new(i915, ctx, .engine = eb_ring(e), > + spin[n] = igt_spin_new(i915, .ctx = ctx, > + .engine = eb_ring(e), > .flags = (IGT_SPIN_FENCE_OUT | > IGT_SPIN_POLL_RUN | > flags)); > - gem_context_destroy(i915, ctx); > + intel_ctx_destroy(i915, ctx); Any particular reason why we are creating legacy intel_ctx_t here (and also in test_noheartbeat_close())? There are other places in this file where we have not changed previous gem contexts created with gem_context_create() so just wondering. > @@ -772,13 +783,10 @@ static void test_process_mixed(int pfd, unsigned int engine) > > for (int persists = 0; persists <= 1; persists++) { > igt_spin_t *spin; > - uint32_t ctx; > - > - ctx = gem_context_create(i915); > - gem_context_copy_engines(pfd, 0, i915, ctx); > - gem_context_set_persistence(i915, ctx, persists); > + const intel_ctx_t *ctx; > > - spin = igt_spin_new(i915, ctx, > + ctx = ctx_create_persistence(i915, cfg, persists); > + spin = igt_spin_new(i915, .ctx = ctx, > .engine = engine, > .flags = IGT_SPIN_FENCE_OUT); No intel_ctx_destroy() here, but neither gem_context_destroy() so that seems to be the design... _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev