[igt-dev] [PATCH i-g-t v5 2/2] Add device selection for IGT
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Tue Sep 3 06:51:51 UTC 2019
On Fri, Aug 23, 2019 at 09:03:51AM +0200, Zbigniew Kempczyński wrote:
> New IGT command line argument --device, IGT_DEVICE enviroment
> and .igtrc Common::Device were added to allow selecting device
> using device selection API.
>
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Petri Latvala <petri.latvala at intel.com>
> ---
> docs/chamelium.txt | 3 +
> lib/drmtest.c | 188 ++++++++++++++++++++++++++++++++++++++++++++-
> lib/drmtest.h | 9 +++
> lib/igt_core.c | 34 ++++++++
> 4 files changed, 232 insertions(+), 2 deletions(-)
>
> diff --git a/docs/chamelium.txt b/docs/chamelium.txt
> index 32c296f7..d3a379d3 100644
> --- a/docs/chamelium.txt
> +++ b/docs/chamelium.txt
> @@ -84,6 +84,9 @@ example (only Chamelium.URL is mandatory):
> # The path to dump frames that fail comparison checks
> FrameDumpPath=/root/
>
> + # Device selection filter
> + Device=pci:vendor=8086,card=0;vgem:
> +
> # The following section is used for configuring the Device Under Test.
> # It is not mandatory and allows overriding default values.
> [DUT]
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index c379a7b7..c01a88a9 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -55,6 +55,7 @@
> #include "igt_gt.h"
> #include "igt_kmod.h"
> #include "igt_sysfs.h"
> +#include "igt_device_scan.h"
> #include "version.h"
> #include "config.h"
> #include "intel_reg.h"
> @@ -289,25 +290,208 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> return __search_and_open(base, offset, chipset);
> }
>
> +static int __open_driver_exact(const char *name, unsigned int chipset)
> +{
> + static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> + int fd;
> +
> + fd = open_device(name, chipset);
> + if (fd != -1)
> + return fd;
> +
> + pthread_mutex_lock(&mutex);
> + for (const struct module *m = modules; m->module; m++) {
> + if (chipset & m->bit) {
> + if (m->modprobe)
> + m->modprobe(m->module);
> + else
> + modprobe(m->module);
> + }
> + }
> + pthread_mutex_unlock(&mutex);
> +
> + return open_device(name, chipset);
> +}
> +
> +/*
> + * For compatibility mode when multiple --device argument were passed
> + * this function tries to be smart enough to handle tests which opens
> + * more than single device. It iterates over filter list and
> + * compares requested chipset to card chipset (or any).
> + *
> + * Returns:
> + * True if card according to filters added and chipset was found,
> + * false othwerwise.
> + */
> +static bool __find_card_with_chipset(int chipset, struct igt_device_card *card)
> +{
> + int i, n = igt_device_filter_count();
> + const char *filter;
> + bool match;
> +
> + for (i = 0; i < n; i++) {
> + filter = igt_device_filter_get(i);
I think that for now we should just go with the first filter and then,
when we have an API for explicit opening a second, different device
start caring about other filter. For now we can just
assert_f(igt_device_filter_count() > 1, "Only a single filter is
supported by this version of igt\n"); or something.
> + match = igt_device_card_match(filter, card);
> + if (match && (card->chipset == chipset ||
> + card->chipset == DRIVER_ANY)) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> /**
> * __drm_open_driver:
> * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> *
> - * Open the first DRM device we can find, searching up to 16 device nodes
> + * Function opens device in the following order:
> + * 1. when --device arguments are present device scanning will be executed,
> + * then filter argument is used to find matching one.
> + * 2. compatibility mode - open the first DRM device we can find,
> + * searching up to 16 device nodes.
> *
> * Returns:
> * An open DRM fd or -1 on error
> */
> int __drm_open_driver(int chipset)
> {
> + int n = igt_device_filter_count();
> +
> + if (n) {
> + bool found;
> + struct igt_device_card card;
> +
> + found = __find_card_with_chipset(chipset, &card);
> + if (!found || !strlen(card.card))
> + return -1;
> +
> + return __open_driver_exact(card.card, chipset);
> + }
> +
$ ./core_auth --r getclient-simple --device vkms
IGT-Version: 1.24-g64068440 (x86_64) (Linux: 5.2.10-250.vanilla.knurd.1.fc30.x86_64 x86_64)
Starting subtest: getclient-simple
Test requirement not met in function drm_open_driver, file ../lib/drmtest.c:569:
Test requirement: !(fd<0)
No known gpu found for chipset flags 0x4294967291 (any)
Last errno: 2, No such file or directory
Subtest getclient-simple: SKIP (0.002s)
Test requirement not met in function drm_open_driver, file ../lib/drmtest.c:569:
Test requirement: !(fd<0)
No known gpu found for chipset flags 0x4294967291 (any)
Which says nothing about filters. In such cases I think we should be
pretty verbose herbose:
1. print the filter
2. print discovered cards
3. mention that we have failed to match
Otherwise we will get people confused by shared logs, somthing they put
in .igtrc ages ago, CI, etc.
> return __open_driver("/dev/dri/card", 0, chipset);
> }
>
> -static int __drm_open_driver_render(int chipset)
> +int __drm_open_driver_render(int chipset)
> {
> + int n = igt_device_filter_count();
> +
> + if (n) {
> + bool found;
> + struct igt_device_card card;
> +
> + found = __find_card_with_chipset(chipset, &card);
> + if (!found || !strlen(card.render))
> + return -1;
> +
> + return __open_driver_exact(card.render, chipset);
> + }
> +
> return __open_driver("/dev/dri/renderD", 128, chipset);
> }
>
> +static int __drm_open_with_nth_filter(int num, int chipset, bool open_render)
> +{
> + struct igt_device_card card;
> + const char *filter, *devname;
> + bool match;
> + int n = igt_device_filter_count();
> +
> + if (!n || num < 0 || num >= n) {
> + igt_warn("No device filter num == %d passed\n", num);
> + return -1;
> + }
> +
> + filter = igt_device_filter_get(num);
> + match = igt_device_card_match(filter, &card);
> + if (!match) {
> + igt_warn("No device match filter: %s\n", filter);
> + return -1;
> + }
> +
> + if (!strlen(card.card) && !strlen(card.render))
> + return -1;
> +
> + devname = open_render ? card.render : card.card;
> + if (!strlen(devname)) {
> + igt_warn("No %s node matching filter: %s\n",
> + open_render ? "render" : "card", filter);
> + return -1;
> + }
> + return __open_driver_exact(devname, chipset);
> +}
> +
> +/**
> + * drm_open_card_with_nth_filter:
> + * @num: number of --device argument
> + * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> + *
> + * Open /dev/dri/cardX device using multi-device selection API.
> + * When test require to open more than one device selection uses filter
> + * passed as --device argument. Arguments @num indicate filter number
> + * and @chipset expected device card chipset.
> + *
> + * An open DRM fd or -1 on error
> + */
> +int drm_open_card_with_nth_filter(int num, int chipset)
> +{
> + return __drm_open_with_nth_filter(num, chipset, false);
> +}
> +
> +/**
> + * drm_open_render_with_nth_filter:
> + * @num: number of --device argument
> + * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> + *
> + * Open /dev/dri/renderDX device using multi-device selection API.
> + *
> + * When test require to open more than one device selection uses filter
> + * passed as --device argument. Arguments @num indicate filter number
> + * and @chipset expected device card chipset.
> + *
> + * An open DRM fd or -1 on error
> + */
> +int drm_open_render_with_nth_filter(int num, int chipset)
> +{
> + return __drm_open_with_nth_filter(num, chipset, true);
> +}
I am still not completely happy with those.
I can't imagine any real use-cases of the current implementation.
Instead I would probably just go with writing what I have described in
the previous iteration.
How about splitting the *_with_nth_filter() into a separate patch and
sending it on its own, as a reference? Then we can leave the design of
this bit of the interface to people that will actually consume it.
> +/**
> + * drm_open_card:
> + * @card: pointer to igt_device_card structure
> + *
> + * Open /dev/dri/cardX device using multi-device selection API.
> + *
> + * Require filled @card argument (see igt_device_card_match() function).
> + *
> + * An open DRM fd or -1 on error
> + */
> +int drm_open_card(struct igt_device_card *card)
> +{
> + if (!card || !strlen(card->card))
> + return -1;
> +
> + return __open_driver_exact(card->card, card->chipset);
> +}
> +
> +/**
> + * drm_open_render:
> + * @card: pointer to igt_device_card structure
> + *
> + * Open /dev/dri/renderDX device using multi-device selection API.
> + * Require filled @card argument (see igt_device_card_match() function).
> + *
> + * An open DRM fd or -1 on error
> + */
> +int drm_open_render(struct igt_device_card *card)
> +{
> + if (!card || !strlen(card->render))
> + return -1;
> +
> + return __open_driver_exact(card->render, card->chipset);
> +}
> +
> +
> static int at_exit_drm_fd = -1;
> static int at_exit_drm_render_fd = -1;
>
> diff --git a/lib/drmtest.h b/lib/drmtest.h
> index 614f57e6..fc2a0b57 100644
> --- a/lib/drmtest.h
> +++ b/lib/drmtest.h
> @@ -50,6 +50,7 @@
> #define DRIVER_AMDGPU (1 << 3)
> #define DRIVER_V3D (1 << 4)
> #define DRIVER_PANFROST (1 << 5)
> +
> /*
> * Exclude DRVER_VGEM from DRIVER_ANY since if you run on a system
> * with vgem as well as a supported driver, you can end up with a
> @@ -81,6 +82,14 @@ int drm_open_driver(int chipset);
> int drm_open_driver_master(int chipset);
> int drm_open_driver_render(int chipset);
> int __drm_open_driver(int chipset);
> +int __drm_open_driver_render(int chipset);
> +
> +/* Multi device API */
> +int drm_open_card_with_nth_filter(int num, int chipset);
> +int drm_open_render_with_nth_filter(int num, int chipset);
> +struct igt_device_card;
> +int drm_open_card(struct igt_device_card *card);
> +int drm_open_render(struct igt_device_card *card);
>
> void gem_quiescent_gpu(int fd);
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 1cbb09f9..a8a11b5c 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -71,6 +71,7 @@
> #include "igt_sysrq.h"
> #include "igt_rc.h"
> #include "igt_list.h"
> +#include "igt_device_scan.h"
>
> #define UNW_LOCAL_ONLY
> #include <libunwind.h>
> @@ -245,6 +246,9 @@
> * [Common]
> * FrameDumpPath=/tmp # The path to dump frames that fail comparison checks
> *
> + * # Device selection filter
> + * Device=pci:vendor=8086,card=0;vgem:
> + *
> * # The following section is used for configuring the Device Under Test.
> * # It is not mandatory and allows overriding default values.
> * [DUT]
> @@ -304,6 +308,7 @@ enum {
> OPT_DEBUG,
> OPT_INTERACTIVE_DEBUG,
> OPT_SKIP_CRC,
> + OPT_DEVICE,
> OPT_HELP = 'h'
> };
>
> @@ -320,6 +325,7 @@ static pthread_mutex_t log_buffer_mutex = PTHREAD_MUTEX_INITIALIZER;
> GKeyFile *igt_key_file;
>
> char *igt_frame_dump_path;
> +char *igt_rc_device;
>
> static bool stderr_needs_sentinel = false;
>
> @@ -624,6 +630,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
> " --skip-crc-compare\n"
> " --help-description\n"
> " --describe\n"
> + " --device filter\n"
> " --help|-h\n");
> if (help_str)
> fprintf(f, "%s\n", help_str);
> @@ -688,6 +695,16 @@ static void common_init_config(void)
> if (ret != 0)
> igt_set_autoresume_delay(ret);
>
> + /* No IGT_DEVICE and no --device passed, try .igtrc Common::Device */
> + if (!igt_rc_device)
> + igt_rc_device =
> + g_key_file_get_string(igt_key_file, "Common",
> + "Device", &error);
> + if (igt_rc_device && !igt_device_filter_count())
> + igt_device_filter_add(igt_rc_device);
> +
> + g_clear_error(&error);
> +
> out:
> if (!key_file_env && key_file_loc)
> free(key_file_loc);
> @@ -725,6 +742,11 @@ static void common_init_env(void)
> if (env) {
> __set_forced_driver(env);
> }
> +
> + env = getenv("IGT_DEVICE");
> + if (env) {
> + igt_device_filter_add(env);
> + }
> }
>
> static int common_init(int *argc, char **argv,
> @@ -743,6 +765,7 @@ static int common_init(int *argc, char **argv,
> {"debug", optional_argument, NULL, OPT_DEBUG},
> {"interactive-debug", optional_argument, NULL, OPT_INTERACTIVE_DEBUG},
> {"skip-crc-compare", no_argument, NULL, OPT_SKIP_CRC},
> + {"device", required_argument, NULL, OPT_DEVICE},
> {"help", no_argument, NULL, OPT_HELP},
> {0, 0, 0, 0}
> };
> @@ -865,6 +888,17 @@ static int common_init(int *argc, char **argv,
> case OPT_SKIP_CRC:
> igt_skip_crc_compare = true;
> goto out;
> + case OPT_DEVICE:
> + assert(optarg);
> + /* We need to free all devices passed in
> + * IGT_DEVICE environment variable.
> + */
> + if (getenv("IGT_DEVICE") && igt_device_filter_count()) {
> + unsetenv("IGT_DEVICE");
> + igt_device_filter_free_all();
> + }
I would structure that slightly differently:
char *device_filter_str = NULL;
device_filter_str = g_key_file_get_string(igt_key_file, "Common", "Device", &error);
env = getenv("IGT_DEVICE");
if (env) {
free(device_filter_str);
device_filter_str = strdup(env);
}
/* ... */
switch (c) {
case OPT_DEVICE:
assert(optarg);
free(device_filter_str);
device_filter_str = strdup(optarg);
break;
}
/* ... */
if (device_filter_str) {
igt_device_filter_add(device_filter_str);
free(device_filter_str);
}
--
Cheers,
Arek
More information about the igt-dev
mailing list