Re: [igt-dev] [PATCH i-g-t] lib/i915: Return actual submission method from gem_submission_method

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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 <john.c.harrison@intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > ---
> > >   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.