From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5D0DB10E603 for ; Tue, 15 Feb 2022 22:58:56 +0000 (UTC) Date: Tue, 15 Feb 2022 14:58:54 -0800 Message-ID: <87k0dvd9e9.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <20220214172208.3338908-2-jasmine.newsome@intel.com> References: <20220214172208.3338908-1-jasmine.newsome@intel.com> <20220214172208.3338908-2-jasmine.newsome@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [igt-dev] [PATCH i-g-t 1/1] tests/i915/gem_spin_batch: Removing context persistence List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Jasmine Newsome Cc: igt-dev@lists.freedesktop.org List-ID: On Mon, 14 Feb 2022 09:22:08 -0800, Jasmine Newsome wrote: > > The spin all test relied on context persistence unecessarily > by trying to destroy contexts while keeping spinners active. > The current implementation of context persistence in i915 > can cause failures, and persistence is not needed for this > test. IGT follows the kernel coding style so 8 wide tabs and no spaces, you can run kernel checkpatch to check IGT patches "for the most part" (ignoring kernel specific issues). > Moving intel_ctx_destroy after igt_spin_end/free. > > Signed-off-by: Jasmine Newsome > --- > tests/i915/gem_spin_batch.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/tests/i915/gem_spin_batch.c b/tests/i915/gem_spin_batch.c > index 653812c7..65dccd83 100644 > --- a/tests/i915/gem_spin_batch.c > +++ b/tests/i915/gem_spin_batch.c > @@ -145,28 +145,31 @@ static void spin_all(int i915, const intel_ctx_t *ctx, unsigned int flags) > struct igt_spin *spin, *n; > uint64_t ahnd; > IGT_LIST_HEAD(list); > + const intel_ctx_t *local_ctx[GEM_MAX_ENGINES]; > + int num_ctx; > > - for_each_ctx_cfg_engine(i915, &cfg, e) { > + num_ctx = 0; > + for_each_ctx_cfg_engine(i915, &cfg, e) { > if (!gem_class_can_store_dword(i915, e->class)) > continue; > > if (flags & PARALLEL_SPIN_NEW_CTX) > - ctx = intel_ctx_create(i915, &cfg); > + local_ctx[num_ctx] = intel_ctx_create(i915, &cfg); > ahnd = get_reloc_ahnd(i915, ctx->id); > > /* Prevent preemption so only one is allowed on each engine */ > spin = igt_spin_new(i915, > .ahnd = ahnd, > - .ctx = ctx, > + .ctx = local_ctx[num_ctx], This is not initialized if !PARALLEL_SPIN_NEW_CTX. I would say local_ctx[] array is not needed. We could do something like: igt_spin_new(.ctx = flags & PARALLEL_SPIN_NEW_CTX ? intel_ctx_create() : ctx); And then use intel_ctx_destroy(spin->execbuf.rsvd1) before freeing the spin in the igt_list_for_each_entry_safe() loop below. > .engine = e->flags, > .flags = (IGT_SPIN_POLL_RUN | > IGT_SPIN_NO_PREEMPTION)); > - if (flags & PARALLEL_SPIN_NEW_CTX) > - intel_ctx_destroy(i915, ctx); > > - igt_spin_busywait_until_started(spin); > - igt_list_move(&spin->link, &list); > - } > + igt_assert_eq(spin->execbuf.rsvd1, local_ctx[num_ctx]->id); > + igt_spin_busywait_until_started(spin); > + igt_list_move(&spin->link, &list); > + num_ctx++; > + } > > igt_list_for_each_entry_safe(spin, n, &list, link) { > igt_assert(gem_bo_busy(i915, spin->handle)); > @@ -176,6 +179,11 @@ static void spin_all(int i915, const intel_ctx_t *ctx, unsigned int flags) > igt_spin_free(i915, spin); > put_ahnd(ahnd); > } > + > + if (flags & PARALLEL_SPIN_NEW_CTX){ > + while (num_ctx) > + intel_ctx_destroy(i915, local_ctx[--num_ctx]); > + } > } > > static bool has_userptr(int fd) > -- > 2.25.1 >