[PATCH i-g-t] tests/i915: Restore some BAT coverage

[PATCH i-g-t] tests/i915: Restore some BAT coverage

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine enumerated test names have changed so fast-feedback.testlist needs
to be updated. However listing all engines there won't scale. So instead
add new tests cases which iterate all engines internally.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 tests/i915/gem_busy.c                 | 17 +++++++++++---
 tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
 tests/intel-ci/fast-feedback.testlist | 33 ++++-----------------------
 3 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
index 2872e7b7be62..1f2a89991b9b 100644
--- a/tests/i915/gem_busy.c
+++ b/tests/i915/gem_busy.c
@@ -460,6 +460,14 @@ static void basic(int fd, const struct intel_execution_engine2 *e, unsigned flag
 	igt_spin_free(fd, spin);
 }
 
+static void all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		basic(i915, e, false);
+}
+
 igt_main
 {
 	const struct intel_execution_engine2 *e;
@@ -477,11 +485,14 @@ igt_main
 			igt_fork_hang_detector(fd);
 		}
 
+		igt_subtest("busy-all") {
+			gem_quiescent_gpu(fd);
+			all(fd);
+		}
+
 		__for_each_physical_engine(fd, e) {
 			igt_subtest_group {
-				igt_subtest_f("%sbusy-%s",
-					      e->class == I915_ENGINE_CLASS_RENDER
-					      ? "basic-" : "", e->name) {
+				igt_subtest_f("busy-%s", e->name) {
 					gem_quiescent_gpu(fd);
 					basic(fd, e, false);
 				}
diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
index 140cbdbc66a5..24e1cb5bdaa9 100644
--- a/tests/i915/gem_exec_basic.c
+++ b/tests/i915/gem_exec_basic.c
@@ -114,6 +114,30 @@ static void gtt(int fd, uint64_t flags)
 	munmap(execbuf, 4096);
 }
 
+static void all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		noop(i915, e->flags);
+}
+
+static void readonly_all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		readonly(i915, e->flags);
+}
+
+static void gtt_all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		gtt(i915, e->flags);
+}
+
 igt_main
 {
 	const struct intel_execution_engine2 *e;
@@ -126,6 +150,15 @@ igt_main
 		igt_fork_hang_detector(fd);
 	}
 
+	igt_subtest("all")
+		all(fd);
+
+	igt_subtest("readonly-all")
+		readonly_all(fd);
+
+	igt_subtest("gtt-all")
+		gtt_all(fd);
+
 	__for_each_physical_engine(fd, e) {
 		igt_subtest_f("basic-%s", e->name)
 			noop(fd, e->flags);
diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
index 40475b1ab361..03ac6dd1eee8 100644
--- a/tests/intel-ci/fast-feedback.testlist
+++ b/tests/intel-ci/fast-feedback.testlist
@@ -5,7 +5,7 @@ igt@debugfs_test@read_all_entries
 igt@gem_basic@bad-close
 igt@gem_basic@create-close
 igt@gem_basic@create-fd-close
-igt@gem_busy@basic-busy-default
+igt@gem_busy@busy-all
 igt@gem_close_race@basic-process
 igt@gem_close_race@basic-threads
 igt@gem_cpu_reloc@basic
@@ -15,27 +15,9 @@ igt@gem_ctx_exec@basic
 igt@gem_ctx_param@basic
 igt@gem_ctx_param@basic-default
 igt@gem_ctx_switch@basic-default
-igt@gem_exec_basic@basic-blt
-igt@gem_exec_basic@basic-bsd
-igt@gem_exec_basic@basic-bsd1
-igt@gem_exec_basic@basic-bsd2
-igt@gem_exec_basic@basic-default
-igt@gem_exec_basic@basic-render
-igt@gem_exec_basic@basic-vebox
-igt@gem_exec_basic@gtt-blt
-igt@gem_exec_basic@gtt-bsd
-igt@gem_exec_basic@gtt-bsd1
-igt@gem_exec_basic@gtt-bsd2
-igt@gem_exec_basic@gtt-default
-igt@gem_exec_basic@gtt-render
-igt@gem_exec_basic@gtt-vebox
-igt@gem_exec_basic@readonly-blt
-igt@gem_exec_basic@readonly-bsd
-igt@gem_exec_basic@readonly-bsd1
-igt@gem_exec_basic@readonly-bsd2
-igt@gem_exec_basic@readonly-default
-igt@gem_exec_basic@readonly-render
-igt@gem_exec_basic@readonly-vebox
+igt@gem_exec_basic@basic-all
+igt@gem_exec_basic@gtt-all
+igt@gem_exec_basic@readonly-all
 igt@gem_exec_create@basic
 igt@gem_exec_fence@basic-busy-default
 igt@gem_exec_fence@basic-wait-default
@@ -74,13 +56,6 @@ igt@gem_exec_reloc@basic-write-gtt-active
 igt@gem_exec_reloc@basic-write-read-active
 igt@gem_exec_reloc@basic-softpin
 igt@gem_exec_store@basic-all
-igt@gem_exec_store@basic-blt
-igt@gem_exec_store@basic-bsd
-igt@gem_exec_store@basic-bsd1
-igt@gem_exec_store@basic-bsd2
-igt@gem_exec_store@basic-default
-igt@gem_exec_store@basic-render
-igt@gem_exec_store@basic-vebox
 igt@gem_exec_suspend@basic
 igt@gem_exec_suspend@basic-s3
 igt@gem_exec_suspend@basic-s4-devices
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] tests/i915: Restore some BAT coverage

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-22 17:31:41)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine enumerated test names have changed so fast-feedback.testlist needs
> to be updated. However listing all engines there won't scale. So instead
> add new tests cases which iterate all engines internally.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  tests/i915/gem_busy.c                 | 17 +++++++++++---
>  tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
>  tests/intel-ci/fast-feedback.testlist | 33 ++++-----------------------
>  3 files changed, 51 insertions(+), 32 deletions(-)
> 
> diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> index 2872e7b7be62..1f2a89991b9b 100644
> --- a/tests/i915/gem_busy.c
> +++ b/tests/i915/gem_busy.c
> @@ -460,6 +460,14 @@ static void basic(int fd, const struct intel_execution_engine2 *e, unsigned flag
>         igt_spin_free(fd, spin);
>  }
>  
> +static void all(int i915)
> +{
> +       const struct intel_execution_engine2 *e;
> +
> +       __for_each_physical_engine(i915, e)
> +               basic(i915, e, false);
> +}
> +
>  igt_main
>  {
>         const struct intel_execution_engine2 *e;
> @@ -477,11 +485,14 @@ igt_main
>                         igt_fork_hang_detector(fd);
>                 }
>  
> +               igt_subtest("busy-all") {
> +                       gem_quiescent_gpu(fd);
> +                       all(fd);
> +               }

Lgtm.

> +
>                 __for_each_physical_engine(fd, e) {
>                         igt_subtest_group {
> -                               igt_subtest_f("%sbusy-%s",
> -                                             e->class == I915_ENGINE_CLASS_RENDER
> -                                             ? "basic-" : "", e->name) {
> +                               igt_subtest_f("busy-%s", e->name) {
>                                         gem_quiescent_gpu(fd);
>                                         basic(fd, e, false);
>                                 }
> diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
> index 140cbdbc66a5..24e1cb5bdaa9 100644
> --- a/tests/i915/gem_exec_basic.c
> +++ b/tests/i915/gem_exec_basic.c
> @@ -114,6 +114,30 @@ static void gtt(int fd, uint64_t flags)
>         munmap(execbuf, 4096);
>  }
>  
> +static void all(int i915)
> +{
> +       const struct intel_execution_engine2 *e;
> +
> +       __for_each_physical_engine(i915, e)
> +               noop(i915, e->flags);
> +}
> +
> +static void readonly_all(int i915)
> +{
> +       const struct intel_execution_engine2 *e;
> +
> +       __for_each_physical_engine(i915, e)
> +               readonly(i915, e->flags);
> +}
> +
> +static void gtt_all(int i915)
> +{
> +       const struct intel_execution_engine2 *e;
> +
> +       __for_each_physical_engine(i915, e)
> +               gtt(i915, e->flags);
> +}
> +
>  igt_main
>  {
>         const struct intel_execution_engine2 *e;
> @@ -126,6 +150,15 @@ igt_main
>                 igt_fork_hang_detector(fd);
>         }
>  
> +       igt_subtest("all")
> +               all(fd);
> +
> +       igt_subtest("readonly-all")
> +               readonly_all(fd);
> +
> +       igt_subtest("gtt-all")
> +               gtt_all(fd);

Ok. I'd say we only need all itself in BAT. Using readonly or gtt
pointers for params only needs irregular testing and doesn't do much for
verifying driver sanity.

> +
>         __for_each_physical_engine(fd, e) {
>                 igt_subtest_f("basic-%s", e->name)
>                         noop(fd, e->flags);
> diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
> index 40475b1ab361..03ac6dd1eee8 100644
> --- a/tests/intel-ci/fast-feedback.testlist
> +++ b/tests/intel-ci/fast-feedback.testlist
> @@ -5,7 +5,7 @@ igt@debugfs_test@read_all_entries
>  igt@gem_basic@bad-close
>  igt@gem_basic@create-close
>  igt@gem_basic@create-fd-close
> -igt@gem_busy@basic-busy-default
> +igt@gem_busy@busy-all
>  igt@gem_close_race@basic-process
>  igt@gem_close_race@basic-threads
>  igt@gem_cpu_reloc@basic
> @@ -15,27 +15,9 @@ igt@gem_ctx_exec@basic
>  igt@gem_ctx_param@basic
>  igt@gem_ctx_param@basic-default
>  igt@gem_ctx_switch@basic-default
> -igt@gem_exec_basic@basic-blt
> -igt@gem_exec_basic@basic-bsd
> -igt@gem_exec_basic@basic-bsd1
> -igt@gem_exec_basic@basic-bsd2
> -igt@gem_exec_basic@basic-default
> -igt@gem_exec_basic@basic-render
> -igt@gem_exec_basic@basic-vebox
> -igt@gem_exec_basic@gtt-blt
> -igt@gem_exec_basic@gtt-bsd
> -igt@gem_exec_basic@gtt-bsd1
> -igt@gem_exec_basic@gtt-bsd2
> -igt@gem_exec_basic@gtt-default
> -igt@gem_exec_basic@gtt-render
> -igt@gem_exec_basic@gtt-vebox
> -igt@gem_exec_basic@readonly-blt
> -igt@gem_exec_basic@readonly-bsd
> -igt@gem_exec_basic@readonly-bsd1
> -igt@gem_exec_basic@readonly-bsd2
> -igt@gem_exec_basic@readonly-default
> -igt@gem_exec_basic@readonly-render
> -igt@gem_exec_basic@readonly-vebox
> +igt@gem_exec_basic@basic-all
> +igt@gem_exec_basic@gtt-all
> +igt@gem_exec_basic@readonly-all

I'd vote for only basic-all

>  igt@gem_exec_create@basic
>  igt@gem_exec_fence@basic-busy-default
>  igt@gem_exec_fence@basic-wait-default
> @@ -74,13 +56,6 @@ igt@gem_exec_reloc@basic-write-gtt-active
>  igt@gem_exec_reloc@basic-write-read-active
>  igt@gem_exec_reloc@basic-softpin
>  igt@gem_exec_store@basic-all
> -igt@gem_exec_store@basic-blt
> -igt@gem_exec_store@basic-bsd
> -igt@gem_exec_store@basic-bsd1
> -igt@gem_exec_store@basic-bsd2
> -igt@gem_exec_store@basic-default
> -igt@gem_exec_store@basic-render
> -igt@gem_exec_store@basic-vebox
>  igt@gem_exec_suspend@basic
>  igt@gem_exec_suspend@basic-s3
>  igt@gem_exec_suspend@basic-s4-devices

Nevertheless,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] tests/i915: Restore some BAT coverage

[Andi Shyti]

Hi Tvrtko,

> +static void all(int i915)
> +{
> +	const struct intel_execution_engine2 *e;
> +
> +	__for_each_physical_engine(i915, e)
> +		basic(i915, e, false);

ops... there is an error in my previous patch.

I restored the meaining of flags because you and Chris didn't
like the boolean true/false meaning, indeed here I still check
with:

	if ((flags & HANG) == 0) {

but then I forgot to update the test cases.

This means that this line needs to be

	basic(i915, e, 0);

as the rest.

I'm sorry about the confusion! In any case,

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

Thanks,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2] tests/i915: Restore some BAT coverage

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Engine enumerated test names have changed so fast-feedback.testlist needs
to be updated. However listing all engines there won't scale. So instead
add new tests cases which iterate all engines internally.

v2:
 * Fix basic-all test name.
 * Fix params to basic (bool false to zero).
 * Use I915_EXEC_DEFAULT in has_extended_busy_ioctl to workaround stateful
   contexts for now.
 * Have only basic-all in BAT. (Chris)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
Reviewed-by: Andi Shyti <andi.shyti@intel.com> # v1
---
 tests/i915/gem_busy.c                 | 19 +++++++++++----
 tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
 tests/intel-ci/fast-feedback.testlist | 31 ++-----------------------
 3 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
index 781a3bfab1d1..f3ebb37a33b4 100644
--- a/tests/i915/gem_busy.c
+++ b/tests/i915/gem_busy.c
@@ -417,7 +417,7 @@ static bool has_semaphores(int fd)
 
 static bool has_extended_busy_ioctl(int fd)
 {
-	igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_RENDER);
+	igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_DEFAULT);
 	uint32_t read, write;
 
 	__gem_busy(fd, spin->handle, &read, &write);
@@ -460,6 +460,14 @@ static void basic(int fd, const struct intel_execution_engine2 *e, unsigned flag
 	igt_spin_free(fd, spin);
 }
 
+static void all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		basic(i915, e, 0);
+}
+
 igt_main
 {
 	const struct intel_execution_engine2 *e;
@@ -477,11 +485,14 @@ igt_main
 			igt_fork_hang_detector(fd);
 		}
 
+		igt_subtest("busy-all") {
+			gem_quiescent_gpu(fd);
+			all(fd);
+		}
+
 		__for_each_physical_engine(fd, e) {
 			igt_subtest_group {
-				igt_subtest_f("%sbusy-%s",
-					      e->class == I915_ENGINE_CLASS_RENDER
-					      ? "basic-" : "", e->name) {
+				igt_subtest_f("busy-%s", e->name) {
 					gem_quiescent_gpu(fd);
 					basic(fd, e, 0);
 				}
diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
index 140cbdbc66a5..1287860be3b1 100644
--- a/tests/i915/gem_exec_basic.c
+++ b/tests/i915/gem_exec_basic.c
@@ -114,6 +114,30 @@ static void gtt(int fd, uint64_t flags)
 	munmap(execbuf, 4096);
 }
 
+static void all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		noop(i915, e->flags);
+}
+
+static void readonly_all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		readonly(i915, e->flags);
+}
+
+static void gtt_all(int i915)
+{
+	const struct intel_execution_engine2 *e;
+
+	__for_each_physical_engine(i915, e)
+		gtt(i915, e->flags);
+}
+
 igt_main
 {
 	const struct intel_execution_engine2 *e;
@@ -126,6 +150,15 @@ igt_main
 		igt_fork_hang_detector(fd);
 	}
 
+	igt_subtest("basic-all")
+		all(fd);
+
+	igt_subtest("readonly-all")
+		readonly_all(fd);
+
+	igt_subtest("gtt-all")
+		gtt_all(fd);
+
 	__for_each_physical_engine(fd, e) {
 		igt_subtest_f("basic-%s", e->name)
 			noop(fd, e->flags);
diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
index 40475b1ab361..58e6b5c5f882 100644
--- a/tests/intel-ci/fast-feedback.testlist
+++ b/tests/intel-ci/fast-feedback.testlist
@@ -5,7 +5,7 @@ igt@debugfs_test@read_all_entries
 igt@gem_basic@bad-close
 igt@gem_basic@create-close
 igt@gem_basic@create-fd-close
-igt@gem_busy@basic-busy-default
+igt@gem_busy@busy-all
 igt@gem_close_race@basic-process
 igt@gem_close_race@basic-threads
 igt@gem_cpu_reloc@basic
@@ -15,27 +15,7 @@ igt@gem_ctx_exec@basic
 igt@gem_ctx_param@basic
 igt@gem_ctx_param@basic-default
 igt@gem_ctx_switch@basic-default
-igt@gem_exec_basic@basic-blt
-igt@gem_exec_basic@basic-bsd
-igt@gem_exec_basic@basic-bsd1
-igt@gem_exec_basic@basic-bsd2
-igt@gem_exec_basic@basic-default
-igt@gem_exec_basic@basic-render
-igt@gem_exec_basic@basic-vebox
-igt@gem_exec_basic@gtt-blt
-igt@gem_exec_basic@gtt-bsd
-igt@gem_exec_basic@gtt-bsd1
-igt@gem_exec_basic@gtt-bsd2
-igt@gem_exec_basic@gtt-default
-igt@gem_exec_basic@gtt-render
-igt@gem_exec_basic@gtt-vebox
-igt@gem_exec_basic@readonly-blt
-igt@gem_exec_basic@readonly-bsd
-igt@gem_exec_basic@readonly-bsd1
-igt@gem_exec_basic@readonly-bsd2
-igt@gem_exec_basic@readonly-default
-igt@gem_exec_basic@readonly-render
-igt@gem_exec_basic@readonly-vebox
+igt@gem_exec_basic@basic-all
 igt@gem_exec_create@basic
 igt@gem_exec_fence@basic-busy-default
 igt@gem_exec_fence@basic-wait-default
@@ -74,13 +54,6 @@ igt@gem_exec_reloc@basic-write-gtt-active
 igt@gem_exec_reloc@basic-write-read-active
 igt@gem_exec_reloc@basic-softpin
 igt@gem_exec_store@basic-all
-igt@gem_exec_store@basic-blt
-igt@gem_exec_store@basic-bsd
-igt@gem_exec_store@basic-bsd1
-igt@gem_exec_store@basic-bsd2
-igt@gem_exec_store@basic-default
-igt@gem_exec_store@basic-render
-igt@gem_exec_store@basic-vebox
 igt@gem_exec_suspend@basic
 igt@gem_exec_suspend@basic-s3
 igt@gem_exec_suspend@basic-s4-devices
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t v2] tests/i915: Restore some BAT coverage

[Tvrtko Ursulin]

On 23/05/2019 07:37, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine enumerated test names have changed so fast-feedback.testlist needs
> to be updated. However listing all engines there won't scale. So instead
> add new tests cases which iterate all engines internally.
> 
> v2:
>   * Fix basic-all test name.
>   * Fix params to basic (bool false to zero).
>   * Use I915_EXEC_DEFAULT in has_extended_busy_ioctl to workaround stateful
>     contexts for now.
>   * Have only basic-all in BAT. (Chris)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> Reviewed-by: Andi Shyti <andi.shyti@intel.com> # v1
> ---
>   tests/i915/gem_busy.c                 | 19 +++++++++++----
>   tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
>   tests/intel-ci/fast-feedback.testlist | 31 ++-----------------------
>   3 files changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> index 781a3bfab1d1..f3ebb37a33b4 100644
> --- a/tests/i915/gem_busy.c
> +++ b/tests/i915/gem_busy.c
> @@ -417,7 +417,7 @@ static bool has_semaphores(int fd)
>   
>   static bool has_extended_busy_ioctl(int fd)
>   {
> -	igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_RENDER);
> +	igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_DEFAULT);

(This fails on platforms with only rcs0 (no other engines) due context 
has a map now, and I915_EXEC_RENDER == 1 == -EINVAL.)

We need to come up with a robust and easy to remember solution for 
dealing with the fact contexts are stateful now and 
__for_each_physical_engine iterator configures the default one.

Could end game for test conversion be to stop passing in eb.flags to 
igt_spin_new and do class:instance instead? That would enable dummyload 
to unambiguously know what to use, depending on get_engines query. Until 
then it is fiddling and guess work.

But are there more tests which could be affected? Must be..

Or we drop the aspiration to support 
non-engine-discovery-and-no-context-map kernels?

Regards,

Tvrtko

>   	uint32_t read, write;
>   
>   	__gem_busy(fd, spin->handle, &read, &write);
> @@ -460,6 +460,14 @@ static void basic(int fd, const struct intel_execution_engine2 *e, unsigned flag
>   	igt_spin_free(fd, spin);
>   }
>   
> +static void all(int i915)
> +{
> +	const struct intel_execution_engine2 *e;
> +
> +	__for_each_physical_engine(i915, e)
> +		basic(i915, e, 0);
> +}
> +
>   igt_main
>   {
>   	const struct intel_execution_engine2 *e;
> @@ -477,11 +485,14 @@ igt_main
>   			igt_fork_hang_detector(fd);
>   		}
>   
> +		igt_subtest("busy-all") {
> +			gem_quiescent_gpu(fd);
> +			all(fd);
> +		}
> +
>   		__for_each_physical_engine(fd, e) {
>   			igt_subtest_group {
> -				igt_subtest_f("%sbusy-%s",
> -					      e->class == I915_ENGINE_CLASS_RENDER
> -					      ? "basic-" : "", e->name) {
> +				igt_subtest_f("busy-%s", e->name) {
>   					gem_quiescent_gpu(fd);
>   					basic(fd, e, 0);
>   				}
> diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
> index 140cbdbc66a5..1287860be3b1 100644
> --- a/tests/i915/gem_exec_basic.c
> +++ b/tests/i915/gem_exec_basic.c
> @@ -114,6 +114,30 @@ static void gtt(int fd, uint64_t flags)
>   	munmap(execbuf, 4096);
>   }
>   
> +static void all(int i915)
> +{
> +	const struct intel_execution_engine2 *e;
> +
> +	__for_each_physical_engine(i915, e)
> +		noop(i915, e->flags);
> +}
> +
> +static void readonly_all(int i915)
> +{
> +	const struct intel_execution_engine2 *e;
> +
> +	__for_each_physical_engine(i915, e)
> +		readonly(i915, e->flags);
> +}
> +
> +static void gtt_all(int i915)
> +{
> +	const struct intel_execution_engine2 *e;
> +
> +	__for_each_physical_engine(i915, e)
> +		gtt(i915, e->flags);
> +}
> +
>   igt_main
>   {
>   	const struct intel_execution_engine2 *e;
> @@ -126,6 +150,15 @@ igt_main
>   		igt_fork_hang_detector(fd);
>   	}
>   
> +	igt_subtest("basic-all")
> +		all(fd);
> +
> +	igt_subtest("readonly-all")
> +		readonly_all(fd);
> +
> +	igt_subtest("gtt-all")
> +		gtt_all(fd);
> +
>   	__for_each_physical_engine(fd, e) {
>   		igt_subtest_f("basic-%s", e->name)
>   			noop(fd, e->flags);
> diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
> index 40475b1ab361..58e6b5c5f882 100644
> --- a/tests/intel-ci/fast-feedback.testlist
> +++ b/tests/intel-ci/fast-feedback.testlist
> @@ -5,7 +5,7 @@ igt@debugfs_test@read_all_entries
>   igt@gem_basic@bad-close
>   igt@gem_basic@create-close
>   igt@gem_basic@create-fd-close
> -igt@gem_busy@basic-busy-default
> +igt@gem_busy@busy-all
>   igt@gem_close_race@basic-process
>   igt@gem_close_race@basic-threads
>   igt@gem_cpu_reloc@basic
> @@ -15,27 +15,7 @@ igt@gem_ctx_exec@basic
>   igt@gem_ctx_param@basic
>   igt@gem_ctx_param@basic-default
>   igt@gem_ctx_switch@basic-default
> -igt@gem_exec_basic@basic-blt
> -igt@gem_exec_basic@basic-bsd
> -igt@gem_exec_basic@basic-bsd1
> -igt@gem_exec_basic@basic-bsd2
> -igt@gem_exec_basic@basic-default
> -igt@gem_exec_basic@basic-render
> -igt@gem_exec_basic@basic-vebox
> -igt@gem_exec_basic@gtt-blt
> -igt@gem_exec_basic@gtt-bsd
> -igt@gem_exec_basic@gtt-bsd1
> -igt@gem_exec_basic@gtt-bsd2
> -igt@gem_exec_basic@gtt-default
> -igt@gem_exec_basic@gtt-render
> -igt@gem_exec_basic@gtt-vebox
> -igt@gem_exec_basic@readonly-blt
> -igt@gem_exec_basic@readonly-bsd
> -igt@gem_exec_basic@readonly-bsd1
> -igt@gem_exec_basic@readonly-bsd2
> -igt@gem_exec_basic@readonly-default
> -igt@gem_exec_basic@readonly-render
> -igt@gem_exec_basic@readonly-vebox
> +igt@gem_exec_basic@basic-all
>   igt@gem_exec_create@basic
>   igt@gem_exec_fence@basic-busy-default
>   igt@gem_exec_fence@basic-wait-default
> @@ -74,13 +54,6 @@ igt@gem_exec_reloc@basic-write-gtt-active
>   igt@gem_exec_reloc@basic-write-read-active
>   igt@gem_exec_reloc@basic-softpin
>   igt@gem_exec_store@basic-all
> -igt@gem_exec_store@basic-blt
> -igt@gem_exec_store@basic-bsd
> -igt@gem_exec_store@basic-bsd1
> -igt@gem_exec_store@basic-bsd2
> -igt@gem_exec_store@basic-default
> -igt@gem_exec_store@basic-render
> -igt@gem_exec_store@basic-vebox
>   igt@gem_exec_suspend@basic
>   igt@gem_exec_suspend@basic-s3
>   igt@gem_exec_suspend@basic-s4-devices
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t v2] tests/i915: Restore some BAT coverage

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-23 07:43:12)
> 
> On 23/05/2019 07:37, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Engine enumerated test names have changed so fast-feedback.testlist needs
> > to be updated. However listing all engines there won't scale. So instead
> > add new tests cases which iterate all engines internally.
> > 
> > v2:
> >   * Fix basic-all test name.
> >   * Fix params to basic (bool false to zero).
> >   * Use I915_EXEC_DEFAULT in has_extended_busy_ioctl to workaround stateful
> >     contexts for now.
> >   * Have only basic-all in BAT. (Chris)
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> > Reviewed-by: Andi Shyti <andi.shyti@intel.com> # v1
> > ---
> >   tests/i915/gem_busy.c                 | 19 +++++++++++----
> >   tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
> >   tests/intel-ci/fast-feedback.testlist | 31 ++-----------------------
> >   3 files changed, 50 insertions(+), 33 deletions(-)
> > 
> > diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> > index 781a3bfab1d1..f3ebb37a33b4 100644
> > --- a/tests/i915/gem_busy.c
> > +++ b/tests/i915/gem_busy.c
> > @@ -417,7 +417,7 @@ static bool has_semaphores(int fd)
> >   
> >   static bool has_extended_busy_ioctl(int fd)
> >   {
> > -     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_RENDER);
> > +     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_DEFAULT);
> 
> (This fails on platforms with only rcs0 (no other engines) due context 
> has a map now, and I915_EXEC_RENDER == 1 == -EINVAL.)

In which case, it probably should be a plain 0 as it no longer has the
old EXEC_RING semantics but is just an index, i.e. igt_spin_new(fd);

> We need to come up with a robust and easy to remember solution for 
> dealing with the fact contexts are stateful now and 
> __for_each_physical_engine iterator configures the default one.
> 
> Could end game for test conversion be to stop passing in eb.flags to 
> igt_spin_new and do class:instance instead? That would enable dummyload 
> to unambiguously know what to use, depending on get_engines query.

Speak to Mika, we abuse igt_spin_t much more by resubmitting the same
dummyload to multiple engines and contexts.

> Until 
> then it is fiddling and guess work.

Hmm, but it shouldn't be. We pass both context and engine to
igt_spin_new(). It is fully under the caller's control. What has
happened is that we've changed the caller's context...

> But are there more tests which could be affected? Must be..
> 
> Or we drop the aspiration to support 
> non-engine-discovery-and-no-context-map kernels?

igt must be able to run on linus and drm-tip, at the very least.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2] tests/i915: Restore some BAT coverage

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-23 07:37:01)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Engine enumerated test names have changed so fast-feedback.testlist needs
> to be updated. However listing all engines there won't scale. So instead
> add new tests cases which iterate all engines internally.
> 
> v2:
>  * Fix basic-all test name.
>  * Fix params to basic (bool false to zero).
>  * Use I915_EXEC_DEFAULT in has_extended_busy_ioctl to workaround stateful
>    contexts for now.
>  * Have only basic-all in BAT. (Chris)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> Reviewed-by: Andi Shyti <andi.shyti@intel.com> # v1
> ---
>  tests/i915/gem_busy.c                 | 19 +++++++++++----
>  tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
>  tests/intel-ci/fast-feedback.testlist | 31 ++-----------------------
>  3 files changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> index 781a3bfab1d1..f3ebb37a33b4 100644
> --- a/tests/i915/gem_busy.c
> +++ b/tests/i915/gem_busy.c
> @@ -417,7 +417,7 @@ static bool has_semaphores(int fd)
>  
>  static bool has_extended_busy_ioctl(int fd)
>  {
> -       igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_RENDER);
> +       igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_DEFAULT);

I vote for igt_spin_t *spin = igt_spin_new(fd); for ultra-funkiness.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t v2] tests/i915: Restore some BAT coverage

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Tvrtko Ursulin (2019-05-23 07:43:12)
>> 
>> On 23/05/2019 07:37, Tvrtko Ursulin wrote:
>> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> > 
>> > Engine enumerated test names have changed so fast-feedback.testlist needs
>> > to be updated. However listing all engines there won't scale. So instead
>> > add new tests cases which iterate all engines internally.
>> > 
>> > v2:
>> >   * Fix basic-all test name.
>> >   * Fix params to basic (bool false to zero).
>> >   * Use I915_EXEC_DEFAULT in has_extended_busy_ioctl to workaround stateful
>> >     contexts for now.
>> >   * Have only basic-all in BAT. (Chris)
>> > 
>> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
>> > Reviewed-by: Andi Shyti <andi.shyti@intel.com> # v1
>> > ---
>> >   tests/i915/gem_busy.c                 | 19 +++++++++++----
>> >   tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
>> >   tests/intel-ci/fast-feedback.testlist | 31 ++-----------------------
>> >   3 files changed, 50 insertions(+), 33 deletions(-)
>> > 
>> > diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
>> > index 781a3bfab1d1..f3ebb37a33b4 100644
>> > --- a/tests/i915/gem_busy.c
>> > +++ b/tests/i915/gem_busy.c
>> > @@ -417,7 +417,7 @@ static bool has_semaphores(int fd)
>> >   
>> >   static bool has_extended_busy_ioctl(int fd)
>> >   {
>> > -     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_RENDER);
>> > +     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_DEFAULT);
>> 
>> (This fails on platforms with only rcs0 (no other engines) due context 
>> has a map now, and I915_EXEC_RENDER == 1 == -EINVAL.)
>
> In which case, it probably should be a plain 0 as it no longer has the
> old EXEC_RING semantics but is just an index, i.e. igt_spin_new(fd);
>
>> We need to come up with a robust and easy to remember solution for 
>> dealing with the fact contexts are stateful now and 
>> __for_each_physical_engine iterator configures the default one.
>> 
>> Could end game for test conversion be to stop passing in eb.flags to 
>> igt_spin_new and do class:instance instead? That would enable dummyload 
>> to unambiguously know what to use, depending on get_engines query.
>
> Speak to Mika, we abuse igt_spin_t much more by resubmitting the same
> dummyload to multiple engines and contexts.

I have wandered there yes. The reason for this abuse is still unclear.
We could do a clearer interface for creating spinners but is the
reusage due to latency concerns?

-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t v2] tests/i915: Restore some BAT coverage

[Chris Wilson]

Quoting Mika Kuoppala (2019-05-23 13:02:30)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Tvrtko Ursulin (2019-05-23 07:43:12)
> >> 
> >> On 23/05/2019 07:37, Tvrtko Ursulin wrote:
> >> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> > 
> >> > Engine enumerated test names have changed so fast-feedback.testlist needs
> >> > to be updated. However listing all engines there won't scale. So instead
> >> > add new tests cases which iterate all engines internally.
> >> > 
> >> > v2:
> >> >   * Fix basic-all test name.
> >> >   * Fix params to basic (bool false to zero).
> >> >   * Use I915_EXEC_DEFAULT in has_extended_busy_ioctl to workaround stateful
> >> >     contexts for now.
> >> >   * Have only basic-all in BAT. (Chris)
> >> > 
> >> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> # v1
> >> > Reviewed-by: Andi Shyti <andi.shyti@intel.com> # v1
> >> > ---
> >> >   tests/i915/gem_busy.c                 | 19 +++++++++++----
> >> >   tests/i915/gem_exec_basic.c           | 33 +++++++++++++++++++++++++++
> >> >   tests/intel-ci/fast-feedback.testlist | 31 ++-----------------------
> >> >   3 files changed, 50 insertions(+), 33 deletions(-)
> >> > 
> >> > diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> >> > index 781a3bfab1d1..f3ebb37a33b4 100644
> >> > --- a/tests/i915/gem_busy.c
> >> > +++ b/tests/i915/gem_busy.c
> >> > @@ -417,7 +417,7 @@ static bool has_semaphores(int fd)
> >> >   
> >> >   static bool has_extended_busy_ioctl(int fd)
> >> >   {
> >> > -     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_RENDER);
> >> > +     igt_spin_t *spin = igt_spin_new(fd, .engine = I915_EXEC_DEFAULT);
> >> 
> >> (This fails on platforms with only rcs0 (no other engines) due context 
> >> has a map now, and I915_EXEC_RENDER == 1 == -EINVAL.)
> >
> > In which case, it probably should be a plain 0 as it no longer has the
> > old EXEC_RING semantics but is just an index, i.e. igt_spin_new(fd);
> >
> >> We need to come up with a robust and easy to remember solution for 
> >> dealing with the fact contexts are stateful now and 
> >> __for_each_physical_engine iterator configures the default one.
> >> 
> >> Could end game for test conversion be to stop passing in eb.flags to 
> >> igt_spin_new and do class:instance instead? That would enable dummyload 
> >> to unambiguously know what to use, depending on get_engines query.
> >
> > Speak to Mika, we abuse igt_spin_t much more by resubmitting the same
> > dummyload to multiple engines and contexts.
> 
> I have wandered there yes. The reason for this abuse is still unclear.
> We could do a clearer interface for creating spinners but is the
> reusage due to latency concerns?

No, just because we are lazy and creating a clean, clear interface takes
time and lots of effort. So we excuse ourselves to get something that
does the job and once we know what we want, we tell the next fool to
come along to fix it neatly. :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx