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

["Kempczynski, Zbigniew" <zbigniew.kempczynski@intel.com>]

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


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev