Re: [Intel-gfx] [igt-dev] [PATCH v2 i-g-t] tests/i915: Skip gem_exec_fair on GuC based platforms

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

On Fri, 15 Oct 2021 14:46:20 -0700, John Harrison wrote:
>
> On 10/15/2021 07:52, Dixit, Ashutosh wrote:
> > On Thu, 14 Oct 2021 12:42:38 -0700, <John.C.Harrison@Intel.com> wrote:
> >> +		/*
> >> +		 * These tests are for a specific scheduling model which is
> >> +		 * not currently implemented by GuC. So skip on GuC platforms.
> >> +		 */
> >> +		devid = intel_get_drm_devid(i915);
> >> +		igt_require((intel_gen(devid) < 12) || IS_TIGERLAKE(devid) ||
> >> +			    IS_ROCKETLAKE(devid) || IS_ALDERLAKE_S(devid));
> > As I hinted on v1 let's just do this here:
> >
> >	igt_require(gem_has_guc_submission(i915));
> >
> > So that we can can have a single unified way of detecting if GuC is being
> > used throughout IGT. Today it is gem_has_guc_submission() and it works with
> > the current kernel.
>
> Earlier, you were saying that 'has' was only checking for capability not
> usage. Which would be pretty useless for this situation. Looking at the
> code, though it sort of does work. It checks the live value of the
> enable_guc module parameter. If that says that GuC submission is enabled
> then either we are using GuC submission or we have no engines (because a
> failure to start the submission backend is terminal, there is no fallback
> to execlist mode if GuC didn't work). So it can be used.
>
> I say sort of, though, because the code also sets 'has_execlists' when it
> sets 'has_guc'. Which means that the gem_has_execlists() test is not
> useable as an indication that the execlist back end is being used. So
> gem_has_execlists() and gem_has_guc_submission() are basically very
> non-orthogonal. One is a test of hardware presence irrespective of use, the
> other is a test of usage irrespective of presence. The comment in the code
> is 'query whether the driver is using execlists as a hardware submission
> method'. So it seems like that was the original intention. Whether it has
> been broken since or was just broken from the beginning is unclear.

I completely agree, there is all this confusion in the code, it needs to be
cleaned up. For now I thought it was better to use a centralized way to
detect GuC rather than have different ways in different files. Fortunately
gem_has_guc_submission() is doing that though gem_has_execlists() is
broken in detecting if execlists are being used.