Re: [igt-dev] [PATCH i-g-t v6 2/3] Add device selection in IGT

[Arkadiusz Hiler <arkadiusz.hiler@intel.com>]

On Wed, Sep 04, 2019 at 12:37:00PM +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@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
>  docs/chamelium.txt                            |   3 +
>  .../igt-gpu-tools/igt_test_programs.xml       |   7 +
>  lib/drmtest.c                                 | 137 +++++++++++++++++-
>  lib/drmtest.h                                 |   7 +
>  lib/igt_core.c                                |  49 +++++++
>  5 files changed, 201 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:
> +


This is unnecessary here - it's  chamelium specific .txt that
should be integrated into our gtk-doc-generated documentation anyway.

We should also add a section dedicated to .igtrc there, but that's a
separate effort.

>      # The following section is used for configuring the Device Under Test.
>      # It is not mandatory and allows overriding default values.
>      [DUT]
> diff --git a/docs/reference/igt-gpu-tools/igt_test_programs.xml b/docs/reference/igt-gpu-tools/igt_test_programs.xml
> index b64ba474..fcce1458 100644
> --- a/docs/reference/igt-gpu-tools/igt_test_programs.xml
> +++ b/docs/reference/igt-gpu-tools/igt_test_programs.xml
> @@ -43,6 +43,13 @@
>              </para></listitem>
>            </varlistentry>
>  
> +          <varlistentry>
> +            <term><option>--device filter</option></term>
> +            <listitem><para>
> +                select device using filter (see "Device selection" for details)
> +            </para></listitem>
> +          </varlistentry>
> +

oo, I've forgot about this, I have to add --describe to this list, thanks :-P

>            <varlistentry>
>              <term><option>--help-description</option></term>
>              <listitem><para>
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index c379a7b7..52199821 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,157 @@ 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);

This is the same part as in __open_driver(), it should be extraced into
a helper, e.g.:

static void __try_modprobe(unsigned int chipset)
{
	static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

	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);
}

This will avoid duplication and would keep the intended concurency
constrains on modules loading even when someone messes up with mixed
*_open_* function calls.

> +
> +	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
> + * check does requested chipset has common bits with card->chipset.
> + *
> + * @chipset: requested chipset
> + * @card: pointer to the igt_device_card structure to be filled
> + * when a card is found.
> + *
> + * 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)) {
> +			igt_debug("Filter [%s] matched\n", filter);
> +			return true;
> +		}
> +	}
> +
> +	igt_warn("Can't find card with chipset (mask): 0x%x\n", chipset);
> +	igt_warn("Filters tried:\n");
> +	for (i = 0; i < n; i++) {
> +		filter = igt_device_filter_get(i);
> +		igt_warn("%2d: %s\n", i, filter);
> +	}
> +
> +	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) {

if we use it in only one place

for places where we use it multiple times please name the variable more
descriptively, e.g. filter_count.

> +		bool found;
> +		struct igt_device_card card;
> +
> +		found = __find_card_with_chipset(chipset, &card);
> +		if (!found || !strlen(card.card))
> +			return -1;
> +		igt_debug("Trying to open: %s (chipset mask/card: %x/%x)\n",
> +			  card.card, chipset, card.chipset);
> +
> +		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;
> +
> +		igt_debug("Trying to open: %s (chipset mask/card: %x/%x)\n",
> +			  card.render, chipset, card.chipset);
> +
> +		return __open_driver_exact(card.render, chipset);
> +	}
> +
>  	return __open_driver("/dev/dri/renderD", 128, chipset);
>  }
>  
> +/**
> + * 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..31947f43 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,12 @@ 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 */
> +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..b45a1be7 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,33 @@ static void common_init_config(void)
>  	if (ret != 0)
>  		igt_set_autoresume_delay(ret);
>  
> +	/* Adding filters, order .igtrc, IGT_DEVICE, --device filter */
> +	if (igt_device_filter_count())
> +		igt_debug("Notice: using --device filters:\n");
> +	else {
> +		if (igt_rc_device) {
> +			igt_debug("Notice: using IGT_DEVICE env:\n");
> +		} else {
> +			igt_rc_device =	g_key_file_get_string(igt_key_file,
> +							      "Common",
> +							      "Device", &error);
> +			g_clear_error(&error);
> +			if (igt_rc_device)
> +				igt_debug("Notice: using .igtrc "
> +					  "Common::Device:\n");
> +		}
> +		if (igt_rc_device) {
> +			igt_device_filter_add(igt_rc_device);
> +			free(igt_rc_device);
> +			igt_rc_device = NULL;
> +		}
> +	}
> +
> +	if (igt_device_filter_count()) {
> +		for (int i = 0; i < igt_device_filter_count(); i++)
> +			igt_debug("%2d: [%s]\n", i, igt_device_filter_get(i));
> +	}
> +
>  out:
>  	if (!key_file_env && key_file_loc)
>  		free(key_file_loc);
> @@ -725,6 +759,11 @@ static void common_init_env(void)
>  	if (env) {
>  		__set_forced_driver(env);
>  	}
> +
> +	env = getenv("IGT_DEVICE");
> +	if (env) {
> +		igt_rc_device = strdup(env);
> +	}
>  }
>  
>  static int common_init(int *argc, char **argv,
> @@ -743,6 +782,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 +905,15 @@ static int common_init(int *argc, char **argv,
>  		case OPT_SKIP_CRC:
>  			igt_skip_crc_compare = true;
>  			goto out;
> +		case OPT_DEVICE:
> +			assert(optarg);
> +			/* if set by env IGT_DEVICE we need to free it */
> +			if (igt_rc_device) {
> +				free(igt_rc_device);
> +				igt_rc_device = NULL;
> +			}
> +			igt_device_filter_add(optarg);
> +			break;

It's not exactly linear to follow and should be reworked + we should
document how the option shadowing works, but that's on me.

So with the chamelium.txt change removal, modprobe stuff extracted out
and few cosmetic changes this patch is:

Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev