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

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

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The gem_exec_fair test is specifically testing scheduler algorithm
performance. However, GuC does not implement the same algorithm as
execlist mode and this test is not applicable. So, until sw arch
approves a new algorithm and it is implemented in GuC, stop running
the test.

v2: Add exceptions for TGL/RKL/ADL-S - review feedback from Tvrtko.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 tests/i915/gem_exec_fair.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/i915/gem_exec_fair.c b/tests/i915/gem_exec_fair.c
index ef5a450f6..fc690f438 100644
--- a/tests/i915/gem_exec_fair.c
+++ b/tests/i915/gem_exec_fair.c
@@ -1302,6 +1302,8 @@ igt_main
 	int i915 = -1;
 
 	igt_fixture {
+		uint32_t devid;
+
 		igt_require_sw_sync();
 
 		i915 = drm_open_driver_master(DRIVER_INTEL);
@@ -1314,6 +1316,14 @@ igt_main
 		igt_require(gem_scheduler_enabled(i915));
 		igt_require(gem_scheduler_has_ctx_priority(i915));
 
+		/*
+		 * 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));
+
 		cfg = intel_ctx_cfg_all_physical(i915);
 
 		igt_info("CS timestamp frequency: %d\n",
-- 
2.25.1

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

[Dixit, Ashutosh]

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.

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

[John Harrison]

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.

John.

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

[Dixit, Ashutosh]

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.