Re: [PATCH i-g-t v13 5/5] tests/intel/gem_exec_gttfill: simplify multiGPU subtest

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

Hi Kamil,

The series now looks good to me, however, I still have one doubt.

On Monday, 18 March 2024 14:26:23 CET Kamil Konieczny wrote:
> Simplify multi-GPU subtest with the help of new multigpu library.
> 
> v10: remove igt_require_multigpu() as it checks filters but we
>   want to use legacy opens (Kamil), correct Cc name (Zbigniew)
> 
> Cc: "Zbigniew Kempczyński" <zbigniew.kempczynski@intel.com>
> Cc: "Dominik Karol Piątkowski" <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>  tests/intel/gem_exec_gttfill.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/intel/gem_exec_gttfill.c b/tests/intel/gem_exec_gttfill.c
> index b1063437b..1eb7325bb 100644
> --- a/tests/intel/gem_exec_gttfill.c
> +++ b/tests/intel/gem_exec_gttfill.c
> @@ -24,8 +24,8 @@
>  #include "i915/gem.h"
>  #include "i915/gem_create.h"
>  #include "igt.h"
> -#include "igt_device_scan.h"
>  #include "igt_rand.h"
> +#include "igt_multigpu.h"
>  /**
>   * TEST: gem exec gttfill
>   * Description: Fill the GTT with batches.
> @@ -251,7 +251,7 @@ igt_main
>  {
>  	const struct intel_execution_engine2 *e;
>  	const intel_ctx_t *ctx;
> -	int i915 = -1, gpu_count;
> +	int i915 = -1;
>  
>  	igt_fixture {
>  		i915 = drm_open_driver(DRIVER_INTEL);
> @@ -285,17 +285,11 @@ igt_main
>  	igt_fixture {
>  		igt_stop_hang_detector();
>  		intel_ctx_destroy(i915, ctx);
> -		// prepare multigpu tests
> -		gpu_count = igt_device_filter_count();
>  	}
>  
>  	igt_subtest("multigpu-basic") { /* run on two or more discrete cards */
> -		igt_require(gpu_count > 1);
> -		igt_multi_fork(child, gpu_count) {
> -			int g_fd;
> +		igt_multi_fork_foreach_gpu(g_fd, gpu_idx, DRIVER_INTEL) {
>  			// prepare
> -			g_fd = __drm_open_driver_another(child, DRIVER_INTEL);
> -			igt_assert(g_fd >= 0);

Since igt_multi_fork_foreach_gpu() now calls multigpu_open_another() which 
introduces igt_require_gem() into the processing path, you are effectively 
changing prerequisites before calling intel_ctx_create_all_physical().  Two 
questions arise:
1) Was that missing requirement for GEM on Intel intentional here, or was that 
   a bug which you are now fixing while being at it?
2) Do we expect all future users of igt_multi_fork_foreach_gpu() to always 
   require GEM on Intel?  If you think so then can you please explain why?

Wouldn't igt_multi_fork_foreach_gpu() be more versatile if it called 
drm_open_driver_another() instead, without that skip on no GEM?  Wouldn't it 
be more clear if tests were explicitly taking care as needed of such scenario 
specific prerequisites?

If you still see benefits from using multigpu_open_another() here then could 
we please plan for introducing two variants of igt_multi_fork_foreach_gpu(), 
one that doesn't skip and the other that skips on no GEM, unless you are sure 
we don't need the former?  Maybe the latter could be implemented as a wrapper 
around the former, while the opposite seems not doable.

Thanks,
Janusz

>  			ctx = intel_ctx_create_all_physical(g_fd);
>  			igt_fork_hang_detector(g_fd);
>  			// subtest
> @@ -303,7 +297,6 @@ igt_main
>  			// release resources
>  			igt_stop_hang_detector();
>  			intel_ctx_destroy(g_fd, ctx);
> -			drm_close_driver(g_fd);
>  		}
>  
>  		igt_waitchildren();
>