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

Thu Jan 25 17:08:25 UTC 2024

Hi Piatkowski,,
On 2024-01-25 at 14:11:30 +0000, Piatkowski, Dominik Karol wrote:
> 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?

I will send new version soon, I hope it will be clear then.

Thanks for review,
Kamil

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