Re: [igt-dev] [PATCH i-g-t v5 2/2] Add device selection for IGT

Kempczynski, Zbigniew
2019-09-03T12:25:16Z

On Tue, 2019-09-03 at 09:51 +0300, Arkadiusz Hiler wrote:

<cut>

> > +/*
> > + * 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;
> > +}
> > +

My intention was to run kms_prime tests without changing test code,
allowing user to select vgem and other device he wants. Without
reiterating the filters and matching chipset it is not possible.
Above function has a bug, I've fixed it.

So:
# kms_prime --device vgem: --device pci:
or
# kms_prime --device pci: --device vgem:

will work properly when I'll push the new patchset.

I would like to rewrite tests adopting new API partially, not making
revolution within tests now.

<cut>

> $ ./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.

You're right. Currently device selection code is not verbose and
in case of problems you're not informed what's wrong.
I will add debug / error messages to the code.

<cut>
> +int drm_open_card_with_nth_filter(int num, int chipset)
<cut>
> +int drm_open_render_with_nth_filter(int num, int chipset)

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

Ok, I will add this as separate patch when device selection core
will be reviewed and merged.
We can take then few tests and propose new API in drmtest.[ch].

<cut>
>
> > 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
> > *
> > + * &num; Device selection filter
> > + * Device=pci:vendor=8086,card=0;vgem:
> > + *
> > * &num; The following section is used for configuring the Device Under
> > Test.
> > * &num; 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);
> }

I would also do that but current IGT code do the following:

common_init()
-> common_init_env() - getenv(IGT_DEVICE)
...
-> getopt() loop - (--device) passing
...
-> common_init_config() - parse .igtrc

So reading .igtrc is done at the end, so I have do some tricks
to keep device selection order we set in previous email.

Zbigniew