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 830F76E886 for ; Tue, 19 Oct 2021 00:12:06 +0000 (UTC) Date: Mon, 18 Oct 2021 17:12:05 -0700 Message-ID: <87bl3l98kq.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" In-Reply-To: <87czo1995c.wl-ashutosh.dixit@intel.com> References: <20211016002334.14580-1-ashutosh.dixit@intel.com> <15873c81-caec-c822-867f-7f0392606126@intel.com> <87czo1995c.wl-ashutosh.dixit@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] lib/i915: Return actual submission method from gem_submission_method List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: John Harrison , Tvrtko Ursulin Cc: igt-dev@lists.freedesktop.org List-ID: On Mon, 18 Oct 2021 16:59:43 -0700, Dixit, Ashutosh wrote: > > On Mon, 18 Oct 2021 16:39:40 -0700, John Harrison wrote: > > > > On 10/15/2021 17:23, Ashutosh Dixit wrote: > > > gem_submission_method() purports to return the currently used submission > > > method by the driver, as evidenced by its callers. Therefore remove the > > > GEM_SUBMISSION_EXECLISTS flag when GuC submission is detected. > > > > > > This also fixes gem_has_execlists() to match its description, previously > > > gem_has_execlists() would return true even if GuC submission was actually > > > being used in the driver. > > > > > > Reported-by: John Harrison > > > Cc: Tvrtko Ursulin > > > Signed-off-by: Ashutosh Dixit > > > --- > > > lib/i915/gem_submission.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c > > > index 2627b802cfb..b037d04cc4a 100644 > > > --- a/lib/i915/gem_submission.c > > > +++ b/lib/i915/gem_submission.c > > > @@ -86,7 +86,7 @@ unsigned gem_submission_method(int fd) > > > return 0; > > > if (igt_sysfs_get_u32(dir, "enable_guc") & 1) { > > > - flags |= GEM_SUBMISSION_GUC | GEM_SUBMISSION_EXECLISTS; > > > + flags |= GEM_SUBMISSION_GUC; > > > goto out; > > > } > > > > > Looks good to me, but as per the comments in the other thread, this might > > have unintended side effects. Have you gone through all instances of the > > submission query usages to check how it is used and whether this will break > > something? E.g. if something is explicitly testing for execlist support to > > mean 'something better than ring buffer' then it would now fail. > > There are at present just these two instances of gem_has_execlists: > > *** tests/i915/gem_ctx_shared.c: > disjoint_timelines[162] igt_require(gem_has_execlists(i915)); > > *** tests/i915/gem_watchdog.c: > virtual[225] igt_require(gem_has_execlists(i915)); > > I'll try to see if I can figure out if they will be affected in any > way. Actually, earlier these tests were running for both execlists and guc submission. If they are meant to run even for guc then they will now skip in the guc case after this patch. Therefore I am thinking the exact equivalent of what was going on earlier is this condition: igt_require(gem_has_execlists() | gem_has_guc_submission()); So that's all that is needed now correct? But what I am not sure of yet is whether they were meant to run for guc too or just execlists.