[Intel-gfx] [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 at 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 at 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 at debugfs_test@read_all_entries
>  igt at gem_basic@bad-close
>  igt at gem_basic@create-close
>  igt at gem_basic@create-fd-close
> -igt at gem_busy@basic-busy-default
> +igt at gem_busy@busy-all
>  igt at gem_close_race@basic-process
>  igt at gem_close_race@basic-threads
>  igt at gem_cpu_reloc@basic
> @@ -15,27 +15,9 @@ igt at gem_ctx_exec@basic
>  igt at gem_ctx_param@basic
>  igt at gem_ctx_param@basic-default
>  igt at gem_ctx_switch@basic-default
> -igt at gem_exec_basic@basic-blt
> -igt at gem_exec_basic@basic-bsd
> -igt at gem_exec_basic@basic-bsd1
> -igt at gem_exec_basic@basic-bsd2
> -igt at gem_exec_basic@basic-default
> -igt at gem_exec_basic@basic-render
> -igt at gem_exec_basic@basic-vebox
> -igt at gem_exec_basic@gtt-blt
> -igt at gem_exec_basic@gtt-bsd
> -igt at gem_exec_basic@gtt-bsd1
> -igt at gem_exec_basic@gtt-bsd2
> -igt at gem_exec_basic@gtt-default
> -igt at gem_exec_basic@gtt-render
> -igt at gem_exec_basic@gtt-vebox
> -igt at gem_exec_basic@readonly-blt
> -igt at gem_exec_basic@readonly-bsd
> -igt at gem_exec_basic@readonly-bsd1
> -igt at gem_exec_basic@readonly-bsd2
> -igt at gem_exec_basic@readonly-default
> -igt at gem_exec_basic@readonly-render
> -igt at gem_exec_basic@readonly-vebox
> +igt at gem_exec_basic@basic-all
> +igt at gem_exec_basic@gtt-all
> +igt at gem_exec_basic@readonly-all

I'd vote for only basic-all

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

Nevertheless,
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris