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

[Kamil Konieczny]

Hi Janusz,

On 2024-03-19 at 10:36:49 +0100, Janusz Krzysztofik wrote:
> 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 at intel.com>
> > Cc: "Dominik Karol Piątkowski" <dominik.karol.piatkowski at intel.com>
> > Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > Signed-off-by: Kamil Konieczny <kamil.konieczny at 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?

Looks like a bug in multigpu subtest, there is igt_require_gem() in fixture
in beginning of igt_main().

> 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?

igt_require_gem() is a way to idle GPU and reset it to defaults on i915.

> 
> 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?
> 

You are right, it will be better, I will drop multigpu_open_another(),
that way igt_require_gem() should be used at places where test would need it.

Regards,
Kamil

> 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();
> > 
> 
> 
> 
>