[igt-dev] [PATCH RFC i-g-t v1 1/2] tests/i915/gem_basic: convert to multithreaded multi-GPUs

[Mauro Carvalho Chehab]

Hi Kamil,

On Wed, 21 Sep 2022 12:29:39 +0200
Kamil Konieczny <kamil.konieczny at linux.intel.com> wrote:

> This will convert basic tests to run on multi-GPU hardware
> configuration. It will take effect when --device option will be
> used and at least two devices will be found with a filter. When
> there are no filtered ones or there is only one, tests will
> fall through to old run.
> 
> One method for use will be to extend filtered devices with '*'
> or regex:
> 
> sudo ./gem_basic --device=pci:vendor=intel,device=discrete,card=*
> 
> Pros: no change in tests names, no new tests added. Use restricted
> to runs with suitable hw config.
> 
> Cons: it can be hard to replicate failed tests or it can break
> igt-framework in compliated use cases. It is also not clear how
> to change this solution into clean macro for reuse.
> 
> Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Cc: Andi Shyti <andi.shyti at linux.intel.com>
> Cc: Anna Karas <anna.karas at intel.com>
> Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
> Cc: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab at linux.intel.com>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> ---
>  tests/i915/gem_basic.c | 71 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/i915/gem_basic.c b/tests/i915/gem_basic.c
> index 17ae190c..66413e60 100644
> --- a/tests/i915/gem_basic.c
> +++ b/tests/i915/gem_basic.c
> @@ -38,6 +38,7 @@
>  
>  #include "drm.h"
>  #include "i915/gem_create.h"
> +#include "igt_device_scan.h"
>  
>  IGT_TEST_DESCRIPTION("Tests basic gem_create and gem_close IOCTLs");
>  
> @@ -48,6 +49,7 @@ test_bad_close(int fd)
>  	int ret;
>  
>  	igt_info("Testing error return on bad close ioctl.\n");
> +	igt_info("%s: pid: %d, tid: %lu, CPU: %d\n", __func__, getpid(), pthread_self(), sched_getcpu());

Hmm... perhaps we should add pid at igt_info(), for multi-GPU scenarios, e. g.
when IGT was asked to run with more than one GPUs.

Not sure if it makes sense to display tid/CPU here.

>  
>  	close_bo.handle = 0x10101010;
>  	ret = ioctl(fd, DRM_IOCTL_GEM_CLOSE, &close_bo);
> @@ -61,6 +63,7 @@ test_create_close(int fd)
>  	uint32_t handle;
>  
>  	igt_info("Testing creating and closing an object.\n");
> +	igt_info("%s: pid: %d, tid: %lu, CPU: %d\n", __func__, getpid(), pthread_self(), sched_getcpu());
>  
>  	handle = gem_create(fd, 16*1024);
>  
> @@ -71,6 +74,7 @@ static void
>  test_create_fd_close(int fd)
>  {
>  	igt_info("Testing closing with an object allocated.\n");
> +	igt_info("%s: pid: %d, tid: %lu, CPU: %d\n", __func__, getpid(), pthread_self(), sched_getcpu());
>  
>  	gem_create(fd, 16*1024);
>  	/* leak it */
> @@ -78,6 +82,49 @@ test_create_fd_close(int fd)
>  	close(fd);
>  }
>  
> +struct multi_gpu_data {
> +	int in_fd;
> +	void (*fn)(int fd);
> +};
> +
> +static void *run_on_one_gpu(void *pdata)
> +{
> +	struct multi_gpu_data *pgpu = pdata;
> +
> +	(pgpu->fn)(pgpu->in_fd);
> +
> +	return NULL;
> +}
> +
> +static void run_on_gpus(void (*fn)(int))
> +{
> +	struct multi_gpu_data *in_arr;
> +	pthread_t *pth;
> +	int size = igt_device_filter_count();
> +
> +	in_arr = calloc(size, sizeof(struct multi_gpu_data));
> +	igt_assert(!in_arr);
> +
> +	pth = calloc(size, sizeof(pthread_t));
> +	igt_assert(!pth);
> +
> +	igt_info("pid: %d, running on %d GPUs\n", getpid(), size);
> +
> +	for (int i = 0; i < size; ++i) {
> +		in_arr[i].in_fd = __drm_open_driver_another(i, DRIVER_ANY);
> +		in_arr[i].fn = fn;
> +	}
> +
> +	for (int i = 0; i < size; ++i)
> +		pthread_create(pth + i, 0, run_on_one_gpu, in_arr + i);
> +
> +	for (int i = 0; i < size; ++i)
> +		pthread_join(pth[i], NULL);
> +
> +	free(in_arr);
> +	free(pth);
> +}
> +

Interesting approach, but having to add the same function on all tests that
will run with multi-GPU seems too much work. We need to add the logic at
the library, if possible using some macro/inline based solution, as Anna
proposed.

>  int fd;
>  
>  igt_main
> @@ -86,14 +133,26 @@ igt_main
>  		fd = drm_open_driver(DRIVER_INTEL);
>  
>  	igt_describe("Verify that gem_close fails with bad params.");
> -	igt_subtest("bad-close")
> -		test_bad_close(fd);
> +	igt_subtest("bad-close") {
> +		if (igt_device_filter_count() < 2)
> +			test_bad_close(fd);
> +		else
> +			run_on_gpus(test_bad_close);
> +	}
>  
>  	igt_describe("Verify basic functionality of gem_create and gem_close.");
> -	igt_subtest("create-close")
> -		test_create_close(fd);
> +	igt_subtest("create-close") {
> +		if (igt_device_filter_count() < 2)
> +			test_create_close(fd);
> +		else
> +			run_on_gpus(test_create_close);
> +	}
>  
>  	igt_describe("Verify that closing drm driver is possible with opened gem object.");
> -	igt_subtest("create-fd-close")
> -		test_create_fd_close(fd);
> +	igt_subtest("create-fd-close") {
> +		if (igt_device_filter_count() < 2)
> +			test_create_fd_close(fd);
> +		else
> +			run_on_gpus(test_create_fd_close);
> +	}
>  }

Regards,
Mauro