[igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Wed Aug 21 07:56:47 UTC 2019
On Mon, Aug 12, 2019 at 10:47:18AM +0200, Zbigniew Kempczyński wrote:
> New IGT command line argument --device as well as IGT_DEVICE enviroment
> were added to allow selecting device using multi-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>
> ---
> lib/drmtest.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++++-
> lib/drmtest.h | 9 +++
> lib/igt_core.c | 13 ++++
> 3 files changed, 208 insertions(+), 2 deletions(-)
>
> 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).
How do you imagine the new API working with multi-device tests that open
more than one node?
Currently some of our tests are depending on haxes to excercise that.
Let's take kms_prime:
static void run_test_crc(int export_chipset, int import_chipset)
{
int exporter_fd = drm_open_driver(export_chipset);
int importer_fd = drm_open_driver_master(import_chipset);
/* ... */
}
igt_subtest("basic-crc")
run_test_crc(DRIVER_VGEM, DRIVER_ANY);
This works only because:
/*
* 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
* near-100% skip rate if you don't explicitly specify the device,
* depending on device-load ordering.
*/
#define DRIVER_ANY ~(DRIVER_VGEM)
So I am expecting similar tests in the future that are touching real
devices. With the current device handling this would not work as
specifying double DRIVER_ANY would result in the same device being
opened twice.
I was initially thinking about something like:
int drm_open_other_device(int chipset, int other_than_fd);
int exporter_fd = drm_open_driver(DRIVER_ANY);
int importer_fd = drm_open_other_device(DRIVER_ANY, exporter_fd);
Now, with the new API I would like to be able to:
1. open two different devices, even without using the filters at all
2. select which device is exporter and importer via --device filters
I see that we have drm_open_card_with_nth_filter, but this requires a
filter, so using it in the tests would require --device, so point 1 is
not addressed.
Do you have any thoughts on that?
To be clear - I am not requesting anything to be implemented here. It's
better to leave that to people actually interested in similar tests. I
just want to make sure the current implementation is flexible enough to
accomodate for that and it won't result in a complete re-write of the
device selection logic :-)
> + * 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);
> + 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) {
if (igt_device_filter_count() > 0)
should be nicer to read, there are few more instances
> + bool found;
> + struct igt_device_card card;
> +
> + found = __find_card_with_chipset(chipset, &card);
> + if (!found || !strlen(card.card))
It would be nice to log (I suggest WARN level) that we do have filters
but failed to match any to provided 'int chipset'.
> + return -1;
> +
Here we can mention that we succeded (INFO) + some context (which
filter, device, etc).
I think we should be as explicit on which node/card was used when
possible. Less surprises and debugging for users who don't know why
something skips or which device was opened exactely.
> + return __open_driver_exact(card.card, chipset);
> + }
> +
> 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);
> +}
Related to the comments above - I would be surprised if any of the test
would be using that directly, as it forces using filters.
> +
> +/**
> + * 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);
> +}
> +
> +/**
> + * 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..9b851175 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>
> @@ -304,6 +305,7 @@ enum {
> OPT_DEBUG,
> OPT_INTERACTIVE_DEBUG,
> OPT_SKIP_CRC,
> + OPT_DEVICE,
> OPT_HELP = 'h'
> };
>
> @@ -624,6 +626,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);
> @@ -725,6 +728,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 +751,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 +874,10 @@ static int common_init(int *argc, char **argv,
> case OPT_SKIP_CRC:
> igt_skip_crc_compare = true;
> goto out;
> + case OPT_DEVICE:
> + assert(optarg);
> + igt_device_filter_add(optarg);
> + break;
> case OPT_HELP:
> print_usage(help_str, false);
> ret = -1;
So first we add filters via IGT_DEVICE and then we *append* filters
passed via --device.
So `export IGT_DEVICE=yyy; ./test --device zzz` is equivalent to '--device yyy;zzz'.
This is surprising to me - I would expect only 'zzz' to take effect.
There were some other people asking how options, which you can configure
in a multiple ways, should be handled. I think that the consensus was
that you overwrite them completely in the following order.
* .igtrc has the lowest importance
* ENV variable overwrites whatever is specified in .igtrc
* --switch on a test overwrites everything else
I guess we should put that in the docs.
--
Cheers,
Arek
More information about the igt-dev
mailing list