[igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Wed Aug 21 10:42:33 UTC 2019
On Wed, Aug 21, 2019 at 10:56:47AM +0300, Arkadiusz Hiler wrote:
> 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);
I would imagine this like:
static void run_test_crc(int export_chipset, int import_chipset)
{
int exporter_fd = drm_open_card_with_nth_filter(0, export_chipset);
int importer_fd = drm_open_card_with_nth_filter(1, import_chipset);
if (exporter_fd < 0 || importer_fd < 0) {
ERROR_DEVICE_NOT_FOUND();
}
/* ... */
}
Chipset ensures you're opening expected device, nothing more.
If you need defaults you should provide them in the test, like this:
if (igt_device_filter_count() == 0) {
igt_filter_device_add("vgem:");
igt_filter_device_add("vkms:");
}
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
If tests wants to open more than single device API with filter selection
should be used.
>
> 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.
I've proposed this API to enable test configure filter as it want
or use filters passed by the user. I wouln't try to be smart enough
to guess what test is doing, rather providing mechanism to get
device it needs.
>
> > +
> > +/**
> > + * 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.
Code above doesn't provide .igtrc support yet. You're right, passing
--device should override IGT_DEVICE enviroment. I'm going to fix that.
Zbigniew
More information about the igt-dev
mailing list