[PATCH i-g-t v11 3/6] lib/intel_multigpu: Introduce library for multi-GPU scenarios
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Wed Mar 6 19:03:03 UTC 2024
Hi Kamil,
On Wednesday, 6 March 2024 14:31:55 CET Kamil Konieczny wrote:
> From: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>
>
> Implemented gem_require_multigpu in order to replace
> igt_require(gpu_count > 1), as well as printing available
> PCI devices if requirement fails.
>
> Introduced gem_multigpu_count_class function that returns an actual
> number of GPUs present in system, which allows for writing multi-GPU
> test scenarios that does not require filter
> --device pci:vendor=intel,device=discrete,card=all
> to run as intended. Based on patch by Chris Wilson.
>
> Introduced igt_multi_fork_foreach_gpu macro that helps with
> writing multi-GPU test scenarios in idiomatic form:
>
> igt_multi_fork_foreach_gpu(i915, DRIVER_INTEL)
> test_function(i915);
> igt_waitchildren();
>
> v10: squashed two commits which introduce multigpu functions (Zbigniew)
> renamed __id and id__ into __chipset/chipset__ (Zbigniew)
> used __drm_close_driver() instead of close() in first macro (Kamil)
> v11: added functions descriptions (Janusz)
>
> Cc: "Zbigniew Kempczyński" <zbigniew.kempczynski at intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> Signed-off-by: "Dominik Karol Piątkowski" <dominik.karol.piatkowski at intel.com>
> [Kamil: fixed whitespace and tabs, moved to lib/intel_multigpu.*]
> Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> ---
> lib/intel_multigpu.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
> lib/intel_multigpu.h | 26 +++++++++++++++++++
Are the new functions introduced with this patch really Intel specific? I
think they're not. Why don't we call this source file igt_multigpu.c (or
drm_multigpu.c)?
> lib/meson.build | 1 +
> 3 files changed, 89 insertions(+)
> create mode 100644 lib/intel_multigpu.c
> create mode 100644 lib/intel_multigpu.h
>
> diff --git a/lib/intel_multigpu.c b/lib/intel_multigpu.c
> new file mode 100644
> index 000000000..c4e7cf542
> --- /dev/null
> +++ b/lib/intel_multigpu.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include "drmtest.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)
Also, these functions don't look gem specific, couldn't we use more generic
names like igt_* (or drm_*) for them? After all, below you use igt_ prefix in
names of related macros.
> +{
> + int count = 0;
> +
> + igt_foreach_gpu(fd, class)
> + count++;
> +
> + return count;
> +}
> +
> +static void print_gpus(int count, int gpu_num)
> +{
> + struct igt_devices_print_format fmt = {
> + .type = IGT_PRINT_SIMPLE,
> + .option = IGT_PRINT_PCI,
> + };
> + int devices;
> +
> + igt_info("PCI devices available in the system:\n");
> +
> + igt_devices_scan(true);
> + devices = igt_device_filter_pci();
> + igt_devices_print(&fmt);
> +
> + igt_info("Test requires at least %d GPUs, got %d, available %d\n", count, gpu_num, devices);
> +}
> +
> +/**
> + * gem_require_multigpu:
> + * @count: minimum number of GPUs required found with filters
> + *
> + * Function checks number of filtered GPU cards.
> + * On error prints available GPUs found on PCI bus and skips.
> + */
> +void gem_require_multigpu(int gpus_wanted)
> +{
> + int gpu_filters = igt_device_filter_count();
> +
> + if (gpu_filters >= gpus_wanted)
> + return;
> +
> + print_gpus(gpus_wanted, gpu_filters);
> + igt_skip_on(gpu_filters < gpus_wanted);
How about moving the trailing igt_info() message out of print_gpus() and
printing it here as igt_skip_on_f() message? Do we really want that
information printed even when not skipping?
> +}
> diff --git a/lib/intel_multigpu.h b/lib/intel_multigpu.h
> new file mode 100644
> index 000000000..5acfcd5f6
> --- /dev/null
> +++ b/lib/intel_multigpu.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef __INTEL_MULTIGPU_H
> +#define __INTEL_MULTIGPU_H
> +
> +#include "drmtest.h"
> +#include "igt_core.h"
> +
> +void gem_require_multigpu(int count);
> +int gem_multigpu_count_class(int chipset);
> +
> +#define igt_foreach_gpu(fd__, chipset__) \
> + for (int igt_unique(i) = 0, fd__; \
> + (fd__ = __drm_open_driver_another(igt_unique(i)++, chipset__)) >= 0; \
> + __drm_close_driver(fd__))
> +
> +#define igt_multi_fork_foreach_gpu(__fd, __chipset) \
> + igt_multi_fork(igt_unique(__i), gem_multigpu_count_class(__chipset)) \
> + for (int __fd = drm_open_driver_another(igt_unique(__i), __chipset); \
Since igt_multi_fork() documentation says it expects no skips and
drm_open_driver_another() may skip, you must use __drm_open_driver_another(),
I believe.
Besides, that igt_multi_fork_foreach_gpu() looks to me like a special case of
multi_fork_foreach_gpu_chip() provided by your next patch, couldn't the former
be implemented as a wrapper around the latter?
> + __fd >= 0; \
> + drm_close_driver(__fd), __fd = -1) \
Please note the trailing backslash, is it intended?
Thanks,
Janusz
> +
> +#endif /* __INTEL_MULTIGPU_H */
> diff --git a/lib/meson.build b/lib/meson.build
> index 6122861d8..8251695e1 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -65,6 +65,7 @@ lib_sources = [
> 'intel_device_info.c',
> 'intel_mmio.c',
> 'intel_mocs.c',
> + 'intel_multigpu.c',
> 'intel_pat.c',
> 'ioctl_wrappers.c',
> 'media_spin.c',
>
More information about the igt-dev
mailing list