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

Arkadiusz Hiler
2019-09-10T12:18:14Z

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 @@
>
> </varlistentry>
>
> + <varlistentry>
> + <term> --device filter </term>
> +
> + select device using filter (see "Device selection" for details)
> +
> + </varlistentry>
> +

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

> <varlistentry>
> <term> --help-description </term>
>
> 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>