[PATCH i-g-t v2 6/8] lib/intel_multigpu: Add xe_multi_fork_foreach_gpu

Thu Jan 25 14:11:30 UTC 2024

Hi Kamil,

> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Sent: Thursday, January 25, 2024 2:55 PM
> To: igt-dev at lists.freedesktop.org
> Cc: Piatkowski, Dominik Karol <dominik.karol.piatkowski at intel.com>
> Subject: Re: [PATCH i-g-t v2 6/8] lib/intel_multigpu: Add
> xe_multi_fork_foreach_gpu
> 
> Hi Dominik,
> 
> On 2024-01-25 at 07:43:35 +0000, Piatkowski, Dominik Karol wrote:
> > Hi Kamil,
> >
> > > -----Original Message-----
> > > From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of
> > > Kamil Konieczny
> > > Sent: Wednesday, January 24, 2024 9:42 PM
> > > To: igt-dev at lists.freedesktop.org
> > > Subject: [PATCH i-g-t v2 6/8] lib/intel_multigpu: Add
> > > xe_multi_fork_foreach_gpu
> > >
> > > Create macro for testing Xe driver in multigpu scenarios with the
> > > help of few new multigpu functions. Also while at this, add
> > > documentation for public functions and use new helper in existing multi-
> gpu macro.
> > >
> > > v2: corrected description (Kamil), rebased on corrected patches
> > > after Dominik review
> > >
> > > Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > ---
> > >  lib/intel_multigpu.c | 94
> > > ++++++++++++++++++++++++++++++++++++++++++--
> > >  lib/intel_multigpu.h | 12 +++++-
> > >  2 files changed, 101 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/lib/intel_multigpu.c b/lib/intel_multigpu.c index
> > > 0e76d8aa3..e631ab0ed 100644
> > > --- a/lib/intel_multigpu.c
> > > +++ b/lib/intel_multigpu.c
> > > @@ -4,10 +4,18 @@
> > >   */
> > >
> > >  #include "drmtest.h"
> > > +#include "i915/gem.h"
> > >  #include "igt_core.h"
> > >  #include "igt_device_scan.h"
> > >  #include "intel_multigpu.h"
> > >
> > > +/**
> > > + * gem_multigpu_count_class:
> > > + * @class: chipset, e.g. DRIVER_XE or DRIVER_INTEL
> > > + *
> > > + * Function counts number of GPU cards with the help of opening all of
> them.
> > > + * Returns: number of GPUs cards found  */
> > >  int gem_multigpu_count_class(int class)  {
> > >  	int count = 0;
> > > @@ -18,7 +26,7 @@ int gem_multigpu_count_class(int class)
> > >  	return count;
> > >  }
> > >
> > > -void gem_require_multigpu(int count)
> > > +static void skip_on_sigle_gpu(int count)
> >
> > Nitpick: typo
> > I guess it should be named "skip_on_single_gpu".
> >
> 
> Good point, I missed that, will fix.
> 
> > >  {
> > >  	struct igt_devices_print_format fmt = {
> > >  		.type = IGT_PRINT_SIMPLE,
> > > @@ -26,9 +34,6 @@ void gem_require_multigpu(int count)
> > >  	};
> > >  	int devices;
> > >
> > > -	if (igt_device_filter_count() >= count)
> > > -		return;
> > > -
> >
> > As this conditional return was moved to gem_require_multigpu(), that
> > function will work correctly. Although, calling skip_on_single_gpu()
> > directly will always cause a skip due to always reachable igt_skip.
> > I suggest changing igt_skip to igt_skip_on to avoid unexpected
> > behavior in the future.
> 
> What about changing this into print-only function and moving skips directly in
> both functions?

Can you describe it further?

> 
> >
> > Additionally, skip_on_single_gpu can be refactored to not take any
> > arguments, as we already get the number of gpus present in the system,
> > and taking required count of gpus is not needed (it is always >1).
> > The idea is to use igt_skip_on when devices < 2.
> >
> 
> It was an optimization, I was thinking about other direction, adding one more
> parameter to it and printing:
> 
> number of GPUs wanted
> what number of GPUs program got
> number of GPUs after PCI bus scan
> 
> What do you think?

Ok, but then skip_on_single_gpu won't hold to its name, unless you mean the change in gem_require_multigpu. Please further describe your idea.

Dominik Karol

> 
> > >  	igt_info("PCI devices available in the system:\n");
> > >
> > >  	igt_devices_scan(true);
> > > @@ -37,3 +42,84 @@ void gem_require_multigpu(int count)
> > >
> > >  	igt_skip("Test requires at least %d GPUs, %d available.\n", count,
> > > devices);  }
> > > +
> > > +/**
> > > + * gem_require_multigpu:
> > > + * @count: minimum number of GPUs required
> > > + *
> > > + * Function checks number of filtered GPU cards.
> > > + * On error skips and prints available GPUs found on PCI bus.
> > > + */
> > > +void gem_require_multigpu(int count) {
> > > +	if (igt_device_filter_count() >= count)
> > > +		return;
> > > +
> > > +	skip_on_sigle_gpu(count);
> > > +}
> > > +
> > > +/**
> > > + * multigpu_aquire_view:
> > > + * @gpus_wanted: minimum number of GPUs required
> > > + * @chipset: chipset, e.g. DRIVER_XE or DRIVER_INTEL
> > > + *
> > > + * Function prepares filtered view for GPU cards with given chipset.
> > > + * On error skips and prints available GPUs found on PCI bus.
> > > + * Returns: number of GPUs found
> > > + */
> > > +int multigpu_aquire_view(int gpus_wanted, unsigned int chipset) {
> >
> > Nitpick: typo
> > I guess it should be named "multigpu_acquire_view" (in function name and
> documentation).
> >
> 
> I will fix this,
> 
> Regards,
> Kamil
> 
> > Dominik Karol
> >
> > > +	int gpu_count = drm_prepare_filtered_multigpu(chipset);
> > > +
> > > +	igt_assert(gpus_wanted > 1);
> > > +	if (gpu_count < gpus_wanted)
> > > +		skip_on_sigle_gpu(gpus_wanted);
> > > +
> > > +	return gpu_count;
> > > +}
> > > +
> > > +/**
> > > + * multigpu_open_another:
> > > + * @idx: index of GPU card, starting from 0
> > > + * @chipset: chipset, e.g. DRIVER_XE or DRIVER_INTEL
> > > + *
> > > + * Function opens GPU card with drm_open_driver_another(). For i915
> > > +it checks
> > > + * if opened card is functional. Skips on errors.
> > > + * Returns: opened fd for @idx card  */ int
> > > +multigpu_open_another(int idx, unsigned int chipset) {
> > > +	int fd;
> > > +
> > > +	igt_assert(idx >= 0);
> > > +	fd = drm_open_driver_another(idx, chipset);
> > > +
> > > +	igt_require(fd != -1);
> > > +	if (chipset == DRIVER_INTEL)
> > > +		igt_require_gem(fd);
> > > +
> > > +	return fd;
> > > +}
> > > +
> > > +/**
> > > + * multigpu_open_filtered_card:
> > > + * @idx: index of GPU card, starting from 0
> > > + * @chipset: chipset, e.g. DRIVER_XE or DRIVER_INTEL
> > > + *
> > > + * Function opens GPU card prepared with filtered view. It also
> > > +checks if
> > > + * opened card agrees with desired chipset or checks if opened card
> > > +is
> > > + * functional. Skips on errors.
> > > + * Returns: opened fd for @idx card  */ int
> > > +multigpu_open_filtered_card(int idx, unsigned int chipset) {
> > > +	int fd = drm_open_filtered_card(idx);
> > > +
> > > +	igt_require(fd != -1);
> > > +	if (chipset == DRIVER_XE)
> > > +		igt_require_xe(fd);
> > > +	else if (chipset == DRIVER_INTEL)
> > > +		igt_require_gem(fd);
> > > +
> > > +	return fd;
> > > +}
> > > diff --git a/lib/intel_multigpu.h b/lib/intel_multigpu.h index
> > > 0ebc73e4a..0032a619e 100644
> > > --- a/lib/intel_multigpu.h
> > > +++ b/lib/intel_multigpu.h
> > > @@ -12,6 +12,10 @@
> > >  void gem_require_multigpu(int count);  int
> > > gem_multigpu_count_class(int class);
> > >
> > > +int multigpu_aquire_view(int min_gpus_wanted, unsigned int
> > > +chipset); int multigpu_open_another(int idx, unsigned int chipset);
> > > +int multigpu_open_filtered_card(int idx, unsigned int chipset);
> > > +
> > >  #define igt_foreach_gpu(fd__, id__) \
> > >  	for (int igt_unique(i) = 0, fd__; \
> > >  		(fd__ = __drm_open_driver_another(igt_unique(i)++, id__)) >=
> 0; \
> > > @@ -19,8 +23,14 @@ int gem_multigpu_count_class(int class);
> > >
> > >  #define igt_multi_fork_foreach_gpu(__fd, __id) \
> > >  	igt_multi_fork(igt_unique(__i), gem_multigpu_count_class(__id)) \
> > > -		for (int __fd = drm_open_driver_another(igt_unique(__i),
> > > __id); \
> > > +		for (int __fd = multigpu_open_another(igt_unique(__i), __id);
> > > \
> > >  			__fd >= 0; \
> > >  			close(__fd), __fd = -1) \
> > >
> > > +#define xe_multi_fork_foreach_gpu(__fd, __gpu_index) \
> > > +	igt_multi_fork(__gpu_index, multigpu_aquire_view(2, DRIVER_XE)) \
> > > +		for (int __fd = multigpu_open_filtered_card(__gpu_index,
> > > DRIVER_XE); \
> > > +			__fd >= 0; \
> > > +			drm_close_driver(__fd), __fd = -1) \
> > > +
> > >  #endif /* __INTEL_MULTIGPU_H */
> > > --
> > > 2.42.0
> >