* [igt-dev] [PATCH i-g-t v4 0/2] Add device selection methods
@ 2019-08-12 8:47 Zbigniew Kempczyński
2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API Zbigniew Kempczyński
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Zbigniew Kempczyński @ 2019-08-12 8:47 UTC (permalink / raw)
To: igt-dev
v2: In this series I've removed module management and hand device classes
filtering (udev filters are now used in this case). Lsgpu was
simplified (some options were unreadable and they were removed).
Still migration to gtkdoc is necessary which will be made in v3.
v3: Change device scanning to drm subsystem with getting parent
according to Daniel Vetter suggestion. Splitting to two patches
and adding gtk-doc documentation.
v4: Extending and fixing documentation. Adding reviewers forgotten
in v3.
Zbigniew Kempczyński (2):
Introduce multi-device selection API
Add multi-device selection for IGT
.../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 +
lib/Makefile.sources | 2 +
lib/drmtest.c | 188 ++-
lib/drmtest.h | 9 +
lib/igt_core.c | 13 +
lib/igt_device_scan.c | 1318 +++++++++++++++++
lib/igt_device_scan.h | 69 +
lib/meson.build | 1 +
tools/Makefile.sources | 1 +
tools/lsgpu.c | 183 +++
tools/meson.build | 1 +
11 files changed, 1784 insertions(+), 2 deletions(-)
create mode 100644 lib/igt_device_scan.c
create mode 100644 lib/igt_device_scan.h
create mode 100644 tools/lsgpu.c
--
2.21.0
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 13+ messages in thread* [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API 2019-08-12 8:47 [igt-dev] [PATCH i-g-t v4 0/2] Add device selection methods Zbigniew Kempczyński @ 2019-08-12 8:47 ` Zbigniew Kempczyński 2019-08-21 8:09 ` Arkadiusz Hiler 2019-08-21 11:04 ` Katarzyna Dec 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT Zbigniew Kempczyński ` (2 subsequent siblings) 3 siblings, 2 replies; 13+ messages in thread From: Zbigniew Kempczyński @ 2019-08-12 8:47 UTC (permalink / raw) To: igt-dev; +Cc: Daniel Vetter, Petri Latvala Change adds device selection based on scanning drm subsystem using udev library. Tool 'lsgpu' which uses device scanning and selection feature was added. 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> --- .../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 + lib/Makefile.sources | 2 + lib/igt_device_scan.c | 1318 +++++++++++++++++ lib/igt_device_scan.h | 69 + lib/meson.build | 1 + tools/Makefile.sources | 1 + tools/lsgpu.c | 183 +++ tools/meson.build | 1 + 8 files changed, 1576 insertions(+) create mode 100644 lib/igt_device_scan.c create mode 100644 lib/igt_device_scan.h create mode 100644 tools/lsgpu.c diff --git a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml index ac83272f..4b3c38af 100644 --- a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml +++ b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml @@ -23,6 +23,7 @@ <xi:include href="xml/igt_core.xml"/> <xi:include href="xml/igt_debugfs.xml"/> <xi:include href="xml/igt_device.xml"/> + <xi:include href="xml/igt_device_scan.xml"/> <xi:include href="xml/igt_draw.xml"/> <xi:include href="xml/igt_dummyload.xml"/> <xi:include href="xml/igt_fb.xml"/> diff --git a/lib/Makefile.sources b/lib/Makefile.sources index e16de86e..c383a817 100644 --- a/lib/Makefile.sources +++ b/lib/Makefile.sources @@ -25,6 +25,8 @@ lib_source_list = \ igt_debugfs.h \ igt_device.c \ igt_device.h \ + igt_device_scan.c \ + igt_device_scan.h \ igt_aux.c \ igt_aux.h \ igt_color_encoding.c \ diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c new file mode 100644 index 00000000..4fc165b2 --- /dev/null +++ b/lib/igt_device_scan.c @@ -0,0 +1,1318 @@ +/* + * Copyright © 2019 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +#include "igt.h" +#include "igt_sysfs.h" +#include "igt_device.h" +#include "igt_device_scan.h" +#include <glib.h> +#include <libudev.h> +#include <linux/limits.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <dirent.h> +#include <fcntl.h> + +/** + * SECTION:igt_device_scan + * @short_description: Multi-device scanning and selection + * @title: Multi-device selection + * @include: igt.h + * + * # Device scanning + * + * Device scanning iterates over drm subsystem using udev library + * to acquire drm devices. + * For each drm device we also get and store its parent in a device array + * to allow device selection in more contextual way. + * + * Parent devices are bus devices (like pci, platform, etc.) and contain a lot + * of usable information comparing to drm device. + * + * Udev keeps properties and sysattrs for the device as a list what is not + * convinient for key/value searching. So udev device properties and sysattrs + * are rewritten to internal hash tables in igt_device structure. + * + * # Filters + * + * Device selection can be done using filters. Filters allows sophisticated + * device matching and selection. Previously mentioned properties and sysattrs + * collected in igt_device hash tables simplify writing filter implementation. + * + * Direct device selection filter + * is special filter and it is checked first. Then contextual filter is chosen + * depending on filter name. + * + * Direct device selection filter must be: + * + * |[<!-- language="plain" --> + * subsystem:/sys + * ]| + * + * So, when user passes filter which looks like follows: + * |[<!-- language="plain" --> + * - drm:/sys/devices/pci0000:00/0000:00:02.0/drm/card0 + * - pci:/sys/devices/pci0000:00/0000:00:02.0 + * - platform:/sys/devices/platform/vgem + * ]| + * + * device from array which has subsystem and sys path will be returned. + * + * When /sys is not specified after colon contextual filters are taken + * into account. Drm device occurs in the system when appropriate module + * is loaded and device is detected (or exposed for platform devices). Loading + * drivers in different order can be problematic from CI point of view, where + * expectation is to get same device especially when multiple gpu devices + * exists in the system. For such devices its parent location on pci bus + * is constant and allows appropriate device selection using for example + * vendor / device ids. + * + * For tests which opens more than one device device filter collection API + * can be used. You can add filter to the array using igt_device_filter_add(), + * get nth filter stored using igt_device_filter_get() and return + * #igt_device_card using filter in igt_device_card_match(). + * + * Implemented filters: + * + * - drm: get drm /dev/dri/... device directly + * + * |[<!-- language="plain" --> + * drm:/dev/dri/... + * ]| + * + * - pci: select device using pci properties + * |[<!-- language="plain" --> + * pci:[vendor=%04x/name][,device=%04x][,card=%d] + * ]| + * + * Filter allows device selection using vendor (hex or name), device id + * (hex) and nth-card from all matches. For example if there're 4 pci + * cards installed (let two cards have 1234 and other two 1235 device id, + * all of them of vendor Intel) you can select one using: + * + * |[<!-- language="plain" --> + * pci:vendor=Intel,device=1234,card=0 + * ]| + * + * or + * + * |[<!-- language="plain" --> + * pci:vendor=8086,device=1234,card=0 + * ]| + * + * This takes first device with 1234 id for Intel vendor (8086). + * + * |[<!-- language="plain" --> + * pci:vendor=Intel,device=1234,card=1 + * ]| + * + * or + * + * |[<!-- language="plain" --> + * pci:vendor=8086,device=1234,card=1 + * ]| + * + * It selects second one. + * + * As order on pci bus doesn't change (unless you'll add new device or + * reorder existing one) device selection using this filter will always + * return you same device regardless module loading time. + * + * - vgem: select virtual gem device + * + * |[<!-- language="plain" --> + * vgem:[card=%d] + * ]| + * + * For example: + * + * |[<!-- language="plain" --> + * vgem: + * vgem:card=0 + * ]| + * + * Both selects first vgem device (no idea if more than one device will + * be visible, if yes use card= argument to point right one). + * + * - vkms: select virtual kms device + * + * |[<!-- language="plain" --> + * vkms:[card=%d] + * ]| + * + * For example: + * + * |[<!-- language="plain" --> + * vkms: + * vkms:card=0 + * ]| + * + * Both selects first vkms device. Same as for vgem I assume more than + * one device can appear in the future. + * + * - vc4: select VC4 device + * + * |[<!-- language="plain" --> + * vc4:[card=%d] + * ]| + * + * For example: + * + * |[<!-- language="plain" --> + * vc4: + * vc4:card=0 + * ]| + * + * Both selects first VC4 device. At the moment Rasperry PI has only + * one gpu, but filter is ready if there will be more. + * + * # lsgpu + * + * Scanned devices can be displayed using 'lsgpu' tool. Tool also displays + * properties and sysattrs (-D switch, means detail) which can be used during + * filter implementation. + * + * Tool can also be used to check does the implemented filter works as expected. + * To select device using filter use '-d' or '--device' argument like: + * + * |[<!-- language="plain" --> + * ./lsgpu -d 'pci:vendor=Intel' + * === Device filter list === + * [ 0]: pci:vendor=Intel + + * === Testing device open === + * subsystem : pci + * chipset : 1 + * drm card : /dev/dri/card0 + * drm render : /dev/dri/renderD128 + * Device /dev/dri/card0 successfully opened + * Device /dev/dri/renderD128 successfully opened + * ]| + * + * Apart of selecting device lsgpu try to open card and render nodes. +*/ + +//#define DEBUG_DEVICE_SCAN +#ifdef DEBUG_DEVICE_SCAN +#define DBG(...) \ +{ \ + struct timeval tm; \ + gettimeofday(&tm, NULL); \ + printf("%10ld.%03ld: ", tm.tv_sec, tm.tv_usec); \ + printf(__VA_ARGS__); \ + } + +#else +#define DBG(...) {} +#endif + +#define strequal(x, y) ((x) && (y) && !strcmp((x), (y))) +#define IGT_DRM_PATH "/dev/dri" + +static GHashTable *sysattrs_blacklist_ht; //sysattrs we don't want to read +static GHashTable *gpu_vendor_ht; //search id -> vendor_spec mapping +static GHashTable *filter_definition_ht; //supported filters (pci=..., etc.) + +/* Generic name->value struct */ +struct name_value { + const char *name; + gpointer *value; +}; + +/* Vendor specific data */ +struct vendor_spec { + const char *vendor; + const char *match; + int chipset; +}; + +struct igt_device { + /* Filled for drm devices */ + struct igt_device *parent; + + /* Point to vendor spec if can be found */ + struct vendor_spec *vs; + + /* Properties / sysattrs rewriten from udev lists */ + GHashTable *props_ht; + GHashTable *attrs_ht; + + /* Most usable variables from udev device */ + char *subsystem; + char *syspath; + char *devnode; + + /* /dev/dri/... paths */ + char *drm_card; + char *drm_render; + + /* For pci subsystem */ + char *vendor; + char *device; +}; + +/* Scanned devices */ +struct igt_devices { + GPtrArray *devs; //all devices + GPtrArray *view; //filtered view + bool devs_scanned; +}; + +/* Scanned devices holder */ +static struct igt_devices igt_devs; + +static struct vendor_spec v_intel = { .vendor = "Intel", + .chipset = DRIVER_INTEL + }; +static struct vendor_spec v_amd = { .vendor = "AMD", + .chipset = DRIVER_AMDGPU + }; +static struct vendor_spec v_vgem = { .vendor = "Virtual-GEM", + .chipset = DRIVER_VGEM + }; +static struct vendor_spec v_vc4 = { .vendor = "Broadcom", + .chipset = DRIVER_VC4 + }; + +/* Mapping vendor id => vendor_spec should be unique (vendor string matching + * is written around this). + * + * Keys must be defined as follows: + * PCI devices: PCI_SLOT_ID -> vendor_spec. +*/ +struct name_value gpu_vendor_list[] = { + { "8086", (gpointer) &v_intel }, + { "1002", (gpointer) &v_amd }, + { NULL, }, +}; + +/* Generic hash table fill function, requires name / value ptrs array */ +static void fill_ht(GHashTable **ht, struct name_value *data) +{ + if (*ht) + return; + + *ht = g_hash_table_new(g_str_hash, g_str_equal); + igt_assert(*ht); + + while (data->name) { + g_hash_table_insert(*ht, + (gpointer) data->name, + data->value); + data++; + } +} + +#define get_vendor_spec(prop) \ + g_hash_table_lookup(gpu_vendor_ht, prop) + +/* Go through whole vendor list and compare against vendor field. + * Used mostly with vendor=... filter parameter when PCI id is not matched. +*/ +static const char *get_pci_vendor_id_by_name(const char *name) +{ + struct name_value *data = &gpu_vendor_list[0]; + + while (data->name) { + struct vendor_spec *vs = (struct vendor_spec *) data->value; + if (!strcasecmp(name, vs->vendor)) + return data->name; + data++; + } + + return NULL; +} + +/* Reading sysattr values can take time (even seconds), + * we want to avoid reading such keys. +*/ +static void populate_blacklist_keys(void) +{ + const char *keys[] = { "config", "modalias", "modes", + "resource", + "resource0", "resource1", "resource2", + "resource3", "resource4", "resource5", + "resource0_wc", "resource1_wc", "resource2_wc", + "resource3_wc", "resource4_wc", "resource5_wc", + "driver", + "uevent", NULL}; + const char *key; + int i = 0; + + if (sysattrs_blacklist_ht) + return; + + sysattrs_blacklist_ht = g_hash_table_new(g_str_hash, g_str_equal); + igt_assert(sysattrs_blacklist_ht); + + while ((key = keys[i++])) + g_hash_table_add(sysattrs_blacklist_ht, (gpointer) key); +} + +#define is_on_blacklist(key) \ + g_hash_table_contains(sysattrs_blacklist_ht, key) + +static struct igt_device *igt_device_new(void) +{ + struct igt_device *dev; + dev = calloc(1, sizeof(struct igt_device)); + if (!dev) + return NULL; + + dev->attrs_ht = g_hash_table_new_full(g_str_hash, g_str_equal, + free, free); + dev->props_ht = g_hash_table_new_full(g_str_hash, g_str_equal, + free, free); + + if (dev->attrs_ht && dev->props_ht) + return dev; + + return NULL; +} + +static void igt_device_add_prop(struct igt_device *dev, + const char *key, const char *value) +{ + if (!key || !value) + return; + + g_hash_table_insert(dev->props_ht, strdup(key), strdup(value)); +} + +static void igt_device_add_attr(struct igt_device *dev, + const char *key, const char *value) +{ + const char *v = value; + + if (!key) + return; + + /* It's possible we have symlink at key filename, but udev + * library resolves only few of them */ + if (!v) { + struct stat st; + char path[PATH_MAX]; + char linkto[PATH_MAX]; + int len; + + snprintf(path, sizeof(path), "%s/%s", dev->syspath, key); + if (lstat(path, &st) != 0) + return; + + len = readlink(path, linkto, sizeof(linkto)); + if (len <= 0 || len == (ssize_t) sizeof(linkto)) + return; + linkto[len] = '\0'; + v = strrchr(linkto, '/'); + if (v == NULL) + return; + v++; + } + + g_hash_table_insert(dev->attrs_ht, strdup(key), strdup(v)); +} + +/* Iterate over udev properties list and rewrite it to igt_device properties + * hash table for instant access. + */ +static void get_props(struct udev_device *dev, struct igt_device *idev) +{ + struct udev_list_entry *entry; + + entry = udev_device_get_properties_list_entry(dev); + while (entry) { + const char *name = udev_list_entry_get_name(entry); + const char *value = udev_list_entry_get_value(entry); + igt_device_add_prop(idev, name, value); + entry = udev_list_entry_get_next(entry); + DBG("prop: %s, val: %s\n", name, value); + } +} + +/* Same as get_props(), but rewrites sysattrs. Resolves symbolic links + * not handled by udev get_sysattr_value(). + * Function skips sysattrs from blacklist ht (acquiring some values can take + * seconds). + */ +static void get_attrs(struct udev_device *dev, struct igt_device *idev) +{ + struct udev_list_entry *entry; + + entry = udev_device_get_sysattr_list_entry(dev); + while (entry) { + const char *key = udev_list_entry_get_name(entry); + const char *value; + + if (is_on_blacklist(key)) { + entry = udev_list_entry_get_next(entry); + continue; + } + + value = udev_device_get_sysattr_value(dev, key); + igt_device_add_attr(idev, key, value); + entry = udev_list_entry_get_next(entry); + DBG("attr: %s, val: %s\n", key, value); + } +} + +#define get_prop(dev, prop) (char *) g_hash_table_lookup(dev->props_ht, prop) +#define get_attr(dev, attr) (char *) g_hash_table_lookup(dev->attrs_ht, attr) +#define get_prop_subsystem(dev) get_prop(dev, "SUBSYSTEM") +#define is_drm_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "drm")) +#define is_pci_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "pci")) +#define is_platform_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "platform")) + +/* Gets PCI_ID property, splits to xxxx:yyyy and stores + * xxxx to dev->vendor and yyyy to dev->device for + * faster access. + */ +static void set_vendor_device(struct igt_device *dev) +{ + const char *pci_id = get_prop(dev, "PCI_ID"); + if (!pci_id || strlen(pci_id) != 9) + return; + dev->vendor = strndup(pci_id, 4); + dev->device = strndup(pci_id + 5, 4); +} + +/* Allocate arrays for keeping scanned devices */ +static bool prepare_scan(void) +{ + if (!igt_devs.devs) + igt_devs.devs = g_ptr_array_sized_new(4); + if (!igt_devs.view) + igt_devs.view = g_ptr_array_sized_new(4); + + if (!igt_devs.devs || !igt_devs.view) + return false; + + return true; +} + +/* Create new igt_device from udev device. + Fills structure with most usable udev device variables, properties + and sysattrs. +*/ +static struct igt_device *igt_device_new_from_udev(struct udev_device *dev) +{ + struct igt_device *idev = igt_device_new(); + igt_assert(idev); + + idev->syspath = g_strdup(udev_device_get_syspath(dev)); + idev->subsystem = g_strdup(udev_device_get_subsystem(dev)); + idev->devnode = g_strdup(udev_device_get_devnode(dev)); + + if (idev->devnode && strstr(idev->devnode, "/dev/dri/card")) + idev->drm_card = g_strdup(idev->devnode); + else if (idev->devnode && strstr(idev->devnode, "/dev/dri/render")) + idev->drm_render = g_strdup(idev->devnode); + + get_props(dev, idev); + get_attrs(dev, idev); + + return idev; +} + +/* Iterate over all igt_devices array and find one matched to + * subsystem and syspath. +*/ +static struct igt_device *igt_device_find(const char *subsystem, + const char *syspath) +{ + struct igt_device *dev; + + for (int i = 0; i < igt_devs.devs->len; i++) { + dev = g_ptr_array_index(igt_devs.devs, i); + if (!strcmp(dev->subsystem, subsystem) && + !strcmp(dev->syspath, syspath)) + return dev; + } + return NULL; +} + +/* For each drm igt_device add or update its parent igt_device to the array. + As card/render drm devices mostly have same parent (vkms is an exception) + link to it and update corresponding drm_card / drm_render fields. +*/ +static void update_or_add_parent(struct udev_device *dev, + struct igt_device *idev) +{ + struct udev_device *parent_dev; + struct igt_device *parent_idev; + const char *subsystem, *syspath, *devname; + + parent_dev = udev_device_get_parent(dev); + igt_assert(parent_dev); + + subsystem = udev_device_get_subsystem(parent_dev); + syspath = udev_device_get_syspath(parent_dev); + + parent_idev = igt_device_find(subsystem, syspath); + if (!parent_idev) { + parent_idev = igt_device_new_from_udev(parent_dev); + if (is_pci_subsystem(parent_idev)) { + set_vendor_device(parent_idev); + parent_idev->vs = get_vendor_spec(parent_idev->vendor); + } + g_ptr_array_add(igt_devs.devs, parent_idev); + } + devname = udev_device_get_devnode(dev); + if (strstr(devname, "/dev/dri/card")) + parent_idev->drm_card = g_strdup(devname); + else if (strstr(devname, "/dev/dri/render")) + parent_idev->drm_render= g_strdup(devname); + + idev->parent = parent_idev; +} + +static gint devs_compare(gconstpointer a, gconstpointer b) +{ + struct igt_device *dev1, *dev2; + int ret; + + dev1 = *(struct igt_device **) a; + dev2 = *(struct igt_device **) b; + ret = strcmp(dev1->subsystem, dev2->subsystem); + if (ret) + return ret; + + return strcmp(dev1->syspath, dev2->syspath); +} + +/* Core scanning function. + * + * All scanned devices are kept inside igt_devs.devs pointer array. + * Each added device is igt_device structure, which contrary to udev device + * has properties / sysattrs stored inside hash table instead of list. + * + * Function iterates over devices on 'drm' subsystem. For each drm device + * its parent is taken (bus device) and stored inside same array. + * Function sorts all found devices to keep same order of bus devices + * for providing predictable search. + */ +static void scan_drm_devices(void) +{ + struct udev *udev; + struct udev_enumerate *enumerate; + struct udev_list_entry *devices, *dev_list_entry; + int ret; + + udev = udev_new(); + igt_assert(udev); + + enumerate = udev_enumerate_new(udev); + igt_assert(enumerate); + + DBG("Scanning drm subsystem\n"); + ret = udev_enumerate_add_match_subsystem(enumerate, "drm"); + igt_assert(!ret); + + udev_enumerate_add_match_property(enumerate, "DEVNAME", "/dev/dri/*"); + igt_assert(!ret); + + ret = udev_enumerate_scan_devices(enumerate); + igt_assert(!ret); + + devices = udev_enumerate_get_list_entry(enumerate); + if (!devices) + return; + + udev_list_entry_foreach(dev_list_entry, devices) { + const char *path; + struct udev_device *dev; + struct igt_device *idev; + + path = udev_list_entry_get_name(dev_list_entry); + dev = udev_device_new_from_syspath(udev, path); + idev = igt_device_new_from_udev(dev); + update_or_add_parent(dev, idev); + g_ptr_array_add(igt_devs.devs, idev); + + udev_device_unref(dev); + } + udev_enumerate_unref(enumerate); + udev_unref(udev); + + g_ptr_array_sort(igt_devs.devs, devs_compare); + for (int i = 0; i < igt_devs.devs->len; i++) { + g_ptr_array_add(igt_devs.view, + g_ptr_array_index(igt_devs.devs, i)); + } +} + +struct name_value filter_definition_list[]; +static void populate_gpu_data(void) +{ + fill_ht(&gpu_vendor_ht, &gpu_vendor_list[0]); + fill_ht(&filter_definition_ht, &filter_definition_list[0]); +} + +static void igt_device_free(struct igt_device *dev) +{ + free(dev->devnode); + free(dev->subsystem); + free(dev->syspath); + free(dev->drm_card); + free(dev->drm_render); + free(dev->vendor); + free(dev->device); + g_hash_table_destroy(dev->attrs_ht); + g_hash_table_destroy(dev->props_ht); +} + +/** + * igt_devices_scan + * @force: enforce scanning devices + * + * Function scans udev in search of gpu devices. For first run it can be + * called with @force = false. If something changes during the the test + * or test does some module loading (new drm devices occurs during execution) + * function must be called again with @force = true to refresh device array. + */ +void igt_devices_scan(bool force) +{ + if (force && igt_devs.devs_scanned) { + for (int i = 0; i < igt_devs.devs->len; i++) { + struct igt_device *dev = + g_ptr_array_index(igt_devs.devs, i); + igt_device_free(dev); + free(dev); + } + igt_devs.devs_scanned = false; + g_ptr_array_free(igt_devs.view, true); + g_ptr_array_free(igt_devs.devs, true); + igt_devs.view = NULL; + igt_devs.devs = NULL; + } + + if (igt_devs.devs_scanned) + return; + + populate_blacklist_keys(); //keys from sysattr we skip + populate_gpu_data(); + + prepare_scan(); + scan_drm_devices(); + + igt_devs.devs_scanned = true; +} + +#define pr_simple(k, v) printf(" %-16s: %s\n", k, v) +#define pr_simple2(k, v1, v2) printf(" %-16s: %s:%s\n", k, v1, v2) +#define pr_simple_prop(dev, key) pr_simple(key, get_prop(dev, key)) +#define pr_simple_attr(dev, key) pr_simple(key, get_attr(dev, key)) + +static void igt_devs_print_simple(GPtrArray *view) +{ + struct igt_device *dev; + int i; + + if (!view) + return; + + for (i = 0; i < view->len; i++) { + dev = g_ptr_array_index(view, i); + printf("%s:%s\n", dev->subsystem, dev->syspath); + if (dev->drm_card) + pr_simple("drm card", dev->drm_card); + if (dev->drm_render) + pr_simple("drm render", dev->drm_render); + if (is_drm_subsystem(dev)) { + pr_simple2("parent", dev->parent->subsystem, + dev->parent->syspath); + } else { + if (is_pci_subsystem(dev)) { + pr_simple("vendor", dev->vendor); + pr_simple("device", dev->device); + } + } + printf("\n"); + } +} + +#define pr_detail(k, v) printf("%-32s: %s\n", k, v) + +static void print_ht(GHashTable *ht) +{ + GList *keys = g_hash_table_get_keys(ht); + keys = g_list_sort(keys, (GCompareFunc) strcmp); + while (keys) { + char *k = (char *) keys->data; + char *v = g_hash_table_lookup(ht, k); + pr_detail(k, v); + keys = g_list_next(keys); + } + g_list_free(keys); +} + +static void igt_devs_print_detail(GPtrArray *view) +{ + struct igt_device *dev; + int i; + + if (!view) + return; + + for (i = 0; i < view->len; i++) { + dev = g_ptr_array_index(view, i); + printf("========== %s:%s ==========\n", + dev->subsystem, dev->syspath); + if (!is_drm_subsystem(dev)) { + pr_detail("card device", dev->drm_card); + pr_detail("render device", dev->drm_render); + } + + printf("\n[properties]\n"); + print_ht(dev->props_ht); + printf("\n[attributes]\n"); + print_ht(dev->attrs_ht); + printf("\n"); + } +} + +static struct print_func { + void (*prn)(GPtrArray *); +} print_functions[] = { + [IGT_PRINT_SIMPLE] = { .prn = igt_devs_print_simple }, + [IGT_PRINT_DETAIL] = { .prn = igt_devs_print_detail }, +}; + +/** + * igt_devices_print + * @printtype: IGT_PRINT_SIMPLE or IGT_PRINT_DETAIL + * + * Function is used by 'lsgpu' tool to print device array in simple or detailed + * form. This function is added here instead in 'lsgpu' to avoid exposing + * internal implementation data structures. + */ +void igt_devices_print(enum igt_devices_print_type printtype) +{ + print_functions[printtype].prn(igt_devs.view); +} + +/** + * igt_devices_print_vendors + * + * Print pci id -> vendor mappings. Vendor names printed by this function + * can be used for filters like pci which allows passing vendor - like + * vendor id (8086) or as a string (Intel). + */ +void igt_devices_print_vendors(void) +{ + struct name_value *list = &gpu_vendor_list[0]; + printf("Recognized vendors:\n"); + + printf("%-8s %-16s\n", "PCI ID", "vendor"); + while (list->name) { + struct vendor_spec *vs = (struct vendor_spec *) list->value; + if (vs) + printf("%-8s %-16s\n", list->name, vs->vendor); + list++; + } +} + +/* ------------------------------------------------------------------------- */ +#define FILTER_SPEC_NAME_LEN 32 +#define FILTER_SPEC_DATA_LEN 256 +struct filter_spec { + char name[FILTER_SPEC_NAME_LEN]; + char data[FILTER_SPEC_DATA_LEN]; +}; + +struct filter_func { + GPtrArray *(*filter_function)(struct filter_spec *fspec, + struct filter_func *ffunc); + const char *help; + const char *detail; + + const char *property; + const char *value; +}; + +struct filter_data { + char *vendor; + char *device; + char *card; + char *drm; +}; + +static GHashTable *split_filter_data(const char *data) +{ + GHashTable *ht = g_hash_table_new_full(g_str_hash, g_str_equal, + free, free); + gchar **s; + gchar **strv = g_strsplit(data, ",", -1); + + s = strv; + while (*s) { + char *k, *v; + v = strchr(*s, '='); + if (!v) { + s++; + continue; + } + k = strndup(*s, v - (*s)); + v = strdup(v + 1); + g_hash_table_insert(ht, k, v); + s++; + } + g_strfreev(strv); + + return ht; +} + +static bool get_filter_spec(const char *filter, struct filter_spec *spec) +{ + if (!filter) + return false; + + if (sscanf(filter, "%31[^:]:%255s", spec->name, spec->data) >= 1) + return true; + + return false; +} + +static bool is_vendor_matched(struct igt_device *dev, const char *vendor) +{ + const char *vendor_id; + + if (!dev->vendor || !vendor) + return false; + + /* First we compare vendor id, like 8086 */ + if (!strcasecmp(dev->vendor, vendor)) + return true; + + /* Likely we have vendor string instead of id */ + vendor_id = get_pci_vendor_id_by_name(vendor); + if (!vendor_id) + return false; + + return !strcasecmp(dev->vendor, vendor_id); +} + +/* Filter which matches subsystem:/sys/... path. + * Used as first filter in chain. + */ +static GPtrArray *filter_sys(struct filter_spec *fspec, + struct filter_func *ffunc) +{ + (void) ffunc; + + DBG("filter sys\n"); + if (!strlen(fspec->data)) + return igt_devs.view; + + for (int i = 0; i < igt_devs.devs->len; i++) { + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); + + if (strequal(dev->subsystem, fspec->name) && + strequal(dev->syspath, fspec->data)) { + g_ptr_array_add(igt_devs.view, dev); + break; + } + } + + DBG("Filter sys view size: %d\n", igt_devs.view->len); + + return igt_devs.view; +} + +/* Find drm device using direct path to /dev/dri/. + * It extends filter_sys to allow using drm:/dev/dri/cardX and + * drm:/dev/dri/renderDX filter syntax. + */ +static GPtrArray *filter_drm(struct filter_spec *fspec, + struct filter_func *ffunc) +{ + (void) ffunc; + + DBG("filter drm\n"); + if (!strlen(fspec->data)) + return igt_devs.view; + + for (int i = 0; i < igt_devs.devs->len; i++) { + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); + if (!is_drm_subsystem(dev)) + continue; + + if (strequal(dev->syspath, fspec->data) || + strequal(dev->drm_card, fspec->data) || + strequal(dev->drm_render, fspec->data)) { + g_ptr_array_add(igt_devs.view, dev); + break; + } + } + + DBG("Filter drm view size: %d\n", igt_devs.view->len); + + return igt_devs.view; +} + +/* Find appropriate pci device matching vendor/device/card filter arguments +*/ +static GPtrArray *filter_pci(struct filter_spec *fspec, + struct filter_func *ffunc) +{ + GHashTable *ht; + struct filter_data fdata; + int card = -1; + (void) ffunc; + + DBG("filter pci\n"); + + ht = split_filter_data(fspec->data); + fdata.vendor = g_hash_table_lookup(ht, "vendor"); + fdata.device = g_hash_table_lookup(ht, "device"); + fdata.card = g_hash_table_lookup(ht, "card"); + + if (fdata.card) { + sscanf(fdata.card, "%d", &card); + if (card < 0) { + g_hash_table_destroy(ht); + return igt_devs.view; + } + } else { + card = 0; + } + + for (int i = 0; i < igt_devs.devs->len; i++) { + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); + + if (!is_pci_subsystem(dev)) + continue; + + /* Skip if 'vendor' doesn't match (hex or name) */ + if (fdata.vendor && !is_vendor_matched(dev, fdata.vendor)) + continue; + + /* Skip if 'device' doesn't match */ + if (fdata.device && strcasecmp(fdata.device, dev->device)) + continue; + + /* We get n-th card */ + if (!card) { + g_ptr_array_add(igt_devs.view, dev); + break; + } + card--; + } + + DBG("Filter pci view size: %d\n", igt_devs.view->len); + + g_hash_table_destroy(ht); + + return igt_devs.view; +} + +/* Helper for finding first device matching property:value. +*/ +static GPtrArray *filter_property(struct filter_spec *fspec, + struct filter_func *ffunc) +{ + GHashTable *ht; + struct filter_data fdata; + int card = -1; + const char *property = ffunc->property; + const char *value = ffunc->value; + + if (!property || !value) + return igt_devs.view; + + DBG("filter property/value [%s/%s]\n", property, value); + + ht = split_filter_data(fspec->data); + fdata.card = g_hash_table_lookup(ht, "card"); + + if (fdata.card) { + sscanf(fdata.card, "%d", &card); + if (card < 0) { + g_hash_table_destroy(ht); + return igt_devs.view; + } + } else { + card = 0; + } + + for (int i = 0; i < igt_devs.devs->len; i++) { + const char *prop_value; + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); + + prop_value = get_prop(dev, property); + if (prop_value && !strcmp(prop_value, value)) { + if (!card) { + g_ptr_array_add(igt_devs.view, dev); + break; + } + card--; + } + } + + DBG("Filter view size: %d\n", igt_devs.view->len); + + g_hash_table_destroy(ht); + + return igt_devs.view; +} + +static GPtrArray *filter_vgem(struct filter_spec *fspec, + struct filter_func *ffunc) +{ + GPtrArray *view = filter_property(fspec, ffunc); + if (view) { + struct igt_device *dev = g_ptr_array_index(view, 0); + dev->vs = &v_vgem; + } + return view; +} + +static GPtrArray *filter_vkms(struct filter_spec *fspec, + struct filter_func *ffunc) +{ + return filter_property(fspec, ffunc); +} + +static GPtrArray *filter_vc4(struct filter_spec *fspec, + struct filter_func *ffunc) +{ + GPtrArray *view = filter_property(fspec, ffunc); + if (view) { + struct igt_device *dev = g_ptr_array_index(view, 0); + dev->vs = &v_vc4; + } + return filter_property(fspec, ffunc); +} + +static struct filter_func f_drm = { .filter_function = filter_drm, + .help = "drm:/dev/dri/* path", + .detail = "find drm device by /dev/dri/* node\n", + }; + +static struct filter_func f_pci = { .filter_function = filter_pci, + .help = "pci:[vendor=%04x/name][,device=%04x][,card=%d]", + .detail = "vendor is hex number or vendor name\n", + }; + +static struct filter_func f_vgem = { .filter_function = filter_vgem, + .help = "vgem:[card=%d]", + .detail = "card is n-th vgem card number\n", + .property = "MODALIAS", + .value = "platform:vgem", + }; + +static struct filter_func f_vkms = { .filter_function = filter_vkms, + .help = "vkms:[card=%d]", + .detail = "card is n-th vkms card number\n", + .property = "MODALIAS", + .value = "platform:vkms", + }; + +static struct filter_func f_vc4 = { .filter_function = filter_vc4, + .help = "vc4:[card=%d]", + .detail = "card is n-th vc4 card number\n", + .property = "OF_COMPATIBLE_0", + .value = "brcm,bcm2835-vc4", + }; + +struct name_value filter_definition_list[] = { + { "drm", (gpointer) &f_drm }, + { "pci", (gpointer) &f_pci }, + { "vgem", (gpointer) &f_vgem }, + { "vkms", (gpointer) &f_vkms }, + { "vc4", (gpointer) &f_vc4 }, + { NULL, }, +}; + +/** + * @igt_device_print_filter_types + * + * Print all filters syntax for device selection. + */ +void igt_device_print_filter_types(void) +{ + struct name_value *list = &filter_definition_list[0]; + printf("Filter types:\n---\n"); + printf("%-12s %s\n---\n", "filter", "syntax"); + + printf("%-12s %s\n", "", "direct subsystem:/sys/... path selection\n"); + printf("%-12s %s\n", "", + "ex: drm:/sys/devices/pci0000:00/0000:00:02.0/drm/card0"); + printf("%-12s %s\n", "", + " pci:/sys/devices/pci0000:00/0000:00:02.0"); + printf("%-12s %s\n", "", + " platform:/sys/devices/platform/vgem\n"); + + while (list->name) { + struct filter_func *v = (struct filter_func *) list->value; + printf("%-12s %s\n", list->name, v->help); + printf("%-12s %s\n", "", v->detail); + list++; + } +} + +static GPtrArray *device_filters = NULL; + +#define DEVICE_FILTER_CHECK_ALLOC() \ + do { \ + if (!device_filters) \ + device_filters = g_ptr_array_new_full(2, free); \ + igt_assert(device_filters); \ + } while(0) + +/** + * igt_device_filter_count + * + * Returns number of filters collected in the filter array. + */ +int igt_device_filter_count(void) +{ + DEVICE_FILTER_CHECK_ALLOC(); + + return device_filters->len; +} + +/** + * igt_device_filter_add + * @filter: filter to be stored in filter array + * + * Function allows passing single or more filters within one string. This is + * for CI when it can extract filter from environment variable (and it must + * be single string). So if @filter contains semicolon ';' it treats + * each part as separate filter and adds to the filter array. + * + * Returns number of filters added to filter array. Can be greater than + * 1 if @filter contains more than one filter separated by semicolon. + */ +int igt_device_filter_add(const char *filter) +{ + gchar **strv, **s; + int c = 0; + + DEVICE_FILTER_CHECK_ALLOC(); + + strv = g_strsplit(filter, ";", -1); + + s = strv; + while (*s) { + g_ptr_array_add(device_filters, strdup(*s)); + s++; + } + g_strfreev(strv); + + return c; +} + +/** + * igt_device_filter_get + * @num: Number of filter from filter array + * + * Returns filter string or NULL if @num is out of range of filter array. +*/ +const char *igt_device_filter_get(int num) +{ + DEVICE_FILTER_CHECK_ALLOC(); + + if (num < 0 || num >= device_filters->len) + return NULL; + + return g_ptr_array_index(device_filters, num); +} + +static bool igt_device_filter_apply(const char *filter) +{ + struct filter_spec fspec; + bool ret; + + if (!filter) + return false; + + memset(&fspec, 0, sizeof(fspec)); + ret = get_filter_spec(filter, &fspec); + if (!ret) { + igt_warn("Can't split filter [%s]\n", filter); + return false; + } + + /* Clean view */ + g_ptr_array_remove_range(igt_devs.view, 0, igt_devs.view->len); + + /* If fspec.data contains "/sys" use direct path instead + * contextual filter */ + if (!strncmp(fspec.data, "/sys", 4)) + filter_sys(&fspec, NULL); + else { + struct filter_func *ffunc; + ffunc = g_hash_table_lookup(filter_definition_ht, fspec.name); + if (!ffunc) { + igt_warn("No filter with name [%s]\n", fspec.name); + return false; + } + ffunc->filter_function(&fspec, ffunc); + } + + return true; +} + +#define safe_strncpy(dst, src, size) \ + if ((dst) && (src)) strncpy((dst), (src), (size)) + +/** + * igt_device_card_match + * @filter: filter string + * @card: pointer to igt_device_card struct + * + * Function applies filter to match device from device array. + * + * Returns: + * false - no card pointer was passed or card wasn't matched, + * true - card matched and returned. + */ +bool igt_device_card_match(const char *filter, struct igt_device_card *card) +{ + struct igt_device *dev = NULL; + + if (!card) + return false; + memset(card, 0, sizeof(*card)); + + igt_devices_scan(false); + + if (igt_device_filter_apply(filter) == false) + return false; + + if (!igt_devs.view->len) + return false; + + /* We take first one if more than one card matches filter */ + dev = g_ptr_array_index(igt_devs.view, 0); + card->chipset = DRIVER_ANY; + if (dev->vs) + card->chipset = dev->vs->chipset; + + safe_strncpy(card->subsystem, dev->subsystem, NAME_MAX); + safe_strncpy(card->card, dev->drm_card, NAME_MAX); + safe_strncpy(card->render, dev->drm_render, NAME_MAX); + + return true; +} diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h new file mode 100644 index 00000000..62f7f39a --- /dev/null +++ b/lib/igt_device_scan.h @@ -0,0 +1,69 @@ +/* + * Copyright © 2019 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +#ifndef __IGT_DEVICE_SCAN_H__ +#define __IGT_DEVICE_SCAN_H__ + +#include <limits.h> +#include <igt.h> + +enum igt_devices_print_type { + IGT_PRINT_SIMPLE, + IGT_PRINT_DETAIL, +}; + +struct igt_device_card { + int chipset; + char subsystem[NAME_MAX]; + char card[NAME_MAX]; + char render[NAME_MAX]; +}; + +/* Scan udev subsystems. Do it once unless force is true, what rescans + * devices again */ +void igt_devices_scan(bool force); + +void igt_devices_print(enum igt_devices_print_type printtype); +void igt_devices_print_vendors(void); +void igt_device_print_filter_types(void); + +/* + * Handle device filter collection array. + * IGT can store/retrieve filters passed by user using '--device' args. +*/ + +/* Return number of filters collected */ +int igt_device_filter_count(void); + +/* Add filter(s) to the array. Returns number of filters added (>1 if + * user passes more than one filter separatined by ';') */ +int igt_device_filter_add(const char *filter); + +/* Get n-th filter stored in the array or NULL */ +const char *igt_device_filter_get(int num); + +/* Use filter to match the device and fill card structure */ +bool igt_device_card_match(const char *filter, struct igt_device_card *card); + +#endif /* __IGT_DEVICE_SCAN_H__ */ diff --git a/lib/meson.build b/lib/meson.build index 157624e7..826ebbe3 100644 --- a/lib/meson.build +++ b/lib/meson.build @@ -10,6 +10,7 @@ lib_sources = [ 'igt_color_encoding.c', 'igt_debugfs.c', 'igt_device.c', + 'igt_device_scan.c', 'igt_aux.c', 'igt_gpu_power.c', 'igt_gt.c', diff --git a/tools/Makefile.sources b/tools/Makefile.sources index 50706f41..0e67b654 100644 --- a/tools/Makefile.sources +++ b/tools/Makefile.sources @@ -33,6 +33,7 @@ tools_prog_lists = \ intel_watermark \ intel_gem_info \ intel_gvtg_test \ + lsgpu \ $(NULL) dist_bin_SCRIPTS = intel_gpu_abrt diff --git a/tools/lsgpu.c b/tools/lsgpu.c new file mode 100644 index 00000000..e723f0b2 --- /dev/null +++ b/tools/lsgpu.c @@ -0,0 +1,183 @@ +/* + * Copyright © 2019 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +#include "igt_device_scan.h" +#include "igt.h" +#include <sys/ioctl.h> +#include <fcntl.h> +#include <errno.h> +#include <string.h> +#include <signal.h> + +enum { + OPT_PRINT_DETAIL = 'D', + OPT_LIST_VENDORS = 'v', + OPT_LIST_FILTERS = 'l', + OPT_DEVICE = 'd', + OPT_HELP = 'h' +}; + +bool g_show_vendors; +bool g_list_filters; +bool g_device; +bool g_help; + +static const char *usage_str = + "usage: lsgpu [options]\n\n" + "Options:\n" + " -D, --print-details Print devices with details\n" + " -v, --list-vendors List recognized vendors\n" + " -l, --list-filter-types List registered device filters types\n" + " -d, --device filter Device filter, can be given multiple times\n" + " -h, --help Show this help message and exit\n"; + +static void test_device_open(struct igt_device_card *card) +{ + int fd; + + if (!card) + return; + + fd = drm_open_card(card); + if (fd >= 0) { + printf("Device %s successfully opened\n", card->card); + close(fd); + } else { + if (strlen(card->card)) + printf("Cannot open card %s device\n", card->card); + else + printf("Cannot open card device, empty name\n"); + } + + fd = drm_open_render(card); + if (fd >= 0) { + printf("Device %s successfully opened\n", card->render); + close(fd); + } else { + if (strlen(card->render)) + printf("Cannot open render %s device\n", card->render); + else + printf("Cannot open render device, empty name\n"); + } +} + +static void print_card(struct igt_device_card *card) +{ + if (!card) + return; + + printf("subsystem : %s\n", card->subsystem); + printf("chipset : %x\n", card->chipset); + printf("drm card : %s\n", card->card); + printf("drm render : %s\n", card->render); +} + +int main(int argc, char *argv[]) +{ + static struct option long_options[] = { + {"print-detail", no_argument, NULL, OPT_PRINT_DETAIL}, + {"list-vendors", no_argument, NULL, OPT_LIST_VENDORS}, + {"list-filter-types", no_argument, NULL, OPT_LIST_FILTERS}, + {"device", required_argument, NULL, OPT_DEVICE}, + {"help", no_argument, NULL, OPT_HELP}, + {0, 0, 0, 0} + }; + int c, index = 0; + const char *env; + enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE; + + while ((c = getopt_long(argc, argv, "Dvld:h", + long_options, &index)) != -1) { + switch(c) { + + case OPT_PRINT_DETAIL: + printtype = IGT_PRINT_DETAIL; + break; + case OPT_LIST_VENDORS: + g_show_vendors = true; + break; + case OPT_LIST_FILTERS: + g_list_filters = true; + break; + case OPT_DEVICE: + g_device = true; + igt_device_filter_add(optarg); + break; + case OPT_HELP: + g_help = true; + break; + } + } + + if (g_help) { + printf("%s\n", usage_str); + exit(0); + } + + env = getenv("IGT_DEVICE"); + if (env) { + igt_device_filter_add(env); + g_device = true; + } + + if (g_show_vendors) { + igt_devices_print_vendors(); + return 0; + } + + if (g_list_filters) { + igt_device_print_filter_types(); + return 0; + } + + igt_devices_scan(false); + + if (g_device) { + int n = igt_device_filter_count(); + printf("=== Device filter list ===\n"); + for (int i = 0; i < n; i++) { + printf("[%2d]: %s\n", i, + igt_device_filter_get(i)); + } + printf("\n"); + + printf("=== Testing device open ===\n"); + for (int i = 0; i < n; i++) { + struct igt_device_card card; + const char *filter = igt_device_filter_get(i); + + if (!igt_device_card_match(filter, &card)) + continue; + print_card(&card); + test_device_open(&card); + printf("---\n"); + } + + return 0; + } + + igt_devices_print(printtype); + + return 0; +} diff --git a/tools/meson.build b/tools/meson.build index 6e72b263..9b3a2a69 100644 --- a/tools/meson.build +++ b/tools/meson.build @@ -36,6 +36,7 @@ tools_progs = [ 'intel_gem_info', 'intel_gvtg_test', 'dpcd_reg', + 'lsgpu', ] tool_deps = igt_deps -- 2.21.0 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API Zbigniew Kempczyński @ 2019-08-21 8:09 ` Arkadiusz Hiler 2019-08-21 11:01 ` Zbigniew Kempczyński 2019-08-21 11:04 ` Katarzyna Dec 1 sibling, 1 reply; 13+ messages in thread From: Arkadiusz Hiler @ 2019-08-21 8:09 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev, Petri Latvala, Daniel Vetter On Mon, Aug 12, 2019 at 10:47:17AM +0200, Zbigniew Kempczyński wrote: > Change adds device selection based on scanning drm subsystem > using udev library. > > Tool 'lsgpu' which uses device scanning and selection feature was added. > > 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> > --- > .../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 + > lib/Makefile.sources | 2 + > lib/igt_device_scan.c | 1318 +++++++++++++++++ > lib/igt_device_scan.h | 69 + > lib/meson.build | 1 + > tools/Makefile.sources | 1 + > tools/lsgpu.c | 183 +++ > tools/meson.build | 1 + > 8 files changed, 1576 insertions(+) > create mode 100644 lib/igt_device_scan.c > create mode 100644 lib/igt_device_scan.h > create mode 100644 tools/lsgpu.c > > diff --git a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml > index ac83272f..4b3c38af 100644 > --- a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml > +++ b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml > @@ -23,6 +23,7 @@ > <xi:include href="xml/igt_core.xml"/> > <xi:include href="xml/igt_debugfs.xml"/> > <xi:include href="xml/igt_device.xml"/> > + <xi:include href="xml/igt_device_scan.xml"/> > <xi:include href="xml/igt_draw.xml"/> > <xi:include href="xml/igt_dummyload.xml"/> > <xi:include href="xml/igt_fb.xml"/> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources > index e16de86e..c383a817 100644 > --- a/lib/Makefile.sources > +++ b/lib/Makefile.sources > @@ -25,6 +25,8 @@ lib_source_list = \ > igt_debugfs.h \ > igt_device.c \ > igt_device.h \ > + igt_device_scan.c \ > + igt_device_scan.h \ > igt_aux.c \ > igt_aux.h \ > igt_color_encoding.c \ > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c > new file mode 100644 > index 00000000..4fc165b2 > --- /dev/null > +++ b/lib/igt_device_scan.c > @@ -0,0 +1,1318 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#include "igt.h" > +#include "igt_sysfs.h" > +#include "igt_device.h" > +#include "igt_device_scan.h" > +#include <glib.h> > +#include <libudev.h> > +#include <linux/limits.h> > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <dirent.h> > +#include <fcntl.h> > + > +/** > + * SECTION:igt_device_scan > + * @short_description: Multi-device scanning and selection Multi-device selection is not wrong but the initial interpretation (in my case) was "selecting multiple drm devices at the same time". I think it would work better without "multi", just "device scanning and selection". Any reason why we have this as a separate section in documentation? I think we can merge this with [igt_device] - it has just 3 functions and virtually no documentation of it's own. [igt_device]: https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-igt-device.html > + * @title: Multi-device selection > + * @include: igt.h > + * > + * # Device scanning > + * > + * Device scanning iterates over drm subsystem using udev library > + * to acquire drm devices. > + * For each drm device we also get and store its parent in a device array > + * to allow device selection in more contextual way. > + * > + * Parent devices are bus devices (like pci, platform, etc.) and contain a lot > + * of usable information comparing to drm device. > + * > + * Udev keeps properties and sysattrs for the device as a list what is not > + * convinient for key/value searching. So udev device properties and sysattrs > + * are rewritten to internal hash tables in igt_device structure. > + * > + * # Filters > + * > + * Device selection can be done using filters. Filters allows sophisticated > + * device matching and selection. Previously mentioned properties and sysattrs > + * collected in igt_device hash tables simplify writing filter implementation. > + * > + * Direct device selection filter > + * is special filter and it is checked first. Then contextual filter is chosen > + * depending on filter name. > + * > + * Direct device selection filter must be: > + * > + * |[<!-- language="plain" --> > + * subsystem:/sys > + * ]| > + * > + * So, when user passes filter which looks like follows: > + * |[<!-- language="plain" --> > + * - drm:/sys/devices/pci0000:00/0000:00:02.0/drm/card0 > + * - pci:/sys/devices/pci0000:00/0000:00:02.0 > + * - platform:/sys/devices/platform/vgem > + * ]| > + * > + * device from array which has subsystem and sys path will be returned. > + * > + * When /sys is not specified after colon contextual filters are taken > + * into account. Drm device occurs in the system when appropriate module > + * is loaded and device is detected (or exposed for platform devices). Loading > + * drivers in different order can be problematic from CI point of view, where > + * expectation is to get same device especially when multiple gpu devices > + * exists in the system. For such devices its parent location on pci bus > + * is constant and allows appropriate device selection using for example > + * vendor / device ids. > + * > + * For tests which opens more than one device device filter collection API > + * can be used. You can add filter to the array using igt_device_filter_add(), > + * get nth filter stored using igt_device_filter_get() and return > + * #igt_device_card using filter in igt_device_card_match(). I would reword that slightly: * For tests which opens more than one device device filter collection API * can be used. You can add a filter to the array using igt_device_filter_add(), * get the nth filter stored using igt_device_filter_get() and get * #igt_device_card using igt_device_card_match() with that filter. But that aside - extending the point from the review of patch 2: To handle multiple devices in a single test you would have to: * check existing filters * add any missing ones * open all the cards using 1,2...n filters Which still feels awkward and quite choreful. I don't think that managing filters from inside a test is the greatest idea. Library should hide that. If you have use cases for this thing hit me with some pseudocode, I would like to understand. > + * > + * Implemented filters: > + * > + * - drm: get drm /dev/dri/... device directly > + * > + * |[<!-- language="plain" --> > + * drm:/dev/dri/... > + * ]| > + * > + * - pci: select device using pci properties > + * |[<!-- language="plain" --> > + * pci:[vendor=%04x/name][,device=%04x][,card=%d] > + * ]| > + * > + * Filter allows device selection using vendor (hex or name), device id > + * (hex) and nth-card from all matches. For example if there're 4 pci > + * cards installed (let two cards have 1234 and other two 1235 device id, > + * all of them of vendor Intel) you can select one using: > + * > + * |[<!-- language="plain" --> > + * pci:vendor=Intel,device=1234,card=0 > + * ]| > + * > + * or > + * > + * |[<!-- language="plain" --> > + * pci:vendor=8086,device=1234,card=0 > + * ]| > + * > + * This takes first device with 1234 id for Intel vendor (8086). > + * > + * |[<!-- language="plain" --> > + * pci:vendor=Intel,device=1234,card=1 > + * ]| > + * > + * or > + * > + * |[<!-- language="plain" --> > + * pci:vendor=8086,device=1234,card=1 > + * ]| > + * > + * It selects second one. > + * > + * As order on pci bus doesn't change (unless you'll add new device or > + * reorder existing one) device selection using this filter will always > + * return you same device regardless module loading time. > + * > + * - vgem: select virtual gem device > + * > + * |[<!-- language="plain" --> > + * vgem:[card=%d] > + * ]| > + * > + * For example: > + * > + * |[<!-- language="plain" --> > + * vgem: > + * vgem:card=0 > + * ]| > + * > + * Both selects first vgem device (no idea if more than one device will > + * be visible, if yes use card= argument to point right one). > + * > + * - vkms: select virtual kms device > + * > + * |[<!-- language="plain" --> > + * vkms:[card=%d] > + * ]| > + * > + * For example: > + * > + * |[<!-- language="plain" --> > + * vkms: > + * vkms:card=0 > + * ]| > + * > + * Both selects first vkms device. Same as for vgem I assume more than > + * one device can appear in the future. > + * > + * - vc4: select VC4 device > + * > + * |[<!-- language="plain" --> > + * vc4:[card=%d] > + * ]| > + * > + * For example: > + * > + * |[<!-- language="plain" --> > + * vc4: > + * vc4:card=0 > + * ]| > + * > + * Both selects first VC4 device. At the moment Rasperry PI has only > + * one gpu, but filter is ready if there will be more. > + * > + * # lsgpu > + * > + * Scanned devices can be displayed using 'lsgpu' tool. Tool also displays > + * properties and sysattrs (-D switch, means detail) which can be used during > + * filter implementation. "The devices can be scanned and displayed" "The tool also displays" "-D switch, for details" Filter implementation? I am not sure but I think this paragraph should read "which can be helpful when writing filters". > + * > + * Tool can also be used to check does the implemented filter works as expected. > + * To select device using filter use '-d' or '--device' argument like: "lsgpu can also be used to try out filters" > + * > + * |[<!-- language="plain" --> > + * ./lsgpu -d 'pci:vendor=Intel' > + * === Device filter list === > + * [ 0]: pci:vendor=Intel > + > + * === Testing device open === > + * subsystem : pci > + * chipset : 1 > + * drm card : /dev/dri/card0 > + * drm render : /dev/dri/renderD128 > + * Device /dev/dri/card0 successfully opened > + * Device /dev/dri/renderD128 successfully opened > + * ]| > + * > + * Apart of selecting device lsgpu try to open card and render nodes. I think this could use some context on why, e.g. "Additionally lsgpu tries to open the card and render nodes to verify permissions..." > +*/ > + > +//#define DEBUG_DEVICE_SCAN leftover define > +#ifdef DEBUG_DEVICE_SCAN > +#define DBG(...) \ > +{ \ > + struct timeval tm; \ > + gettimeofday(&tm, NULL); \ > + printf("%10ld.%03ld: ", tm.tv_sec, tm.tv_usec); \ > + printf(__VA_ARGS__); \ > + } > + > +#else > +#define DBG(...) {} > +#endif > + > +#define strequal(x, y) ((x) && (y) && !strcmp((x), (y))) > +#define IGT_DRM_PATH "/dev/dri" > + > +static GHashTable *sysattrs_blacklist_ht; //sysattrs we don't want to read > +static GHashTable *gpu_vendor_ht; //search id -> vendor_spec mapping > +static GHashTable *filter_definition_ht; //supported filters (pci=..., etc.) /* we are using plain old C-style comments, even at the end of the line */ <SNIP> I'll review the actual code in more detail once I have enough caffeine in my bloodstream to handle glib :-) > diff --git a/tools/lsgpu.c b/tools/lsgpu.c > new file mode 100644 > index 00000000..e723f0b2 > --- /dev/null > +++ b/tools/lsgpu.c > @@ -0,0 +1,183 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#include "igt_device_scan.h" > +#include "igt.h" > +#include <sys/ioctl.h> > +#include <fcntl.h> > +#include <errno.h> > +#include <string.h> > +#include <signal.h> > + > +enum { > + OPT_PRINT_DETAIL = 'D', > + OPT_LIST_VENDORS = 'v', > + OPT_LIST_FILTERS = 'l', > + OPT_DEVICE = 'd', > + OPT_HELP = 'h' > +}; > + > +bool g_show_vendors; > +bool g_list_filters; > +bool g_device; > +bool g_help; > + > +static const char *usage_str = > + "usage: lsgpu [options]\n\n" > + "Options:\n" > + " -D, --print-details Print devices with details\n" > + " -v, --list-vendors List recognized vendors\n" > + " -l, --list-filter-types List registered device filters types\n" > + " -d, --device filter Device filter, can be given multiple times\n" > + " -h, --help Show this help message and exit\n"; > + > +static void test_device_open(struct igt_device_card *card) > +{ > + int fd; > + > + if (!card) > + return; > + > + fd = drm_open_card(card); > + if (fd >= 0) { > + printf("Device %s successfully opened\n", card->card); > + close(fd); > + } else { > + if (strlen(card->card)) > + printf("Cannot open card %s device\n", card->card); > + else > + printf("Cannot open card device, empty name\n"); > + } > + > + fd = drm_open_render(card); > + if (fd >= 0) { > + printf("Device %s successfully opened\n", card->render); > + close(fd); > + } else { > + if (strlen(card->render)) > + printf("Cannot open render %s device\n", card->render); > + else > + printf("Cannot open render device, empty name\n"); > + } > +} > + > +static void print_card(struct igt_device_card *card) > +{ > + if (!card) > + return; > + > + printf("subsystem : %s\n", card->subsystem); > + printf("chipset : %x\n", card->chipset); > + printf("drm card : %s\n", card->card); > + printf("drm render : %s\n", card->render); > +} > + > +int main(int argc, char *argv[]) > +{ > + static struct option long_options[] = { > + {"print-detail", no_argument, NULL, OPT_PRINT_DETAIL}, > + {"list-vendors", no_argument, NULL, OPT_LIST_VENDORS}, > + {"list-filter-types", no_argument, NULL, OPT_LIST_FILTERS}, > + {"device", required_argument, NULL, OPT_DEVICE}, > + {"help", no_argument, NULL, OPT_HELP}, > + {0, 0, 0, 0} > + }; > + int c, index = 0; > + const char *env; > + enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE; > + > + while ((c = getopt_long(argc, argv, "Dvld:h", > + long_options, &index)) != -1) { > + switch(c) { > + > + case OPT_PRINT_DETAIL: > + printtype = IGT_PRINT_DETAIL; > + break; > + case OPT_LIST_VENDORS: > + g_show_vendors = true; > + break; > + case OPT_LIST_FILTERS: > + g_list_filters = true; > + break; > + case OPT_DEVICE: > + g_device = true; > + igt_device_filter_add(optarg); > + break; > + case OPT_HELP: > + g_help = true; > + break; > + } > + } > + > + if (g_help) { > + printf("%s\n", usage_str); > + exit(0); > + } > + > + env = getenv("IGT_DEVICE"); > + if (env) { > + igt_device_filter_add(env); > + g_device = true; > + } > + > + if (g_show_vendors) { > + igt_devices_print_vendors(); > + return 0; > + } > + > + if (g_list_filters) { > + igt_device_print_filter_types(); > + return 0; > + } > + > + igt_devices_scan(false); > + > + if (g_device) { > + int n = igt_device_filter_count(); > + printf("=== Device filter list ===\n"); > + for (int i = 0; i < n; i++) { > + printf("[%2d]: %s\n", i, > + igt_device_filter_get(i)); > + } > + printf("\n"); > + > + printf("=== Testing device open ===\n"); > + for (int i = 0; i < n; i++) { > + struct igt_device_card card; > + const char *filter = igt_device_filter_get(i); > + > + if (!igt_device_card_match(filter, &card)) > + continue; > + print_card(&card); > + test_device_open(&card); > + printf("---\n"); > + } > + > + return 0; > + } > + > + igt_devices_print(printtype); > + > + return 0; > +} > diff --git a/tools/meson.build b/tools/meson.build > index 6e72b263..9b3a2a69 100644 > --- a/tools/meson.build > +++ b/tools/meson.build > @@ -36,6 +36,7 @@ tools_progs = [ > 'intel_gem_info', > 'intel_gvtg_test', > 'dpcd_reg', > + 'lsgpu', > ] > tool_deps = igt_deps Oh, I already like --device and lsgpu :-) Finally I'll be able to run kms_ tests on i915 on my personal rig, which also has a discrete card. i915 usually gets enumerated second for me... Thanks for doing this! -- Cheers, Arek _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API 2019-08-21 8:09 ` Arkadiusz Hiler @ 2019-08-21 11:01 ` Zbigniew Kempczyński 0 siblings, 0 replies; 13+ messages in thread From: Zbigniew Kempczyński @ 2019-08-21 11:01 UTC (permalink / raw) To: Arkadiusz Hiler; +Cc: igt-dev, Petri Latvala, Daniel Vetter On Wed, Aug 21, 2019 at 11:09:11AM +0300, Arkadiusz Hiler wrote: > On Mon, Aug 12, 2019 at 10:47:17AM +0200, Zbigniew Kempczyński wrote: > > Change adds device selection based on scanning drm subsystem > > using udev library. > > > > Tool 'lsgpu' which uses device scanning and selection feature was added. > > <cut> > > + > > +/** > > + * SECTION:igt_device_scan > > + * @short_description: Multi-device scanning and selection > > Multi-device selection is not wrong but the initial interpretation (in my > case) was "selecting multiple drm devices at the same time". > > I think it would work better without "multi", just "device scanning and > selection". > > Any reason why we have this as a separate section in documentation? I > think we can merge this with [igt_device] - it has just 3 functions and > virtually no documentation of it's own. > > [igt_device]: https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-igt-device.html I agree regarding 'multi' word. I've chosen another file (not igt_device.c) because technically it is related to scanning udev, not access to device itself. I assumed that igt_device code is related to doing operations on drm device fd. > > + * For tests which opens more than one device device filter collection API > > + * can be used. You can add filter to the array using igt_device_filter_add(), > > + * get nth filter stored using igt_device_filter_get() and return > > + * #igt_device_card using filter in igt_device_card_match(). > > I would reword that slightly: > > * For tests which opens more than one device device filter collection API > * can be used. You can add a filter to the array using igt_device_filter_add(), > * get the nth filter stored using igt_device_filter_get() and get > * #igt_device_card using igt_device_card_match() with that filter. > > But that aside - extending the point from the review of patch 2: > > To handle multiple devices in a single test you would have to: > * check existing filters > * add any missing ones > * open all the cards using 1,2...n filters > > Which still feels awkward and quite choreful. I don't think that > managing filters from inside a test is the greatest idea. Library > should hide that. > > If you have use cases for this thing hit me with some pseudocode, I would > like to understand. Library helpers can be implemented in drmtest.c/h and in my opinion there's appropriate place for that. Device selection adds a code you can use for that. <cut> > > + * Both selects first VC4 device. At the moment Rasperry PI has only > > + * one gpu, but filter is ready if there will be more. > > + * > > + * # lsgpu > > + * > > + * Scanned devices can be displayed using 'lsgpu' tool. Tool also displays > > + * properties and sysattrs (-D switch, means detail) which can be used during > > + * filter implementation. > > "The devices can be scanned and displayed" > > "The tool also displays" > > "-D switch, for details" > > Filter implementation? I am not sure but I think this paragraph should > read "which can be helpful when writing filters". Ok. > > > + * > > + * Tool can also be used to check does the implemented filter works as expected. > > + * To select device using filter use '-d' or '--device' argument like: > > "lsgpu can also be used to try out filters" Ok. > > > + * > > + * |[<!-- language="plain" --> > > + * ./lsgpu -d 'pci:vendor=Intel' > > + * === Device filter list === > > + * [ 0]: pci:vendor=Intel > > + > > + * === Testing device open === > > + * subsystem : pci > > + * chipset : 1 > > + * drm card : /dev/dri/card0 > > + * drm render : /dev/dri/renderD128 > > + * Device /dev/dri/card0 successfully opened > > + * Device /dev/dri/renderD128 successfully opened > > + * ]| > > + * > > + * Apart of selecting device lsgpu try to open card and render nodes. > > I think this could use some context on why, e.g. > "Additionally lsgpu tries to open the card and render nodes to verify > permissions..." Ok. > > > +*/ > > + > > +//#define DEBUG_DEVICE_SCAN > > leftover define I'll remove that. > > > +#ifdef DEBUG_DEVICE_SCAN > > +#define DBG(...) \ > > +{ \ > > + struct timeval tm; \ > > + gettimeofday(&tm, NULL); \ > > + printf("%10ld.%03ld: ", tm.tv_sec, tm.tv_usec); \ > > + printf(__VA_ARGS__); \ > > + } > > + > > +#else > > +#define DBG(...) {} > > +#endif > > + > > +#define strequal(x, y) ((x) && (y) && !strcmp((x), (y))) > > +#define IGT_DRM_PATH "/dev/dri" > > + > > +static GHashTable *sysattrs_blacklist_ht; //sysattrs we don't want to read > > +static GHashTable *gpu_vendor_ht; //search id -> vendor_spec mapping > > +static GHashTable *filter_definition_ht; //supported filters (pci=..., etc.) > > /* we are using plain old C-style comments, even at the end of the line */ This is not true at whole project but ok, I'll change that. > > <SNIP> > > I'll review the actual code in more detail once I have enough caffeine in my > bloodstream to handle glib :-) > > > diff --git a/tools/lsgpu.c b/tools/lsgpu.c > > new file mode 100644 > > index 00000000..e723f0b2 > > --- /dev/null > > +++ b/tools/lsgpu.c > > @@ -0,0 +1,183 @@ > > +/* > > + * Copyright © 2019 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the next > > + * paragraph) shall be included in all copies or substantial portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > + * IN THE SOFTWARE. > > + * > > + */ > > + > > +#include "igt_device_scan.h" > > +#include "igt.h" > > +#include <sys/ioctl.h> > > +#include <fcntl.h> > > +#include <errno.h> > > +#include <string.h> > > +#include <signal.h> > > + > > +enum { > > + OPT_PRINT_DETAIL = 'D', > > + OPT_LIST_VENDORS = 'v', > > + OPT_LIST_FILTERS = 'l', > > + OPT_DEVICE = 'd', > > + OPT_HELP = 'h' > > +}; > > + > > +bool g_show_vendors; > > +bool g_list_filters; > > +bool g_device; > > +bool g_help; > > + > > +static const char *usage_str = > > + "usage: lsgpu [options]\n\n" > > + "Options:\n" > > + " -D, --print-details Print devices with details\n" > > + " -v, --list-vendors List recognized vendors\n" > > + " -l, --list-filter-types List registered device filters types\n" > > + " -d, --device filter Device filter, can be given multiple times\n" > > + " -h, --help Show this help message and exit\n"; > > + > > +static void test_device_open(struct igt_device_card *card) > > +{ > > + int fd; > > + > > + if (!card) > > + return; > > + > > + fd = drm_open_card(card); > > + if (fd >= 0) { > > + printf("Device %s successfully opened\n", card->card); > > + close(fd); > > + } else { > > + if (strlen(card->card)) > > + printf("Cannot open card %s device\n", card->card); > > + else > > + printf("Cannot open card device, empty name\n"); > > + } > > + > > + fd = drm_open_render(card); > > + if (fd >= 0) { > > + printf("Device %s successfully opened\n", card->render); > > + close(fd); > > + } else { > > + if (strlen(card->render)) > > + printf("Cannot open render %s device\n", card->render); > > + else > > + printf("Cannot open render device, empty name\n"); > > + } > > +} > > + > > +static void print_card(struct igt_device_card *card) > > +{ > > + if (!card) > > + return; > > + > > + printf("subsystem : %s\n", card->subsystem); > > + printf("chipset : %x\n", card->chipset); > > + printf("drm card : %s\n", card->card); > > + printf("drm render : %s\n", card->render); > > +} > > + > > +int main(int argc, char *argv[]) > > +{ > > + static struct option long_options[] = { > > + {"print-detail", no_argument, NULL, OPT_PRINT_DETAIL}, > > + {"list-vendors", no_argument, NULL, OPT_LIST_VENDORS}, > > + {"list-filter-types", no_argument, NULL, OPT_LIST_FILTERS}, > > + {"device", required_argument, NULL, OPT_DEVICE}, > > + {"help", no_argument, NULL, OPT_HELP}, > > + {0, 0, 0, 0} > > + }; > > + int c, index = 0; > > + const char *env; > > + enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE; > > + > > + while ((c = getopt_long(argc, argv, "Dvld:h", > > + long_options, &index)) != -1) { > > + switch(c) { > > + > > + case OPT_PRINT_DETAIL: > > + printtype = IGT_PRINT_DETAIL; > > + break; > > + case OPT_LIST_VENDORS: > > + g_show_vendors = true; > > + break; > > + case OPT_LIST_FILTERS: > > + g_list_filters = true; > > + break; > > + case OPT_DEVICE: > > + g_device = true; > > + igt_device_filter_add(optarg); > > + break; > > + case OPT_HELP: > > + g_help = true; > > + break; > > + } > > + } > > + > > + if (g_help) { > > + printf("%s\n", usage_str); > > + exit(0); > > + } > > + > > + env = getenv("IGT_DEVICE"); > > + if (env) { > > + igt_device_filter_add(env); > > + g_device = true; > > + } > > + > > + if (g_show_vendors) { > > + igt_devices_print_vendors(); > > + return 0; > > + } > > + > > + if (g_list_filters) { > > + igt_device_print_filter_types(); > > + return 0; > > + } > > + > > + igt_devices_scan(false); > > + > > + if (g_device) { > > + int n = igt_device_filter_count(); > > + printf("=== Device filter list ===\n"); > > + for (int i = 0; i < n; i++) { > > + printf("[%2d]: %s\n", i, > > + igt_device_filter_get(i)); > > + } > > + printf("\n"); > > + > > + printf("=== Testing device open ===\n"); > > + for (int i = 0; i < n; i++) { > > + struct igt_device_card card; > > + const char *filter = igt_device_filter_get(i); > > + > > + if (!igt_device_card_match(filter, &card)) > > + continue; > > + print_card(&card); > > + test_device_open(&card); > > + printf("---\n"); > > + } > > + > > + return 0; > > + } > > + > > + igt_devices_print(printtype); > > + > > + return 0; > > +} > > diff --git a/tools/meson.build b/tools/meson.build > > index 6e72b263..9b3a2a69 100644 > > --- a/tools/meson.build > > +++ b/tools/meson.build > > @@ -36,6 +36,7 @@ tools_progs = [ > > 'intel_gem_info', > > 'intel_gvtg_test', > > 'dpcd_reg', > > + 'lsgpu', > > ] > > tool_deps = igt_deps > > Oh, I already like --device and lsgpu :-) > > Finally I'll be able to run kms_ tests on i915 on my personal rig, which > also has a discrete card. i915 usually gets enumerated second for me... > > Thanks for doing this! > > -- > Cheers, > Arek I'll push v5 soon. Zbigniew _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API Zbigniew Kempczyński 2019-08-21 8:09 ` Arkadiusz Hiler @ 2019-08-21 11:04 ` Katarzyna Dec 2019-08-21 11:58 ` Katarzyna Dec 2019-08-22 7:23 ` Zbigniew Kempczyński 1 sibling, 2 replies; 13+ messages in thread From: Katarzyna Dec @ 2019-08-21 11:04 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev, Petri Latvala, Daniel Vetter On Mon, Aug 12, 2019 at 10:47:17AM +0200, Zbigniew Kempczyński wrote: Zbyszku, I saw that Arek has commented your changes, but I did not read it in details, so some of my comments can be similar to his. I wanted to have clear head starting this review :). > Change adds device selection based on scanning drm subsystem > using udev library. > > Tool 'lsgpu' which uses device scanning and selection feature was added. > > 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> > --- > .../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 + > lib/Makefile.sources | 2 + > lib/igt_device_scan.c | 1318 +++++++++++++++++ > lib/igt_device_scan.h | 69 + > lib/meson.build | 1 + > tools/Makefile.sources | 1 + > tools/lsgpu.c | 183 +++ > tools/meson.build | 1 + > 8 files changed, 1576 insertions(+) > create mode 100644 lib/igt_device_scan.c > create mode 100644 lib/igt_device_scan.h > create mode 100644 tools/lsgpu.c > > diff --git a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml > index ac83272f..4b3c38af 100644 > --- a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml > +++ b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml > @@ -23,6 +23,7 @@ > <xi:include href="xml/igt_core.xml"/> > <xi:include href="xml/igt_debugfs.xml"/> > <xi:include href="xml/igt_device.xml"/> > + <xi:include href="xml/igt_device_scan.xml"/> > <xi:include href="xml/igt_draw.xml"/> > <xi:include href="xml/igt_dummyload.xml"/> > <xi:include href="xml/igt_fb.xml"/> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources > index e16de86e..c383a817 100644 > --- a/lib/Makefile.sources > +++ b/lib/Makefile.sources > @@ -25,6 +25,8 @@ lib_source_list = \ > igt_debugfs.h \ > igt_device.c \ > igt_device.h \ > + igt_device_scan.c \ > + igt_device_scan.h \ > igt_aux.c \ > igt_aux.h \ > igt_color_encoding.c \ > diff --git a/lib/igt_device_scan.c b/lib/igt_device_scan.c > new file mode 100644 > index 00000000..4fc165b2 > --- /dev/null > +++ b/lib/igt_device_scan.c > @@ -0,0 +1,1318 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#include "igt.h" > +#include "igt_sysfs.h" > +#include "igt_device.h" > +#include "igt_device_scan.h" > +#include <glib.h> > +#include <libudev.h> > +#include <linux/limits.h> > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <dirent.h> > +#include <fcntl.h> > + > +/** > + * SECTION:igt_device_scan > + * @short_description: Multi-device scanning and selection > + * @title: Multi-device selection > + * @include: igt.h > + * > + * # Device scanning > + * > + * Device scanning iterates over drm subsystem using udev library > + * to acquire drm devices. > + * For each drm device we also get and store its parent in a device array > + * to allow device selection in more contextual way. > + * > + * Parent devices are bus devices (like pci, platform, etc.) and contain a lot > + * of usable information comparing to drm device. > + * > + * Udev keeps properties and sysattrs for the device as a list what is not > + * convinient for key/value searching. So udev device properties and sysattrs > + * are rewritten to internal hash tables in igt_device structure. > + * > + * # Filters > + * > + * Device selection can be done using filters. Filters allows sophisticated s/allows/allow :) > + * device matching and selection. Previously mentioned properties and sysattrs > + * collected in igt_device hash tables simplify writing filter implementation. > + * > + * Direct device selection filter > + * is special filter and it is checked first. Then contextual filter is chosen > + * depending on filter name. > + * > + * Direct device selection filter must be: > + * > + * |[<!-- language="plain" --> > + * subsystem:/sys > + * ]| > + * > + * So, when user passes filter which looks like follows: > + * |[<!-- language="plain" --> > + * - drm:/sys/devices/pci0000:00/0000:00:02.0/drm/card0 > + * - pci:/sys/devices/pci0000:00/0000:00:02.0 > + * - platform:/sys/devices/platform/vgem > + * ]| I tried this one locally and what seems to be missing is clear message that device is not there. For now we have detailed information for existing device and empty headlines for non-existing. I would add some warning, e.g. 'Device not found'. > + * > + * device from array which has subsystem and sys path will be returned. > + * > + * When /sys is not specified after colon contextual filters are taken > + * into account. Drm device occurs in the system when appropriate module > + * is loaded and device is detected (or exposed for platform devices). Loading > + * drivers in different order can be problematic from CI point of view, where > + * expectation is to get same device especially when multiple gpu devices > + * exists in the system. For such devices its parent location on pci bus > + * is constant and allows appropriate device selection using for example > + * vendor / device ids. > + * > + * For tests which opens more than one device device filter collection API > + * can be used. You can add filter to the array using igt_device_filter_add(), > + * get nth filter stored using igt_device_filter_get() and return > + * #igt_device_card using filter in igt_device_card_match(). > + * > + * Implemented filters: > + * > + * - drm: get drm /dev/dri/... device directly > + * > + * |[<!-- language="plain" --> > + * drm:/dev/dri/... > + * ]| > + * > + * - pci: select device using pci properties > + * |[<!-- language="plain" --> > + * pci:[vendor=%04x/name][,device=%04x][,card=%d] > + * ]| > + * > + * Filter allows device selection using vendor (hex or name), device id > + * (hex) and nth-card from all matches. For example if there're 4 pci > + * cards installed (let two cards have 1234 and other two 1235 device id, > + * all of them of vendor Intel) you can select one using: > + * > + * |[<!-- language="plain" --> > + * pci:vendor=Intel,device=1234,card=0 > + * ]| > + * > + * or > + * > + * |[<!-- language="plain" --> > + * pci:vendor=8086,device=1234,card=0 > + * ]| > + * > + * This takes first device with 1234 id for Intel vendor (8086). > + * > + * |[<!-- language="plain" --> > + * pci:vendor=Intel,device=1234,card=1 > + * ]| > + * > + * or > + * > + * |[<!-- language="plain" --> > + * pci:vendor=8086,device=1234,card=1 > + * ]| > + * > + * It selects second one. > + * > + * As order on pci bus doesn't change (unless you'll add new device or > + * reorder existing one) device selection using this filter will always > + * return you same device regardless module loading time. > + * > + * - vgem: select virtual gem device > + * > + * |[<!-- language="plain" --> > + * vgem:[card=%d] > + * ]| > + * > + * For example: > + * > + * |[<!-- language="plain" --> > + * vgem: > + * vgem:card=0 > + * ]| > + * > + * Both selects first vgem device (no idea if more than one device will > + * be visible, if yes use card= argument to point right one). > + * > + * - vkms: select virtual kms device > + * > + * |[<!-- language="plain" --> > + * vkms:[card=%d] > + * ]| > + * > + * For example: > + * > + * |[<!-- language="plain" --> > + * vkms: > + * vkms:card=0 > + * ]| > + * > + * Both selects first vkms device. Same as for vgem I assume more than > + * one device can appear in the future. > + * > + * - vc4: select VC4 device > + * > + * |[<!-- language="plain" --> > + * vc4:[card=%d] > + * ]| > + * > + * For example: > + * > + * |[<!-- language="plain" --> > + * vc4: > + * vc4:card=0 > + * ]| > + * > + * Both selects first VC4 device. At the moment Rasperry PI has only > + * one gpu, but filter is ready if there will be more. > + * > + * # lsgpu > + * > + * Scanned devices can be displayed using 'lsgpu' tool. Tool also displays > + * properties and sysattrs (-D switch, means detail) which can be used during > + * filter implementation. > + * > + * Tool can also be used to check does the implemented filter works as expected. > + * To select device using filter use '-d' or '--device' argument like: > + * > + * |[<!-- language="plain" --> > + * ./lsgpu -d 'pci:vendor=Intel' > + * === Device filter list === > + * [ 0]: pci:vendor=Intel > + > + * === Testing device open === > + * subsystem : pci > + * chipset : 1 > + * drm card : /dev/dri/card0 > + * drm render : /dev/dri/renderD128 > + * Device /dev/dri/card0 successfully opened > + * Device /dev/dri/renderD128 successfully opened > + * ]| > + * > + * Apart of selecting device lsgpu try to open card and render nodes. I did not see full picture yet, so maybe the answer it somewhere already, anyway: What can I do with this device opened and returned? Or this is just an information that it exists and can be used by test/filter? > +*/ ^ End of comment section is not aligned, but this is really minor issue. All similar issues can be found by using checkpatch.pl script :). Looking furhter in this file there is a lot of code style warnings that are worth fixing. > + > +//#define DEBUG_DEVICE_SCAN Why ^ this line is commented? > +#ifdef DEBUG_DEVICE_SCAN > +#define DBG(...) \ > +{ \ > + struct timeval tm; \ > + gettimeofday(&tm, NULL); \ > + printf("%10ld.%03ld: ", tm.tv_sec, tm.tv_usec); \ > + printf(__VA_ARGS__); \ > + } > + > +#else > +#define DBG(...) {} > +#endif > + > +#define strequal(x, y) ((x) && (y) && !strcmp((x), (y))) > +#define IGT_DRM_PATH "/dev/dri" > + > +static GHashTable *sysattrs_blacklist_ht; //sysattrs we don't want to read > +static GHashTable *gpu_vendor_ht; //search id -> vendor_spec mapping > +static GHashTable *filter_definition_ht; //supported filters (pci=..., etc.) > + > +/* Generic name->value struct */ > +struct name_value { > + const char *name; > + gpointer *value; > +}; > + > +/* Vendor specific data */ > +struct vendor_spec { > + const char *vendor; > + const char *match; > + int chipset; > +}; > + > +struct igt_device { > + /* Filled for drm devices */ > + struct igt_device *parent; > + > + /* Point to vendor spec if can be found */ > + struct vendor_spec *vs; > + > + /* Properties / sysattrs rewriten from udev lists */ > + GHashTable *props_ht; > + GHashTable *attrs_ht; > + > + /* Most usable variables from udev device */ > + char *subsystem; > + char *syspath; > + char *devnode; > + > + /* /dev/dri/... paths */ > + char *drm_card; > + char *drm_render; > + > + /* For pci subsystem */ > + char *vendor; > + char *device; > +}; > + > +/* Scanned devices */ > +struct igt_devices { > + GPtrArray *devs; //all devices > + GPtrArray *view; //filtered view > + bool devs_scanned; > +}; > + > +/* Scanned devices holder */ > +static struct igt_devices igt_devs; > + > +static struct vendor_spec v_intel = { .vendor = "Intel", > + .chipset = DRIVER_INTEL > + }; > +static struct vendor_spec v_amd = { .vendor = "AMD", > + .chipset = DRIVER_AMDGPU > + }; > +static struct vendor_spec v_vgem = { .vendor = "Virtual-GEM", > + .chipset = DRIVER_VGEM > + }; > +static struct vendor_spec v_vc4 = { .vendor = "Broadcom", > + .chipset = DRIVER_VC4 > + }; > + > +/* Mapping vendor id => vendor_spec should be unique (vendor string matching > + * is written around this). > + * > + * Keys must be defined as follows: > + * PCI devices: PCI_SLOT_ID -> vendor_spec. > +*/ > +struct name_value gpu_vendor_list[] = { > + { "8086", (gpointer) &v_intel }, > + { "1002", (gpointer) &v_amd }, > + { NULL, }, > +}; > + > +/* Generic hash table fill function, requires name / value ptrs array */ > +static void fill_ht(GHashTable **ht, struct name_value *data) > +{ > + if (*ht) > + return; > + > + *ht = g_hash_table_new(g_str_hash, g_str_equal); > + igt_assert(*ht); > + > + while (data->name) { > + g_hash_table_insert(*ht, > + (gpointer) data->name, > + data->value); > + data++; > + } > +} > + > +#define get_vendor_spec(prop) \ > + g_hash_table_lookup(gpu_vendor_ht, prop) > + > +/* Go through whole vendor list and compare against vendor field. > + * Used mostly with vendor=... filter parameter when PCI id is not matched. > +*/ > +static const char *get_pci_vendor_id_by_name(const char *name) > +{ > + struct name_value *data = &gpu_vendor_list[0]; > + > + while (data->name) { > + struct vendor_spec *vs = (struct vendor_spec *) data->value; > + if (!strcasecmp(name, vs->vendor)) > + return data->name; > + data++; > + } > + > + return NULL; > +} > + > +/* Reading sysattr values can take time (even seconds), > + * we want to avoid reading such keys. > +*/ > +static void populate_blacklist_keys(void) > +{ > + const char *keys[] = { "config", "modalias", "modes", > + "resource", > + "resource0", "resource1", "resource2", > + "resource3", "resource4", "resource5", > + "resource0_wc", "resource1_wc", "resource2_wc", > + "resource3_wc", "resource4_wc", "resource5_wc", > + "driver", > + "uevent", NULL}; > + const char *key; > + int i = 0; > + > + if (sysattrs_blacklist_ht) > + return; > + > + sysattrs_blacklist_ht = g_hash_table_new(g_str_hash, g_str_equal); > + igt_assert(sysattrs_blacklist_ht); > + > + while ((key = keys[i++])) > + g_hash_table_add(sysattrs_blacklist_ht, (gpointer) key); > +} > + > +#define is_on_blacklist(key) \ > + g_hash_table_contains(sysattrs_blacklist_ht, key) > + > +static struct igt_device *igt_device_new(void) > +{ > + struct igt_device *dev; > + dev = calloc(1, sizeof(struct igt_device)); > + if (!dev) > + return NULL; > + > + dev->attrs_ht = g_hash_table_new_full(g_str_hash, g_str_equal, > + free, free); > + dev->props_ht = g_hash_table_new_full(g_str_hash, g_str_equal, > + free, free); > + > + if (dev->attrs_ht && dev->props_ht) > + return dev; > + > + return NULL; > +} > + > +static void igt_device_add_prop(struct igt_device *dev, > + const char *key, const char *value) > +{ > + if (!key || !value) > + return; > + > + g_hash_table_insert(dev->props_ht, strdup(key), strdup(value)); > +} > + > +static void igt_device_add_attr(struct igt_device *dev, > + const char *key, const char *value) > +{ > + const char *v = value; > + > + if (!key) > + return; > + > + /* It's possible we have symlink at key filename, but udev > + * library resolves only few of them */ > + if (!v) { > + struct stat st; > + char path[PATH_MAX]; > + char linkto[PATH_MAX]; > + int len; > + > + snprintf(path, sizeof(path), "%s/%s", dev->syspath, key); > + if (lstat(path, &st) != 0) > + return; > + > + len = readlink(path, linkto, sizeof(linkto)); > + if (len <= 0 || len == (ssize_t) sizeof(linkto)) > + return; > + linkto[len] = '\0'; > + v = strrchr(linkto, '/'); > + if (v == NULL) > + return; > + v++; > + } > + > + g_hash_table_insert(dev->attrs_ht, strdup(key), strdup(v)); > +} > + > +/* Iterate over udev properties list and rewrite it to igt_device properties > + * hash table for instant access. > + */ > +static void get_props(struct udev_device *dev, struct igt_device *idev) > +{ > + struct udev_list_entry *entry; > + > + entry = udev_device_get_properties_list_entry(dev); > + while (entry) { > + const char *name = udev_list_entry_get_name(entry); > + const char *value = udev_list_entry_get_value(entry); > + igt_device_add_prop(idev, name, value); > + entry = udev_list_entry_get_next(entry); > + DBG("prop: %s, val: %s\n", name, value); > + } > +} > + > +/* Same as get_props(), but rewrites sysattrs. Resolves symbolic links > + * not handled by udev get_sysattr_value(). > + * Function skips sysattrs from blacklist ht (acquiring some values can take > + * seconds). > + */ > +static void get_attrs(struct udev_device *dev, struct igt_device *idev) > +{ > + struct udev_list_entry *entry; > + > + entry = udev_device_get_sysattr_list_entry(dev); > + while (entry) { > + const char *key = udev_list_entry_get_name(entry); > + const char *value; > + > + if (is_on_blacklist(key)) { > + entry = udev_list_entry_get_next(entry); > + continue; > + } > + > + value = udev_device_get_sysattr_value(dev, key); > + igt_device_add_attr(idev, key, value); > + entry = udev_list_entry_get_next(entry); > + DBG("attr: %s, val: %s\n", key, value); > + } > +} > + > +#define get_prop(dev, prop) (char *) g_hash_table_lookup(dev->props_ht, prop) > +#define get_attr(dev, attr) (char *) g_hash_table_lookup(dev->attrs_ht, attr) I would add parentheses to these 2 macros ^^ - it will be more visible where '(char *)' belongs. It is obvious but anyway :) > +#define get_prop_subsystem(dev) get_prop(dev, "SUBSYSTEM") > +#define is_drm_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "drm")) > +#define is_pci_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "pci")) > +#define is_platform_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "platform")) > + > +/* Gets PCI_ID property, splits to xxxx:yyyy and stores > + * xxxx to dev->vendor and yyyy to dev->device for > + * faster access. > + */ > +static void set_vendor_device(struct igt_device *dev) > +{ > + const char *pci_id = get_prop(dev, "PCI_ID"); > + if (!pci_id || strlen(pci_id) != 9) > + return; > + dev->vendor = strndup(pci_id, 4); > + dev->device = strndup(pci_id + 5, 4); > +} > + > +/* Allocate arrays for keeping scanned devices */ Style comment: previous doc headers started with Gets/Sets (as 'Function' gets). This one starts without 's'. Let's unify that :) > +static bool prepare_scan(void) > +{ > + if (!igt_devs.devs) > + igt_devs.devs = g_ptr_array_sized_new(4); > + if (!igt_devs.view) > + igt_devs.view = g_ptr_array_sized_new(4); > + > + if (!igt_devs.devs || !igt_devs.view) > + return false; > + > + return true; > +} > + > +/* Create new igt_device from udev device. > + Fills structure with most usable udev device variables, properties > + and sysattrs. > +*/ > +static struct igt_device *igt_device_new_from_udev(struct udev_device *dev) > +{ > + struct igt_device *idev = igt_device_new(); > + igt_assert(idev); > + > + idev->syspath = g_strdup(udev_device_get_syspath(dev)); > + idev->subsystem = g_strdup(udev_device_get_subsystem(dev)); > + idev->devnode = g_strdup(udev_device_get_devnode(dev)); > + > + if (idev->devnode && strstr(idev->devnode, "/dev/dri/card")) > + idev->drm_card = g_strdup(idev->devnode); > + else if (idev->devnode && strstr(idev->devnode, "/dev/dri/render")) > + idev->drm_render = g_strdup(idev->devnode); > + > + get_props(dev, idev); > + get_attrs(dev, idev); > + > + return idev; > +} > + > +/* Iterate over all igt_devices array and find one matched to > + * subsystem and syspath. > +*/ > +static struct igt_device *igt_device_find(const char *subsystem, > + const char *syspath) > +{ > + struct igt_device *dev; > + > + for (int i = 0; i < igt_devs.devs->len; i++) { > + dev = g_ptr_array_index(igt_devs.devs, i); > + if (!strcmp(dev->subsystem, subsystem) && > + !strcmp(dev->syspath, syspath)) > + return dev; > + } > + return NULL; > +} > + > +/* For each drm igt_device add or update its parent igt_device to the array. > + As card/render drm devices mostly have same parent (vkms is an exception) > + link to it and update corresponding drm_card / drm_render fields. > +*/ > +static void update_or_add_parent(struct udev_device *dev, > + struct igt_device *idev) > +{ > + struct udev_device *parent_dev; > + struct igt_device *parent_idev; > + const char *subsystem, *syspath, *devname; > + > + parent_dev = udev_device_get_parent(dev); > + igt_assert(parent_dev); > + > + subsystem = udev_device_get_subsystem(parent_dev); > + syspath = udev_device_get_syspath(parent_dev); > + > + parent_idev = igt_device_find(subsystem, syspath); > + if (!parent_idev) { > + parent_idev = igt_device_new_from_udev(parent_dev); > + if (is_pci_subsystem(parent_idev)) { > + set_vendor_device(parent_idev); > + parent_idev->vs = get_vendor_spec(parent_idev->vendor); > + } > + g_ptr_array_add(igt_devs.devs, parent_idev); > + } > + devname = udev_device_get_devnode(dev); > + if (strstr(devname, "/dev/dri/card")) > + parent_idev->drm_card = g_strdup(devname); > + else if (strstr(devname, "/dev/dri/render")) > + parent_idev->drm_render= g_strdup(devname); > + > + idev->parent = parent_idev; > +} > + > +static gint devs_compare(gconstpointer a, gconstpointer b) > +{ > + struct igt_device *dev1, *dev2; > + int ret; > + > + dev1 = *(struct igt_device **) a; > + dev2 = *(struct igt_device **) b; > + ret = strcmp(dev1->subsystem, dev2->subsystem); > + if (ret) > + return ret; > + > + return strcmp(dev1->syspath, dev2->syspath); > +} > + > +/* Core scanning function. > + * > + * All scanned devices are kept inside igt_devs.devs pointer array. > + * Each added device is igt_device structure, which contrary to udev device > + * has properties / sysattrs stored inside hash table instead of list. > + * > + * Function iterates over devices on 'drm' subsystem. For each drm device > + * its parent is taken (bus device) and stored inside same array. > + * Function sorts all found devices to keep same order of bus devices > + * for providing predictable search. > + */ > +static void scan_drm_devices(void) > +{ > + struct udev *udev; > + struct udev_enumerate *enumerate; > + struct udev_list_entry *devices, *dev_list_entry; > + int ret; > + > + udev = udev_new(); > + igt_assert(udev); > + > + enumerate = udev_enumerate_new(udev); > + igt_assert(enumerate); > + > + DBG("Scanning drm subsystem\n"); > + ret = udev_enumerate_add_match_subsystem(enumerate, "drm"); > + igt_assert(!ret); > + > + udev_enumerate_add_match_property(enumerate, "DEVNAME", "/dev/dri/*"); > + igt_assert(!ret); > + > + ret = udev_enumerate_scan_devices(enumerate); > + igt_assert(!ret); > + > + devices = udev_enumerate_get_list_entry(enumerate); > + if (!devices) > + return; > + > + udev_list_entry_foreach(dev_list_entry, devices) { > + const char *path; > + struct udev_device *dev; > + struct igt_device *idev; > + > + path = udev_list_entry_get_name(dev_list_entry); > + dev = udev_device_new_from_syspath(udev, path); > + idev = igt_device_new_from_udev(dev); > + update_or_add_parent(dev, idev); > + g_ptr_array_add(igt_devs.devs, idev); > + > + udev_device_unref(dev); > + } > + udev_enumerate_unref(enumerate); > + udev_unref(udev); > + > + g_ptr_array_sort(igt_devs.devs, devs_compare); > + for (int i = 0; i < igt_devs.devs->len; i++) { > + g_ptr_array_add(igt_devs.view, > + g_ptr_array_index(igt_devs.devs, i)); > + } > +} > + > +struct name_value filter_definition_list[]; > +static void populate_gpu_data(void) > +{ > + fill_ht(&gpu_vendor_ht, &gpu_vendor_list[0]); > + fill_ht(&filter_definition_ht, &filter_definition_list[0]); > +} > + > +static void igt_device_free(struct igt_device *dev) > +{ > + free(dev->devnode); > + free(dev->subsystem); > + free(dev->syspath); > + free(dev->drm_card); > + free(dev->drm_render); > + free(dev->vendor); > + free(dev->device); > + g_hash_table_destroy(dev->attrs_ht); > + g_hash_table_destroy(dev->props_ht); > +} > + > +/** > + * igt_devices_scan > + * @force: enforce scanning devices > + * > + * Function scans udev in search of gpu devices. For first run it can be > + * called with @force = false. If something changes during the the test > + * or test does some module loading (new drm devices occurs during execution) > + * function must be called again with @force = true to refresh device array. > + */ > +void igt_devices_scan(bool force) > +{ > + if (force && igt_devs.devs_scanned) { > + for (int i = 0; i < igt_devs.devs->len; i++) { > + struct igt_device *dev = > + g_ptr_array_index(igt_devs.devs, i); > + igt_device_free(dev); > + free(dev); > + } > + igt_devs.devs_scanned = false; > + g_ptr_array_free(igt_devs.view, true); > + g_ptr_array_free(igt_devs.devs, true); > + igt_devs.view = NULL; > + igt_devs.devs = NULL; > + } > + > + if (igt_devs.devs_scanned) > + return; > + > + populate_blacklist_keys(); //keys from sysattr we skip > + populate_gpu_data(); > + > + prepare_scan(); > + scan_drm_devices(); > + > + igt_devs.devs_scanned = true; > +} > + > +#define pr_simple(k, v) printf(" %-16s: %s\n", k, v) > +#define pr_simple2(k, v1, v2) printf(" %-16s: %s:%s\n", k, v1, v2) > +#define pr_simple_prop(dev, key) pr_simple(key, get_prop(dev, key)) > +#define pr_simple_attr(dev, key) pr_simple(key, get_attr(dev, key)) > + > +static void igt_devs_print_simple(GPtrArray *view) > +{ > + struct igt_device *dev; > + int i; > + > + if (!view) > + return; > + > + for (i = 0; i < view->len; i++) { > + dev = g_ptr_array_index(view, i); > + printf("%s:%s\n", dev->subsystem, dev->syspath); > + if (dev->drm_card) > + pr_simple("drm card", dev->drm_card); > + if (dev->drm_render) > + pr_simple("drm render", dev->drm_render); > + if (is_drm_subsystem(dev)) { > + pr_simple2("parent", dev->parent->subsystem, > + dev->parent->syspath); > + } else { > + if (is_pci_subsystem(dev)) { > + pr_simple("vendor", dev->vendor); > + pr_simple("device", dev->device); > + } > + } > + printf("\n"); > + } > +} Playing around with usage of this code shows that we print 'nothing' when nothing is found, what is good :) but we also do not print any information that device do not exist, e.g. warining. > + > +#define pr_detail(k, v) printf("%-32s: %s\n", k, v) > + > +static void print_ht(GHashTable *ht) > +{ > + GList *keys = g_hash_table_get_keys(ht); > + keys = g_list_sort(keys, (GCompareFunc) strcmp); > + while (keys) { > + char *k = (char *) keys->data; > + char *v = g_hash_table_lookup(ht, k); > + pr_detail(k, v); > + keys = g_list_next(keys); > + } > + g_list_free(keys); > +} > + > +static void igt_devs_print_detail(GPtrArray *view) > +{ > + struct igt_device *dev; > + int i; > + > + if (!view) > + return; > + > + for (i = 0; i < view->len; i++) { > + dev = g_ptr_array_index(view, i); > + printf("========== %s:%s ==========\n", > + dev->subsystem, dev->syspath); > + if (!is_drm_subsystem(dev)) { > + pr_detail("card device", dev->drm_card); > + pr_detail("render device", dev->drm_render); > + } > + > + printf("\n[properties]\n"); > + print_ht(dev->props_ht); > + printf("\n[attributes]\n"); > + print_ht(dev->attrs_ht); > + printf("\n"); > + } > +} > + > +static struct print_func { > + void (*prn)(GPtrArray *); Do we want to have a name here ^? > +} print_functions[] = { > + [IGT_PRINT_SIMPLE] = { .prn = igt_devs_print_simple }, > + [IGT_PRINT_DETAIL] = { .prn = igt_devs_print_detail }, > +}; > + > +/** > + * igt_devices_print > + * @printtype: IGT_PRINT_SIMPLE or IGT_PRINT_DETAIL > + * > + * Function is used by 'lsgpu' tool to print device array in simple or detailed > + * form. This function is added here instead in 'lsgpu' to avoid exposing > + * internal implementation data structures. > + */ > +void igt_devices_print(enum igt_devices_print_type printtype) > +{ > + print_functions[printtype].prn(igt_devs.view); > +} Should it be possible to do something like: 'lsgpu -D pci:/sys/devices/pci0000:00/0000:00:02.0' to show only this particular device? Currently anything added after '-D' parameter is ignored. Writing this comment I realized that parameters '-d' and '-D' can be easily mistaken. Maybe we should rethink the naming? > + > +/** > + * igt_devices_print_vendors > + * > + * Print pci id -> vendor mappings. Vendor names printed by this function > + * can be used for filters like pci which allows passing vendor - like > + * vendor id (8086) or as a string (Intel). > + */ > +void igt_devices_print_vendors(void) > +{ > + struct name_value *list = &gpu_vendor_list[0]; > + printf("Recognized vendors:\n"); > + > + printf("%-8s %-16s\n", "PCI ID", "vendor"); > + while (list->name) { > + struct vendor_spec *vs = (struct vendor_spec *) list->value; > + if (vs) > + printf("%-8s %-16s\n", list->name, vs->vendor); > + list++; > + } > +} > + > +/* ------------------------------------------------------------------------- */ > +#define FILTER_SPEC_NAME_LEN 32 > +#define FILTER_SPEC_DATA_LEN 256 > +struct filter_spec { > + char name[FILTER_SPEC_NAME_LEN]; > + char data[FILTER_SPEC_DATA_LEN]; > +}; > + > +struct filter_func { > + GPtrArray *(*filter_function)(struct filter_spec *fspec, > + struct filter_func *ffunc); > + const char *help; > + const char *detail; > + > + const char *property; > + const char *value; > +}; > + > +struct filter_data { > + char *vendor; > + char *device; > + char *card; > + char *drm; > +}; > + > +static GHashTable *split_filter_data(const char *data) > +{ > + GHashTable *ht = g_hash_table_new_full(g_str_hash, g_str_equal, > + free, free); > + gchar **s; > + gchar **strv = g_strsplit(data, ",", -1); > + > + s = strv; > + while (*s) { > + char *k, *v; > + v = strchr(*s, '='); > + if (!v) { > + s++; > + continue; > + } > + k = strndup(*s, v - (*s)); > + v = strdup(v + 1); > + g_hash_table_insert(ht, k, v); Just a question here ^ - would checking that k and v be too much here? -> are we sure that value should be valid or g_hash_table_insert is dealing with that? > + s++; > + } > + g_strfreev(strv); > + > + return ht; > +} > + > +static bool get_filter_spec(const char *filter, struct filter_spec *spec) > +{ > + if (!filter) > + return false; > + > + if (sscanf(filter, "%31[^:]:%255s", spec->name, spec->data) >= 1) > + return true; > + > + return false; > +} > + > +static bool is_vendor_matched(struct igt_device *dev, const char *vendor) > +{ > + const char *vendor_id; > + > + if (!dev->vendor || !vendor) > + return false; > + > + /* First we compare vendor id, like 8086 */ > + if (!strcasecmp(dev->vendor, vendor)) > + return true; > + > + /* Likely we have vendor string instead of id */ > + vendor_id = get_pci_vendor_id_by_name(vendor); > + if (!vendor_id) > + return false; > + > + return !strcasecmp(dev->vendor, vendor_id); > +} > + > +/* Filter which matches subsystem:/sys/... path. > + * Used as first filter in chain. > + */ > +static GPtrArray *filter_sys(struct filter_spec *fspec, > + struct filter_func *ffunc) > +{ > + (void) ffunc; > + > + DBG("filter sys\n"); > + if (!strlen(fspec->data)) > + return igt_devs.view; > + > + for (int i = 0; i < igt_devs.devs->len; i++) { > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > + > + if (strequal(dev->subsystem, fspec->name) && > + strequal(dev->syspath, fspec->data)) { > + g_ptr_array_add(igt_devs.view, dev); > + break; > + } > + } > + > + DBG("Filter sys view size: %d\n", igt_devs.view->len); > + > + return igt_devs.view; > +} > + > +/* Find drm device using direct path to /dev/dri/. > + * It extends filter_sys to allow using drm:/dev/dri/cardX and > + * drm:/dev/dri/renderDX filter syntax. > + */ > +static GPtrArray *filter_drm(struct filter_spec *fspec, > + struct filter_func *ffunc) > +{ > + (void) ffunc; > + > + DBG("filter drm\n"); > + if (!strlen(fspec->data)) > + return igt_devs.view; > + > + for (int i = 0; i < igt_devs.devs->len; i++) { > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > + if (!is_drm_subsystem(dev)) > + continue; > + > + if (strequal(dev->syspath, fspec->data) || > + strequal(dev->drm_card, fspec->data) || > + strequal(dev->drm_render, fspec->data)) { > + g_ptr_array_add(igt_devs.view, dev); > + break; > + } > + } > + > + DBG("Filter drm view size: %d\n", igt_devs.view->len); > + > + return igt_devs.view; > +} > + > +/* Find appropriate pci device matching vendor/device/card filter arguments > +*/ > +static GPtrArray *filter_pci(struct filter_spec *fspec, > + struct filter_func *ffunc) > +{ > + GHashTable *ht; > + struct filter_data fdata; > + int card = -1; > + (void) ffunc; > + > + DBG("filter pci\n"); > + > + ht = split_filter_data(fspec->data); > + fdata.vendor = g_hash_table_lookup(ht, "vendor"); > + fdata.device = g_hash_table_lookup(ht, "device"); > + fdata.card = g_hash_table_lookup(ht, "card"); > + > + if (fdata.card) { > + sscanf(fdata.card, "%d", &card); Probably we need to check sscanf result here? > + if (card < 0) { > + g_hash_table_destroy(ht); > + return igt_devs.view; > + } > + } else { > + card = 0; > + } > + > + for (int i = 0; i < igt_devs.devs->len; i++) { > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > + > + if (!is_pci_subsystem(dev)) > + continue; > + > + /* Skip if 'vendor' doesn't match (hex or name) */ > + if (fdata.vendor && !is_vendor_matched(dev, fdata.vendor)) > + continue; > + > + /* Skip if 'device' doesn't match */ > + if (fdata.device && strcasecmp(fdata.device, dev->device)) > + continue; > + > + /* We get n-th card */ > + if (!card) { > + g_ptr_array_add(igt_devs.view, dev); > + break; > + } > + card--; > + } > + > + DBG("Filter pci view size: %d\n", igt_devs.view->len); > + > + g_hash_table_destroy(ht); > + > + return igt_devs.view; > +} > + > +/* Helper for finding first device matching property:value. > +*/ > +static GPtrArray *filter_property(struct filter_spec *fspec, > + struct filter_func *ffunc) > +{ > + GHashTable *ht; > + struct filter_data fdata; > + int card = -1; > + const char *property = ffunc->property; > + const char *value = ffunc->value; > + > + if (!property || !value) > + return igt_devs.view; > + > + DBG("filter property/value [%s/%s]\n", property, value); > + > + ht = split_filter_data(fspec->data); > + fdata.card = g_hash_table_lookup(ht, "card"); > + > + if (fdata.card) { > + sscanf(fdata.card, "%d", &card); > + if (card < 0) { > + g_hash_table_destroy(ht); > + return igt_devs.view; > + } > + } else { > + card = 0; > + } > + > + for (int i = 0; i < igt_devs.devs->len; i++) { > + const char *prop_value; > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > + > + prop_value = get_prop(dev, property); > + if (prop_value && !strcmp(prop_value, value)) { > + if (!card) { > + g_ptr_array_add(igt_devs.view, dev); > + break; > + } > + card--; > + } > + } > + > + DBG("Filter view size: %d\n", igt_devs.view->len); > + > + g_hash_table_destroy(ht); > + > + return igt_devs.view; > +} > + > +static GPtrArray *filter_vgem(struct filter_spec *fspec, > + struct filter_func *ffunc) > +{ > + GPtrArray *view = filter_property(fspec, ffunc); > + if (view) { > + struct igt_device *dev = g_ptr_array_index(view, 0); > + dev->vs = &v_vgem; > + } > + return view; > +} > + > +static GPtrArray *filter_vkms(struct filter_spec *fspec, > + struct filter_func *ffunc) > +{ > + return filter_property(fspec, ffunc); > +} > + > +static GPtrArray *filter_vc4(struct filter_spec *fspec, > + struct filter_func *ffunc) > +{ > + GPtrArray *view = filter_property(fspec, ffunc); > + if (view) { > + struct igt_device *dev = g_ptr_array_index(view, 0); > + dev->vs = &v_vc4; > + } > + return filter_property(fspec, ffunc); > +} > + > +static struct filter_func f_drm = { .filter_function = filter_drm, > + .help = "drm:/dev/dri/* path", > + .detail = "find drm device by /dev/dri/* node\n", > + }; > + > +static struct filter_func f_pci = { .filter_function = filter_pci, > + .help = "pci:[vendor=%04x/name][,device=%04x][,card=%d]", > + .detail = "vendor is hex number or vendor name\n", > + }; > + > +static struct filter_func f_vgem = { .filter_function = filter_vgem, > + .help = "vgem:[card=%d]", > + .detail = "card is n-th vgem card number\n", > + .property = "MODALIAS", > + .value = "platform:vgem", > + }; > + > +static struct filter_func f_vkms = { .filter_function = filter_vkms, > + .help = "vkms:[card=%d]", > + .detail = "card is n-th vkms card number\n", > + .property = "MODALIAS", > + .value = "platform:vkms", > + }; > + > +static struct filter_func f_vc4 = { .filter_function = filter_vc4, > + .help = "vc4:[card=%d]", > + .detail = "card is n-th vc4 card number\n", > + .property = "OF_COMPATIBLE_0", > + .value = "brcm,bcm2835-vc4", > + }; > + > +struct name_value filter_definition_list[] = { > + { "drm", (gpointer) &f_drm }, > + { "pci", (gpointer) &f_pci }, > + { "vgem", (gpointer) &f_vgem }, > + { "vkms", (gpointer) &f_vkms }, > + { "vc4", (gpointer) &f_vc4 }, > + { NULL, }, > +}; > + > +/** > + * @igt_device_print_filter_types > + * > + * Print all filters syntax for device selection. > + */ > +void igt_device_print_filter_types(void) > +{ > + struct name_value *list = &filter_definition_list[0]; > + printf("Filter types:\n---\n"); > + printf("%-12s %s\n---\n", "filter", "syntax"); > + > + printf("%-12s %s\n", "", "direct subsystem:/sys/... path selection\n"); > + printf("%-12s %s\n", "", > + "ex: drm:/sys/devices/pci0000:00/0000:00:02.0/drm/card0"); > + printf("%-12s %s\n", "", > + " pci:/sys/devices/pci0000:00/0000:00:02.0"); > + printf("%-12s %s\n", "", > + " platform:/sys/devices/platform/vgem\n"); > + > + while (list->name) { > + struct filter_func *v = (struct filter_func *) list->value; > + printf("%-12s %s\n", list->name, v->help); > + printf("%-12s %s\n", "", v->detail); > + list++; > + } > +} > + > +static GPtrArray *device_filters = NULL; I wonder why chechpatch returns error here ^. It is not illegal, maybe it shouts becasue it is done by default? > + > +#define DEVICE_FILTER_CHECK_ALLOC() \ > + do { \ > + if (!device_filters) \ > + device_filters = g_ptr_array_new_full(2, free); \ > + igt_assert(device_filters); \ > + } while(0) > + > +/** > + * igt_device_filter_count > + * > + * Returns number of filters collected in the filter array. > + */ > +int igt_device_filter_count(void) > +{ > + DEVICE_FILTER_CHECK_ALLOC(); > + > + return device_filters->len; > +} > + > +/** > + * igt_device_filter_add > + * @filter: filter to be stored in filter array > + * > + * Function allows passing single or more filters within one string. This is > + * for CI when it can extract filter from environment variable (and it must > + * be single string). So if @filter contains semicolon ';' it treats > + * each part as separate filter and adds to the filter array. > + * > + * Returns number of filters added to filter array. Can be greater than > + * 1 if @filter contains more than one filter separated by semicolon. > + */ > +int igt_device_filter_add(const char *filter) > +{ > + gchar **strv, **s; > + int c = 0; > + > + DEVICE_FILTER_CHECK_ALLOC(); > + > + strv = g_strsplit(filter, ";", -1); > + > + s = strv; > + while (*s) { > + g_ptr_array_add(device_filters, strdup(*s)); > + s++; > + } > + g_strfreev(strv); > + > + return c; > +} > + > +/** > + * igt_device_filter_get > + * @num: Number of filter from filter array > + * > + * Returns filter string or NULL if @num is out of range of filter array. > +*/ > +const char *igt_device_filter_get(int num) > +{ > + DEVICE_FILTER_CHECK_ALLOC(); > + > + if (num < 0 || num >= device_filters->len) > + return NULL; > + > + return g_ptr_array_index(device_filters, num); > +} > + > +static bool igt_device_filter_apply(const char *filter) > +{ > + struct filter_spec fspec; > + bool ret; > + > + if (!filter) > + return false; > + > + memset(&fspec, 0, sizeof(fspec)); > + ret = get_filter_spec(filter, &fspec); > + if (!ret) { > + igt_warn("Can't split filter [%s]\n", filter); > + return false; > + } > + > + /* Clean view */ > + g_ptr_array_remove_range(igt_devs.view, 0, igt_devs.view->len); > + > + /* If fspec.data contains "/sys" use direct path instead > + * contextual filter */ > + if (!strncmp(fspec.data, "/sys", 4)) > + filter_sys(&fspec, NULL); > + else { > + struct filter_func *ffunc; > + ffunc = g_hash_table_lookup(filter_definition_ht, fspec.name); > + if (!ffunc) { > + igt_warn("No filter with name [%s]\n", fspec.name); > + return false; > + } > + ffunc->filter_function(&fspec, ffunc); > + } > + Should we free(fspec) at the end? Or it is done somewhere else? > + return true; > +} > + > +#define safe_strncpy(dst, src, size) \ > + if ((dst) && (src)) strncpy((dst), (src), (size)) > + > +/** > + * igt_device_card_match > + * @filter: filter string > + * @card: pointer to igt_device_card struct > + * > + * Function applies filter to match device from device array. > + * > + * Returns: > + * false - no card pointer was passed or card wasn't matched, > + * true - card matched and returned. > + */ > +bool igt_device_card_match(const char *filter, struct igt_device_card *card) > +{ > + struct igt_device *dev = NULL; > + > + if (!card) > + return false; > + memset(card, 0, sizeof(*card)); > + > + igt_devices_scan(false); > + > + if (igt_device_filter_apply(filter) == false) > + return false; > + > + if (!igt_devs.view->len) > + return false; > + > + /* We take first one if more than one card matches filter */ > + dev = g_ptr_array_index(igt_devs.view, 0); > + card->chipset = DRIVER_ANY; > + if (dev->vs) > + card->chipset = dev->vs->chipset; > + > + safe_strncpy(card->subsystem, dev->subsystem, NAME_MAX); > + safe_strncpy(card->card, dev->drm_card, NAME_MAX); > + safe_strncpy(card->render, dev->drm_render, NAME_MAX); This macro is returing true/false, do we check the results? It looks like magic to me :) > + > + return true; > +} > diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h > new file mode 100644 > index 00000000..62f7f39a > --- /dev/null > +++ b/lib/igt_device_scan.h > @@ -0,0 +1,69 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#ifndef __IGT_DEVICE_SCAN_H__ > +#define __IGT_DEVICE_SCAN_H__ > + > +#include <limits.h> > +#include <igt.h> > + > +enum igt_devices_print_type { > + IGT_PRINT_SIMPLE, > + IGT_PRINT_DETAIL, > +}; > + > +struct igt_device_card { > + int chipset; > + char subsystem[NAME_MAX]; > + char card[NAME_MAX]; > + char render[NAME_MAX]; > +}; > + > +/* Scan udev subsystems. Do it once unless force is true, what rescans > + * devices again */ > +void igt_devices_scan(bool force); > + > +void igt_devices_print(enum igt_devices_print_type printtype); > +void igt_devices_print_vendors(void); > +void igt_device_print_filter_types(void); > + > +/* > + * Handle device filter collection array. > + * IGT can store/retrieve filters passed by user using '--device' args. > +*/ This comment ^^ is a general one for what is below? If yes - it is not obvious :/ > + > +/* Return number of filters collected */ > +int igt_device_filter_count(void); > + > +/* Add filter(s) to the array. Returns number of filters added (>1 if > + * user passes more than one filter separatined by ';') */ > +int igt_device_filter_add(const char *filter); > + > +/* Get n-th filter stored in the array or NULL */ > +const char *igt_device_filter_get(int num); > + > +/* Use filter to match the device and fill card structure */ > +bool igt_device_card_match(const char *filter, struct igt_device_card *card); > + > +#endif /* __IGT_DEVICE_SCAN_H__ */ > diff --git a/lib/meson.build b/lib/meson.build > index 157624e7..826ebbe3 100644 > --- a/lib/meson.build > +++ b/lib/meson.build > @@ -10,6 +10,7 @@ lib_sources = [ > 'igt_color_encoding.c', > 'igt_debugfs.c', > 'igt_device.c', > + 'igt_device_scan.c', > 'igt_aux.c', > 'igt_gpu_power.c', > 'igt_gt.c', > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > index 50706f41..0e67b654 100644 > --- a/tools/Makefile.sources > +++ b/tools/Makefile.sources > @@ -33,6 +33,7 @@ tools_prog_lists = \ > intel_watermark \ > intel_gem_info \ > intel_gvtg_test \ > + lsgpu \ > $(NULL) > > dist_bin_SCRIPTS = intel_gpu_abrt > diff --git a/tools/lsgpu.c b/tools/lsgpu.c > new file mode 100644 > index 00000000..e723f0b2 > --- /dev/null > +++ b/tools/lsgpu.c > @@ -0,0 +1,183 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + * > + */ > + > +#include "igt_device_scan.h" > +#include "igt.h" > +#include <sys/ioctl.h> > +#include <fcntl.h> > +#include <errno.h> > +#include <string.h> > +#include <signal.h> > + > +enum { > + OPT_PRINT_DETAIL = 'D', > + OPT_LIST_VENDORS = 'v', > + OPT_LIST_FILTERS = 'l', > + OPT_DEVICE = 'd', > + OPT_HELP = 'h' > +}; > + > +bool g_show_vendors; > +bool g_list_filters; > +bool g_device; > +bool g_help; > + > +static const char *usage_str = > + "usage: lsgpu [options]\n\n" I would add 1 sentence about what pretty cool stuff this tool can do :) > + "Options:\n" > + " -D, --print-details Print devices with details\n" As I wrote above '-D' and '-d' can be easily mistaken, nothing will blow up, but maybe we can use different letter here? It it if for printing then maybe '-P'/'-p'? > + " -v, --list-vendors List recognized vendors\n" > + " -l, --list-filter-types List registered device filters types\n" > + " -d, --device filter Device filter, can be given multiple times\n" > + " -h, --help Show this help message and exit\n"; > + > +static void test_device_open(struct igt_device_card *card) > +{ > + int fd; > + > + if (!card) > + return; > + > + fd = drm_open_card(card); > + if (fd >= 0) { > + printf("Device %s successfully opened\n", card->card); > + close(fd); > + } else { > + if (strlen(card->card)) > + printf("Cannot open card %s device\n", card->card); > + else > + printf("Cannot open card device, empty name\n"); > + } > + > + fd = drm_open_render(card); > + if (fd >= 0) { > + printf("Device %s successfully opened\n", card->render); > + close(fd); > + } else { > + if (strlen(card->render)) > + printf("Cannot open render %s device\n", card->render); > + else > + printf("Cannot open render device, empty name\n"); > + } We do not get any warning when non-existing device is used, is it ok? > +} > + > +static void print_card(struct igt_device_card *card) > +{ > + if (!card) > + return; > + > + printf("subsystem : %s\n", card->subsystem); > + printf("chipset : %x\n", card->chipset); > + printf("drm card : %s\n", card->card); > + printf("drm render : %s\n", card->render); > +} > + > +int main(int argc, char *argv[]) > +{ > + static struct option long_options[] = { > + {"print-detail", no_argument, NULL, OPT_PRINT_DETAIL}, > + {"list-vendors", no_argument, NULL, OPT_LIST_VENDORS}, > + {"list-filter-types", no_argument, NULL, OPT_LIST_FILTERS}, > + {"device", required_argument, NULL, OPT_DEVICE}, > + {"help", no_argument, NULL, OPT_HELP}, > + {0, 0, 0, 0} > + }; > + int c, index = 0; > + const char *env; > + enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE; > + > + while ((c = getopt_long(argc, argv, "Dvld:h", > + long_options, &index)) != -1) { > + switch(c) { > + > + case OPT_PRINT_DETAIL: > + printtype = IGT_PRINT_DETAIL; > + break; > + case OPT_LIST_VENDORS: > + g_show_vendors = true; > + break; > + case OPT_LIST_FILTERS: > + g_list_filters = true; > + break; > + case OPT_DEVICE: > + g_device = true; > + igt_device_filter_add(optarg); > + break; > + case OPT_HELP: > + g_help = true; > + break; > + } > + } > + > + if (g_help) { > + printf("%s\n", usage_str); > + exit(0); > + } > + > + env = getenv("IGT_DEVICE"); > + if (env) { > + igt_device_filter_add(env); > + g_device = true; > + } > + > + if (g_show_vendors) { > + igt_devices_print_vendors(); > + return 0; > + } > + > + if (g_list_filters) { > + igt_device_print_filter_types(); > + return 0; > + } > + > + igt_devices_scan(false); > + > + if (g_device) { > + int n = igt_device_filter_count(); > + printf("=== Device filter list ===\n"); > + for (int i = 0; i < n; i++) { > + printf("[%2d]: %s\n", i, > + igt_device_filter_get(i)); > + } > + printf("\n"); > + > + printf("=== Testing device open ===\n"); > + for (int i = 0; i < n; i++) { > + struct igt_device_card card; > + const char *filter = igt_device_filter_get(i); > + > + if (!igt_device_card_match(filter, &card)) > + continue; > + print_card(&card); > + test_device_open(&card); > + printf("---\n"); > + } > + > + return 0; > + } > + > + igt_devices_print(printtype); > + > + return 0; > +} > diff --git a/tools/meson.build b/tools/meson.build > index 6e72b263..9b3a2a69 100644 > --- a/tools/meson.build > +++ b/tools/meson.build > @@ -36,6 +36,7 @@ tools_progs = [ > 'intel_gem_info', > 'intel_gvtg_test', > 'dpcd_reg', > + 'lsgpu', > ] > tool_deps = igt_deps Zbyszek/Arek - is there any decision about IGT documentation? Should we follow some standards? I am asking because doc sections in this library are in various styles. I know that documenting stuff is boring, but futureproof :) Should we unify doc sections here? What I mean is: should we have something like: /** * function_name: * @param1: * @paramX: * * Description.... * Return value */ also here? Once sentence doc is beter then no doc, but... Zbyszku - huge work done! Kasia > > -- > 2.21.0 > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API 2019-08-21 11:04 ` Katarzyna Dec @ 2019-08-21 11:58 ` Katarzyna Dec 2019-08-22 7:23 ` Zbigniew Kempczyński 1 sibling, 0 replies; 13+ messages in thread From: Katarzyna Dec @ 2019-08-21 11:58 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev, Petri Latvala, Daniel Vetter On Wed, Aug 21, 2019 at 01:04:30PM +0200, Katarzyna Dec wrote: > > + * igt_device_card_match > > + * @filter: filter string > > + * @card: pointer to igt_device_card struct > > + * > > + * Function applies filter to match device from device array. > > + * > > + * Returns: > > + * false - no card pointer was passed or card wasn't matched, > > + * true - card matched and returned. > > + */ > > +bool igt_device_card_match(const char *filter, struct igt_device_card *card) > > +{ > > + struct igt_device *dev = NULL; > > + > > + if (!card) > > + return false; > > + memset(card, 0, sizeof(*card)); > > + > > + igt_devices_scan(false); > > + > > + if (igt_device_filter_apply(filter) == false) > > + return false; > > + > > + if (!igt_devs.view->len) > > + return false; > > + > > + /* We take first one if more than one card matches filter */ > > + dev = g_ptr_array_index(igt_devs.view, 0); > > + card->chipset = DRIVER_ANY; > > + if (dev->vs) > > + card->chipset = dev->vs->chipset; > > + > > + safe_strncpy(card->subsystem, dev->subsystem, NAME_MAX); > > + safe_strncpy(card->card, dev->drm_card, NAME_MAX); > > + safe_strncpy(card->render, dev->drm_render, NAME_MAX); > This macro is returing true/false, do we check the results? It looks like magic > to me :) Now it is not magic :) After a good lunch I realized what is going on here :) Kasia :) _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API 2019-08-21 11:04 ` Katarzyna Dec 2019-08-21 11:58 ` Katarzyna Dec @ 2019-08-22 7:23 ` Zbigniew Kempczyński 1 sibling, 0 replies; 13+ messages in thread From: Zbigniew Kempczyński @ 2019-08-22 7:23 UTC (permalink / raw) To: Katarzyna Dec; +Cc: igt-dev, Petri Latvala, Daniel Vetter On Wed, Aug 21, 2019 at 01:04:30PM +0200, Katarzyna Dec wrote: > On Mon, Aug 12, 2019 at 10:47:17AM +0200, Zbigniew Kempczyński wrote: > > Zbyszku, I saw that Arek has commented your changes, but I did not read it in > details, so some of my comments can be similar to his. I wanted to have clear > head starting this review :). > <cut> > > + * Device selection can be done using filters. Filters allows sophisticated > s/allows/allow :) Ok, queued to fix. <cut> > > + * So, when user passes filter which looks like follows: > > + * |[<!-- language="plain" --> > > + * - drm:/sys/devices/pci0000:00/0000:00:02.0/drm/card0 > > + * - pci:/sys/devices/pci0000:00/0000:00:02.0 > > + * - platform:/sys/devices/platform/vgem > > + * ]| > I tried this one locally and what seems to be missing is clear message that > device is not there. For now we have detailed information for existing device > and empty headlines for non-existing. I would add some warning, e.g. 'Device not > found'. You're right, I'm going to add appropriate info that device for filter wasn't found. Currently there's no info what can be confusing. <cut> > > + * > > + * |[<!-- language="plain" --> > > + * ./lsgpu -d 'pci:vendor=Intel' > > + * === Device filter list === > > + * [ 0]: pci:vendor=Intel > > + > > + * === Testing device open === > > + * subsystem : pci > > + * chipset : 1 > > + * drm card : /dev/dri/card0 > > + * drm render : /dev/dri/renderD128 > > + * Device /dev/dri/card0 successfully opened > > + * Device /dev/dri/renderD128 successfully opened > > + * ]| > > + * > > + * Apart of selecting device lsgpu try to open card and render nodes. > I did not see full picture yet, so maybe the answer it somewhere already, > anyway: > What can I do with this device opened and returned? Or this is just an > information that it exists and can be used by test/filter? This is low-level code for getting device to some helper code (like in drmtest.c). Lsgpu just shows how to use that and also it is checker for your filter syntax. > > +*/ > ^ End of comment section is not aligned, but this is really minor issue. > All similar issues can be found by using checkpatch.pl script :). > Looking furhter in this file there is a lot of code style warnings that are > worth fixing. I'll fix that. > > + > > +//#define DEBUG_DEVICE_SCAN > Why ^ this line is commented? I'll remove that. <cut> > > +#define get_prop(dev, prop) (char *) g_hash_table_lookup(dev->props_ht, prop) > > +#define get_attr(dev, attr) (char *) g_hash_table_lookup(dev->attrs_ht, attr) > I would add parentheses to these 2 macros ^^ - it will be more visible where > '(char *)' belongs. It is obvious but anyway :) In this case it is not necessary. > > +#define get_prop_subsystem(dev) get_prop(dev, "SUBSYSTEM") > > +#define is_drm_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "drm")) > > +#define is_pci_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "pci")) > > +#define is_platform_subsystem(dev) (!strcmp(get_prop_subsystem(dev), "platform")) > > + > > +/* Gets PCI_ID property, splits to xxxx:yyyy and stores > > + * xxxx to dev->vendor and yyyy to dev->device for > > + * faster access. > > + */ > > +static void set_vendor_device(struct igt_device *dev) > > +{ > > + const char *pci_id = get_prop(dev, "PCI_ID"); > > + if (!pci_id || strlen(pci_id) != 9) > > + return; > > + dev->vendor = strndup(pci_id, 4); > > + dev->device = strndup(pci_id + 5, 4); > > +} > > + > > +/* Allocate arrays for keeping scanned devices */ > Style comment: previous doc headers started with Gets/Sets (as 'Function' gets). > This one starts without 's'. Let's unify that :) > > > +static bool prepare_scan(void) > > +{ > > + if (!igt_devs.devs) > > + igt_devs.devs = g_ptr_array_sized_new(4); > > + if (!igt_devs.view) > > + igt_devs.view = g_ptr_array_sized_new(4); > > + > > + if (!igt_devs.devs || !igt_devs.view) > > + return false; > > + > > + return true; > > +} > > + > > +/* Create new igt_device from udev device. > > + Fills structure with most usable udev device variables, properties > > + and sysattrs. > > +*/ > > +static struct igt_device *igt_device_new_from_udev(struct udev_device *dev) > > +{ > > + struct igt_device *idev = igt_device_new(); > > + igt_assert(idev); > > + > > + idev->syspath = g_strdup(udev_device_get_syspath(dev)); > > + idev->subsystem = g_strdup(udev_device_get_subsystem(dev)); > > + idev->devnode = g_strdup(udev_device_get_devnode(dev)); > > + > > + if (idev->devnode && strstr(idev->devnode, "/dev/dri/card")) > > + idev->drm_card = g_strdup(idev->devnode); > > + else if (idev->devnode && strstr(idev->devnode, "/dev/dri/render")) > > + idev->drm_render = g_strdup(idev->devnode); > > + > > + get_props(dev, idev); > > + get_attrs(dev, idev); > > + > > + return idev; > > +} > > + > > +/* Iterate over all igt_devices array and find one matched to > > + * subsystem and syspath. > > +*/ > > +static struct igt_device *igt_device_find(const char *subsystem, > > + const char *syspath) > > +{ > > + struct igt_device *dev; > > + > > + for (int i = 0; i < igt_devs.devs->len; i++) { > > + dev = g_ptr_array_index(igt_devs.devs, i); > > + if (!strcmp(dev->subsystem, subsystem) && > > + !strcmp(dev->syspath, syspath)) > > + return dev; > > + } > > + return NULL; > > +} > > + > > +/* For each drm igt_device add or update its parent igt_device to the array. > > + As card/render drm devices mostly have same parent (vkms is an exception) > > + link to it and update corresponding drm_card / drm_render fields. > > +*/ > > +static void update_or_add_parent(struct udev_device *dev, > > + struct igt_device *idev) > > +{ > > + struct udev_device *parent_dev; > > + struct igt_device *parent_idev; > > + const char *subsystem, *syspath, *devname; > > + > > + parent_dev = udev_device_get_parent(dev); > > + igt_assert(parent_dev); > > + > > + subsystem = udev_device_get_subsystem(parent_dev); > > + syspath = udev_device_get_syspath(parent_dev); > > + > > + parent_idev = igt_device_find(subsystem, syspath); > > + if (!parent_idev) { > > + parent_idev = igt_device_new_from_udev(parent_dev); > > + if (is_pci_subsystem(parent_idev)) { > > + set_vendor_device(parent_idev); > > + parent_idev->vs = get_vendor_spec(parent_idev->vendor); > > + } > > + g_ptr_array_add(igt_devs.devs, parent_idev); > > + } > > + devname = udev_device_get_devnode(dev); > > + if (strstr(devname, "/dev/dri/card")) > > + parent_idev->drm_card = g_strdup(devname); > > + else if (strstr(devname, "/dev/dri/render")) > > + parent_idev->drm_render= g_strdup(devname); > > + > > + idev->parent = parent_idev; > > +} > > + > > +static gint devs_compare(gconstpointer a, gconstpointer b) > > +{ > > + struct igt_device *dev1, *dev2; > > + int ret; > > + > > + dev1 = *(struct igt_device **) a; > > + dev2 = *(struct igt_device **) b; > > + ret = strcmp(dev1->subsystem, dev2->subsystem); > > + if (ret) > > + return ret; > > + > > + return strcmp(dev1->syspath, dev2->syspath); > > +} > > + > > +/* Core scanning function. > > + * > > + * All scanned devices are kept inside igt_devs.devs pointer array. > > + * Each added device is igt_device structure, which contrary to udev device > > + * has properties / sysattrs stored inside hash table instead of list. > > + * > > + * Function iterates over devices on 'drm' subsystem. For each drm device > > + * its parent is taken (bus device) and stored inside same array. > > + * Function sorts all found devices to keep same order of bus devices > > + * for providing predictable search. > > + */ > > +static void scan_drm_devices(void) > > +{ > > + struct udev *udev; > > + struct udev_enumerate *enumerate; > > + struct udev_list_entry *devices, *dev_list_entry; > > + int ret; > > + > > + udev = udev_new(); > > + igt_assert(udev); > > + > > + enumerate = udev_enumerate_new(udev); > > + igt_assert(enumerate); > > + > > + DBG("Scanning drm subsystem\n"); > > + ret = udev_enumerate_add_match_subsystem(enumerate, "drm"); > > + igt_assert(!ret); > > + > > + udev_enumerate_add_match_property(enumerate, "DEVNAME", "/dev/dri/*"); > > + igt_assert(!ret); > > + > > + ret = udev_enumerate_scan_devices(enumerate); > > + igt_assert(!ret); > > + > > + devices = udev_enumerate_get_list_entry(enumerate); > > + if (!devices) > > + return; > > + > > + udev_list_entry_foreach(dev_list_entry, devices) { > > + const char *path; > > + struct udev_device *dev; > > + struct igt_device *idev; > > + > > + path = udev_list_entry_get_name(dev_list_entry); > > + dev = udev_device_new_from_syspath(udev, path); > > + idev = igt_device_new_from_udev(dev); > > + update_or_add_parent(dev, idev); > > + g_ptr_array_add(igt_devs.devs, idev); > > + > > + udev_device_unref(dev); > > + } > > + udev_enumerate_unref(enumerate); > > + udev_unref(udev); > > + > > + g_ptr_array_sort(igt_devs.devs, devs_compare); > > + for (int i = 0; i < igt_devs.devs->len; i++) { > > + g_ptr_array_add(igt_devs.view, > > + g_ptr_array_index(igt_devs.devs, i)); > > + } > > +} > > + > > +struct name_value filter_definition_list[]; > > +static void populate_gpu_data(void) > > +{ > > + fill_ht(&gpu_vendor_ht, &gpu_vendor_list[0]); > > + fill_ht(&filter_definition_ht, &filter_definition_list[0]); > > +} > > + > > +static void igt_device_free(struct igt_device *dev) > > +{ > > + free(dev->devnode); > > + free(dev->subsystem); > > + free(dev->syspath); > > + free(dev->drm_card); > > + free(dev->drm_render); > > + free(dev->vendor); > > + free(dev->device); > > + g_hash_table_destroy(dev->attrs_ht); > > + g_hash_table_destroy(dev->props_ht); > > +} > > + > > +/** > > + * igt_devices_scan > > + * @force: enforce scanning devices > > + * > > + * Function scans udev in search of gpu devices. For first run it can be > > + * called with @force = false. If something changes during the the test > > + * or test does some module loading (new drm devices occurs during execution) > > + * function must be called again with @force = true to refresh device array. > > + */ > > +void igt_devices_scan(bool force) > > +{ > > + if (force && igt_devs.devs_scanned) { > > + for (int i = 0; i < igt_devs.devs->len; i++) { > > + struct igt_device *dev = > > + g_ptr_array_index(igt_devs.devs, i); > > + igt_device_free(dev); > > + free(dev); > > + } > > + igt_devs.devs_scanned = false; > > + g_ptr_array_free(igt_devs.view, true); > > + g_ptr_array_free(igt_devs.devs, true); > > + igt_devs.view = NULL; > > + igt_devs.devs = NULL; > > + } > > + > > + if (igt_devs.devs_scanned) > > + return; > > + > > + populate_blacklist_keys(); //keys from sysattr we skip > > + populate_gpu_data(); > > + > > + prepare_scan(); > > + scan_drm_devices(); > > + > > + igt_devs.devs_scanned = true; > > +} > > + > > +#define pr_simple(k, v) printf(" %-16s: %s\n", k, v) > > +#define pr_simple2(k, v1, v2) printf(" %-16s: %s:%s\n", k, v1, v2) > > +#define pr_simple_prop(dev, key) pr_simple(key, get_prop(dev, key)) > > +#define pr_simple_attr(dev, key) pr_simple(key, get_attr(dev, key)) > > + > > +static void igt_devs_print_simple(GPtrArray *view) > > +{ > > + struct igt_device *dev; > > + int i; > > + > > + if (!view) > > + return; > > + > > + for (i = 0; i < view->len; i++) { > > + dev = g_ptr_array_index(view, i); > > + printf("%s:%s\n", dev->subsystem, dev->syspath); > > + if (dev->drm_card) > > + pr_simple("drm card", dev->drm_card); > > + if (dev->drm_render) > > + pr_simple("drm render", dev->drm_render); > > + if (is_drm_subsystem(dev)) { > > + pr_simple2("parent", dev->parent->subsystem, > > + dev->parent->syspath); > > + } else { > > + if (is_pci_subsystem(dev)) { > > + pr_simple("vendor", dev->vendor); > > + pr_simple("device", dev->device); > > + } > > + } > > + printf("\n"); > > + } > > +} > Playing around with usage of this code shows that we print 'nothing' when > nothing is found, what is good :) but we also do not print any information that > device do not exist, e.g. warining. Ok. I'll add it. > > > + > > +#define pr_detail(k, v) printf("%-32s: %s\n", k, v) > > + > > +static void print_ht(GHashTable *ht) > > +{ > > + GList *keys = g_hash_table_get_keys(ht); > > + keys = g_list_sort(keys, (GCompareFunc) strcmp); > > + while (keys) { > > + char *k = (char *) keys->data; > > + char *v = g_hash_table_lookup(ht, k); > > + pr_detail(k, v); > > + keys = g_list_next(keys); > > + } > > + g_list_free(keys); > > +} > > + > > +static void igt_devs_print_detail(GPtrArray *view) > > +{ > > + struct igt_device *dev; > > + int i; > > + > > + if (!view) > > + return; > > + > > + for (i = 0; i < view->len; i++) { > > + dev = g_ptr_array_index(view, i); > > + printf("========== %s:%s ==========\n", > > + dev->subsystem, dev->syspath); > > + if (!is_drm_subsystem(dev)) { > > + pr_detail("card device", dev->drm_card); > > + pr_detail("render device", dev->drm_render); > > + } > > + > > + printf("\n[properties]\n"); > > + print_ht(dev->props_ht); > > + printf("\n[attributes]\n"); > > + print_ht(dev->attrs_ht); > > + printf("\n"); > > + } > > +} > > + > > +static struct print_func { > > + void (*prn)(GPtrArray *); > Do we want to have a name here ^? > > > +} print_functions[] = { > > + [IGT_PRINT_SIMPLE] = { .prn = igt_devs_print_simple }, > > + [IGT_PRINT_DETAIL] = { .prn = igt_devs_print_detail }, > > +}; > > + > > +/** > > + * igt_devices_print > > + * @printtype: IGT_PRINT_SIMPLE or IGT_PRINT_DETAIL > > + * > > + * Function is used by 'lsgpu' tool to print device array in simple or detailed > > + * form. This function is added here instead in 'lsgpu' to avoid exposing > > + * internal implementation data structures. > > + */ > > +void igt_devices_print(enum igt_devices_print_type printtype) > > +{ > > + print_functions[printtype].prn(igt_devs.view); > > +} > Should it be possible to do something like: > 'lsgpu -D pci:/sys/devices/pci0000:00/0000:00:02.0' to show only this particular > device? Currently anything added after '-D' parameter is ignored. > > Writing this comment I realized that parameters '-d' and '-D' can be easily > mistaken. Maybe we should rethink the naming? For me -d and -D are ok, if you have better letters which can replace them please suggest me. You've realized me, that -D is now ignored and using with -d is not possible now. I'm going to fix it to allow user display detail of single device instead all of them. > > > + > > +/** > > + * igt_devices_print_vendors > > + * > > + * Print pci id -> vendor mappings. Vendor names printed by this function > > + * can be used for filters like pci which allows passing vendor - like > > + * vendor id (8086) or as a string (Intel). > > + */ > > +void igt_devices_print_vendors(void) > > +{ > > + struct name_value *list = &gpu_vendor_list[0]; > > + printf("Recognized vendors:\n"); > > + > > + printf("%-8s %-16s\n", "PCI ID", "vendor"); > > + while (list->name) { > > + struct vendor_spec *vs = (struct vendor_spec *) list->value; > > + if (vs) > > + printf("%-8s %-16s\n", list->name, vs->vendor); > > + list++; > > + } > > +} > > + > > +/* ------------------------------------------------------------------------- */ > > +#define FILTER_SPEC_NAME_LEN 32 > > +#define FILTER_SPEC_DATA_LEN 256 > > +struct filter_spec { > > + char name[FILTER_SPEC_NAME_LEN]; > > + char data[FILTER_SPEC_DATA_LEN]; > > +}; > > + > > +struct filter_func { > > + GPtrArray *(*filter_function)(struct filter_spec *fspec, > > + struct filter_func *ffunc); > > + const char *help; > > + const char *detail; > > + > > + const char *property; > > + const char *value; > > +}; > > + > > +struct filter_data { > > + char *vendor; > > + char *device; > > + char *card; > > + char *drm; > > +}; > > + > > +static GHashTable *split_filter_data(const char *data) > > +{ > > + GHashTable *ht = g_hash_table_new_full(g_str_hash, g_str_equal, > > + free, free); > > + gchar **s; > > + gchar **strv = g_strsplit(data, ",", -1); > > + > > + s = strv; > > + while (*s) { > > + char *k, *v; > > + v = strchr(*s, '='); > > + if (!v) { > > + s++; > > + continue; > > + } > > + k = strndup(*s, v - (*s)); > > + v = strdup(v + 1); > > + g_hash_table_insert(ht, k, v); > Just a question here ^ - would checking that k and v be too much here? -> are we > sure that value should be valid or g_hash_table_insert is dealing with that? You don't need to check k and v here. Finding '=' is enough to properly handle and make k and v strings. > > > + s++; > > + } > > + g_strfreev(strv); > > + > > + return ht; > > +} > > + > > +static bool get_filter_spec(const char *filter, struct filter_spec *spec) > > +{ > > + if (!filter) > > + return false; > > + > > + if (sscanf(filter, "%31[^:]:%255s", spec->name, spec->data) >= 1) > > + return true; > > + > > + return false; > > +} > > + > > +static bool is_vendor_matched(struct igt_device *dev, const char *vendor) > > +{ > > + const char *vendor_id; > > + > > + if (!dev->vendor || !vendor) > > + return false; > > + > > + /* First we compare vendor id, like 8086 */ > > + if (!strcasecmp(dev->vendor, vendor)) > > + return true; > > + > > + /* Likely we have vendor string instead of id */ > > + vendor_id = get_pci_vendor_id_by_name(vendor); > > + if (!vendor_id) > > + return false; > > + > > + return !strcasecmp(dev->vendor, vendor_id); > > +} > > + > > +/* Filter which matches subsystem:/sys/... path. > > + * Used as first filter in chain. > > + */ > > +static GPtrArray *filter_sys(struct filter_spec *fspec, > > + struct filter_func *ffunc) > > +{ > > + (void) ffunc; > > + > > + DBG("filter sys\n"); > > + if (!strlen(fspec->data)) > > + return igt_devs.view; > > + > > + for (int i = 0; i < igt_devs.devs->len; i++) { > > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > > + > > + if (strequal(dev->subsystem, fspec->name) && > > + strequal(dev->syspath, fspec->data)) { > > + g_ptr_array_add(igt_devs.view, dev); > > + break; > > + } > > + } > > + > > + DBG("Filter sys view size: %d\n", igt_devs.view->len); > > + > > + return igt_devs.view; > > +} > > + > > +/* Find drm device using direct path to /dev/dri/. > > + * It extends filter_sys to allow using drm:/dev/dri/cardX and > > + * drm:/dev/dri/renderDX filter syntax. > > + */ > > +static GPtrArray *filter_drm(struct filter_spec *fspec, > > + struct filter_func *ffunc) > > +{ > > + (void) ffunc; > > + > > + DBG("filter drm\n"); > > + if (!strlen(fspec->data)) > > + return igt_devs.view; > > + > > + for (int i = 0; i < igt_devs.devs->len; i++) { > > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > > + if (!is_drm_subsystem(dev)) > > + continue; > > + > > + if (strequal(dev->syspath, fspec->data) || > > + strequal(dev->drm_card, fspec->data) || > > + strequal(dev->drm_render, fspec->data)) { > > + g_ptr_array_add(igt_devs.view, dev); > > + break; > > + } > > + } > > + > > + DBG("Filter drm view size: %d\n", igt_devs.view->len); > > + > > + return igt_devs.view; > > +} > > + > > +/* Find appropriate pci device matching vendor/device/card filter arguments > > +*/ > > +static GPtrArray *filter_pci(struct filter_spec *fspec, > > + struct filter_func *ffunc) > > +{ > > + GHashTable *ht; > > + struct filter_data fdata; > > + int card = -1; > > + (void) ffunc; > > + > > + DBG("filter pci\n"); > > + > > + ht = split_filter_data(fspec->data); > > + fdata.vendor = g_hash_table_lookup(ht, "vendor"); > > + fdata.device = g_hash_table_lookup(ht, "device"); > > + fdata.card = g_hash_table_lookup(ht, "card"); > > + > > + if (fdata.card) { > > + sscanf(fdata.card, "%d", &card); > Probably we need to check sscanf result here? No. card = -1 during initialisation, so only successful scanf can change it. > > + if (card < 0) { > > + g_hash_table_destroy(ht); > > + return igt_devs.view; > > + } > > + } else { > > + card = 0; > > + } > > + > > + for (int i = 0; i < igt_devs.devs->len; i++) { > > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > > + > > + if (!is_pci_subsystem(dev)) > > + continue; > > + > > + /* Skip if 'vendor' doesn't match (hex or name) */ > > + if (fdata.vendor && !is_vendor_matched(dev, fdata.vendor)) > > + continue; > > + > > + /* Skip if 'device' doesn't match */ > > + if (fdata.device && strcasecmp(fdata.device, dev->device)) > > + continue; > > + > > + /* We get n-th card */ > > + if (!card) { > > + g_ptr_array_add(igt_devs.view, dev); > > + break; > > + } > > + card--; > > + } > > + > > + DBG("Filter pci view size: %d\n", igt_devs.view->len); > > + > > + g_hash_table_destroy(ht); > > + > > + return igt_devs.view; > > +} > > + > > +/* Helper for finding first device matching property:value. > > +*/ > > +static GPtrArray *filter_property(struct filter_spec *fspec, > > + struct filter_func *ffunc) > > +{ > > + GHashTable *ht; > > + struct filter_data fdata; > > + int card = -1; > > + const char *property = ffunc->property; > > + const char *value = ffunc->value; > > + > > + if (!property || !value) > > + return igt_devs.view; > > + > > + DBG("filter property/value [%s/%s]\n", property, value); > > + > > + ht = split_filter_data(fspec->data); > > + fdata.card = g_hash_table_lookup(ht, "card"); > > + > > + if (fdata.card) { > > + sscanf(fdata.card, "%d", &card); > > + if (card < 0) { > > + g_hash_table_destroy(ht); > > + return igt_devs.view; > > + } > > + } else { > > + card = 0; > > + } > > + > > + for (int i = 0; i < igt_devs.devs->len; i++) { > > + const char *prop_value; > > + struct igt_device *dev = g_ptr_array_index(igt_devs.devs, i); > > + > > + prop_value = get_prop(dev, property); > > + if (prop_value && !strcmp(prop_value, value)) { > > + if (!card) { > > + g_ptr_array_add(igt_devs.view, dev); > > + break; > > + } > > + card--; > > + } > > + } > > + > > + DBG("Filter view size: %d\n", igt_devs.view->len); > > + > > + g_hash_table_destroy(ht); > > + > > + return igt_devs.view; > > +} > > + > > +static GPtrArray *filter_vgem(struct filter_spec *fspec, > > + struct filter_func *ffunc) > > +{ > > + GPtrArray *view = filter_property(fspec, ffunc); > > + if (view) { > > + struct igt_device *dev = g_ptr_array_index(view, 0); > > + dev->vs = &v_vgem; > > + } > > + return view; > > +} > > + > > +static GPtrArray *filter_vkms(struct filter_spec *fspec, > > + struct filter_func *ffunc) > > +{ > > + return filter_property(fspec, ffunc); > > +} > > + > > +static GPtrArray *filter_vc4(struct filter_spec *fspec, > > + struct filter_func *ffunc) > > +{ > > + GPtrArray *view = filter_property(fspec, ffunc); > > + if (view) { > > + struct igt_device *dev = g_ptr_array_index(view, 0); > > + dev->vs = &v_vc4; > > + } > > + return filter_property(fspec, ffunc); > > +} > > + > > +static struct filter_func f_drm = { .filter_function = filter_drm, > > + .help = "drm:/dev/dri/* path", > > + .detail = "find drm device by /dev/dri/* node\n", > > + }; > > + > > +static struct filter_func f_pci = { .filter_function = filter_pci, > > + .help = "pci:[vendor=%04x/name][,device=%04x][,card=%d]", > > + .detail = "vendor is hex number or vendor name\n", > > + }; > > + > > +static struct filter_func f_vgem = { .filter_function = filter_vgem, > > + .help = "vgem:[card=%d]", > > + .detail = "card is n-th vgem card number\n", > > + .property = "MODALIAS", > > + .value = "platform:vgem", > > + }; > > + > > +static struct filter_func f_vkms = { .filter_function = filter_vkms, > > + .help = "vkms:[card=%d]", > > + .detail = "card is n-th vkms card number\n", > > + .property = "MODALIAS", > > + .value = "platform:vkms", > > + }; > > + > > +static struct filter_func f_vc4 = { .filter_function = filter_vc4, > > + .help = "vc4:[card=%d]", > > + .detail = "card is n-th vc4 card number\n", > > + .property = "OF_COMPATIBLE_0", > > + .value = "brcm,bcm2835-vc4", > > + }; > > + > > +struct name_value filter_definition_list[] = { > > + { "drm", (gpointer) &f_drm }, > > + { "pci", (gpointer) &f_pci }, > > + { "vgem", (gpointer) &f_vgem }, > > + { "vkms", (gpointer) &f_vkms }, > > + { "vc4", (gpointer) &f_vc4 }, > > + { NULL, }, > > +}; > > + > > +/** > > + * @igt_device_print_filter_types > > + * > > + * Print all filters syntax for device selection. > > + */ > > +void igt_device_print_filter_types(void) > > +{ > > + struct name_value *list = &filter_definition_list[0]; > > + printf("Filter types:\n---\n"); > > + printf("%-12s %s\n---\n", "filter", "syntax"); > > + > > + printf("%-12s %s\n", "", "direct subsystem:/sys/... path selection\n"); > > + printf("%-12s %s\n", "", > > + "ex: drm:/sys/devices/pci0000:00/0000:00:02.0/drm/card0"); > > + printf("%-12s %s\n", "", > > + " pci:/sys/devices/pci0000:00/0000:00:02.0"); > > + printf("%-12s %s\n", "", > > + " platform:/sys/devices/platform/vgem\n"); > > + > > + while (list->name) { > > + struct filter_func *v = (struct filter_func *) list->value; > > + printf("%-12s %s\n", list->name, v->help); > > + printf("%-12s %s\n", "", v->detail); > > + list++; > > + } > > +} > > + > > +static GPtrArray *device_filters = NULL; > I wonder why chechpatch returns error here ^. It is not illegal, maybe it shouts > becasue it is done by default? Static variables are zeroed by default, but I like in some specific places points that this is NULL. I've seen that other IGT developers also assign some static variables in some files. > > + > > +#define DEVICE_FILTER_CHECK_ALLOC() \ > > + do { \ > > + if (!device_filters) \ > > + device_filters = g_ptr_array_new_full(2, free); \ > > + igt_assert(device_filters); \ > > + } while(0) > > + > > +/** > > + * igt_device_filter_count > > + * > > + * Returns number of filters collected in the filter array. > > + */ > > +int igt_device_filter_count(void) > > +{ > > + DEVICE_FILTER_CHECK_ALLOC(); > > + > > + return device_filters->len; > > +} > > + > > +/** > > + * igt_device_filter_add > > + * @filter: filter to be stored in filter array > > + * > > + * Function allows passing single or more filters within one string. This is > > + * for CI when it can extract filter from environment variable (and it must > > + * be single string). So if @filter contains semicolon ';' it treats > > + * each part as separate filter and adds to the filter array. > > + * > > + * Returns number of filters added to filter array. Can be greater than > > + * 1 if @filter contains more than one filter separated by semicolon. > > + */ > > +int igt_device_filter_add(const char *filter) > > +{ > > + gchar **strv, **s; > > + int c = 0; > > + > > + DEVICE_FILTER_CHECK_ALLOC(); > > + > > + strv = g_strsplit(filter, ";", -1); > > + > > + s = strv; > > + while (*s) { > > + g_ptr_array_add(device_filters, strdup(*s)); > > + s++; > > + } > > + g_strfreev(strv); > > + > > + return c; > > +} > > + > > +/** > > + * igt_device_filter_get > > + * @num: Number of filter from filter array > > + * > > + * Returns filter string or NULL if @num is out of range of filter array. > > +*/ > > +const char *igt_device_filter_get(int num) > > +{ > > + DEVICE_FILTER_CHECK_ALLOC(); > > + > > + if (num < 0 || num >= device_filters->len) > > + return NULL; > > + > > + return g_ptr_array_index(device_filters, num); > > +} > > + > > +static bool igt_device_filter_apply(const char *filter) > > +{ > > + struct filter_spec fspec; > > + bool ret; > > + > > + if (!filter) > > + return false; > > + > > + memset(&fspec, 0, sizeof(fspec)); > > + ret = get_filter_spec(filter, &fspec); > > + if (!ret) { > > + igt_warn("Can't split filter [%s]\n", filter); > > + return false; > > + } > > + > > + /* Clean view */ > > + g_ptr_array_remove_range(igt_devs.view, 0, igt_devs.view->len); > > + > > + /* If fspec.data contains "/sys" use direct path instead > > + * contextual filter */ > > + if (!strncmp(fspec.data, "/sys", 4)) > > + filter_sys(&fspec, NULL); > > + else { > > + struct filter_func *ffunc; > > + ffunc = g_hash_table_lookup(filter_definition_ht, fspec.name); > > + if (!ffunc) { > > + igt_warn("No filter with name [%s]\n", fspec.name); > > + return false; > > + } > > + ffunc->filter_function(&fspec, ffunc); > > + } > > + > Should we free(fspec) at the end? Or it is done somewhere else? fspec is a structure on the stack, you don't need do free it. > > + return true; > > +} > > + > > +#define safe_strncpy(dst, src, size) \ > > + if ((dst) && (src)) strncpy((dst), (src), (size)) > > + > > +/** > > + * igt_device_card_match > > + * @filter: filter string > > + * @card: pointer to igt_device_card struct > > + * > > + * Function applies filter to match device from device array. > > + * > > + * Returns: > > + * false - no card pointer was passed or card wasn't matched, > > + * true - card matched and returned. > > + */ > > +bool igt_device_card_match(const char *filter, struct igt_device_card *card) > > +{ > > + struct igt_device *dev = NULL; > > + > > + if (!card) > > + return false; > > + memset(card, 0, sizeof(*card)); > > + > > + igt_devices_scan(false); > > + > > + if (igt_device_filter_apply(filter) == false) > > + return false; > > + > > + if (!igt_devs.view->len) > > + return false; > > + > > + /* We take first one if more than one card matches filter */ > > + dev = g_ptr_array_index(igt_devs.view, 0); > > + card->chipset = DRIVER_ANY; > > + if (dev->vs) > > + card->chipset = dev->vs->chipset; > > + > > + safe_strncpy(card->subsystem, dev->subsystem, NAME_MAX); > > + safe_strncpy(card->card, dev->drm_card, NAME_MAX); > > + safe_strncpy(card->render, dev->drm_render, NAME_MAX); > This macro is returing true/false, do we check the results? It looks like magic > to me :) strcpy require valid src and dst pointers. This macro ensures that for null src or null dst strcpy won't be called. > > + > > + return true; > > +} > > diff --git a/lib/igt_device_scan.h b/lib/igt_device_scan.h > > new file mode 100644 > > index 00000000..62f7f39a > > --- /dev/null > > +++ b/lib/igt_device_scan.h > > @@ -0,0 +1,69 @@ > > +/* > > + * Copyright © 2019 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the next > > + * paragraph) shall be included in all copies or substantial portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > + * IN THE SOFTWARE. > > + * > > + */ > > + > > +#ifndef __IGT_DEVICE_SCAN_H__ > > +#define __IGT_DEVICE_SCAN_H__ > > + > > +#include <limits.h> > > +#include <igt.h> > > + > > +enum igt_devices_print_type { > > + IGT_PRINT_SIMPLE, > > + IGT_PRINT_DETAIL, > > +}; > > + > > +struct igt_device_card { > > + int chipset; > > + char subsystem[NAME_MAX]; > > + char card[NAME_MAX]; > > + char render[NAME_MAX]; > > +}; > > + > > +/* Scan udev subsystems. Do it once unless force is true, what rescans > > + * devices again */ > > +void igt_devices_scan(bool force); > > + > > +void igt_devices_print(enum igt_devices_print_type printtype); > > +void igt_devices_print_vendors(void); > > +void igt_device_print_filter_types(void); > > + > > +/* > > + * Handle device filter collection array. > > + * IGT can store/retrieve filters passed by user using '--device' args. > > +*/ > This comment ^^ is a general one for what is below? If yes - it is not obvious > :/ This points where you can find its usage in IGT. > > + > > +/* Return number of filters collected */ > > +int igt_device_filter_count(void); > > + > > +/* Add filter(s) to the array. Returns number of filters added (>1 if > > + * user passes more than one filter separatined by ';') */ > > +int igt_device_filter_add(const char *filter); > > + > > +/* Get n-th filter stored in the array or NULL */ > > +const char *igt_device_filter_get(int num); > > + > > +/* Use filter to match the device and fill card structure */ > > +bool igt_device_card_match(const char *filter, struct igt_device_card *card); > > + > > +#endif /* __IGT_DEVICE_SCAN_H__ */ > > diff --git a/lib/meson.build b/lib/meson.build > > index 157624e7..826ebbe3 100644 > > --- a/lib/meson.build > > +++ b/lib/meson.build > > @@ -10,6 +10,7 @@ lib_sources = [ > > 'igt_color_encoding.c', > > 'igt_debugfs.c', > > 'igt_device.c', > > + 'igt_device_scan.c', > > 'igt_aux.c', > > 'igt_gpu_power.c', > > 'igt_gt.c', > > diff --git a/tools/Makefile.sources b/tools/Makefile.sources > > index 50706f41..0e67b654 100644 > > --- a/tools/Makefile.sources > > +++ b/tools/Makefile.sources > > @@ -33,6 +33,7 @@ tools_prog_lists = \ > > intel_watermark \ > > intel_gem_info \ > > intel_gvtg_test \ > > + lsgpu \ > > $(NULL) > > > > dist_bin_SCRIPTS = intel_gpu_abrt > > diff --git a/tools/lsgpu.c b/tools/lsgpu.c > > new file mode 100644 > > index 00000000..e723f0b2 > > --- /dev/null > > +++ b/tools/lsgpu.c > > @@ -0,0 +1,183 @@ > > +/* > > + * Copyright © 2019 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the next > > + * paragraph) shall be included in all copies or substantial portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > + * IN THE SOFTWARE. > > + * > > + */ > > + > > +#include "igt_device_scan.h" > > +#include "igt.h" > > +#include <sys/ioctl.h> > > +#include <fcntl.h> > > +#include <errno.h> > > +#include <string.h> > > +#include <signal.h> > > + > > +enum { > > + OPT_PRINT_DETAIL = 'D', > > + OPT_LIST_VENDORS = 'v', > > + OPT_LIST_FILTERS = 'l', > > + OPT_DEVICE = 'd', > > + OPT_HELP = 'h' > > +}; > > + > > +bool g_show_vendors; > > +bool g_list_filters; > > +bool g_device; > > +bool g_help; > > + > > +static const char *usage_str = > > + "usage: lsgpu [options]\n\n" > I would add 1 sentence about what pretty cool stuff this tool can do :) > > + "Options:\n" > > + " -D, --print-details Print devices with details\n" > As I wrote above '-D' and '-d' can be easily mistaken, nothing will blow up, but > maybe we can use different letter here? It it if for printing then maybe > '-P'/'-p'? -p looks good, I'll change it. > > + " -v, --list-vendors List recognized vendors\n" > > + " -l, --list-filter-types List registered device filters types\n" > > + " -d, --device filter Device filter, can be given multiple times\n" > > + " -h, --help Show this help message and exit\n"; > > + > > +static void test_device_open(struct igt_device_card *card) > > +{ > > + int fd; > > + > > + if (!card) > > + return; > > + > > + fd = drm_open_card(card); > > + if (fd >= 0) { > > + printf("Device %s successfully opened\n", card->card); > > + close(fd); > > + } else { > > + if (strlen(card->card)) > > + printf("Cannot open card %s device\n", card->card); > > + else > > + printf("Cannot open card device, empty name\n"); > > + } > > + > > + fd = drm_open_render(card); > > + if (fd >= 0) { > > + printf("Device %s successfully opened\n", card->render); > > + close(fd); > > + } else { > > + if (strlen(card->render)) > > + printf("Cannot open render %s device\n", card->render); > > + else > > + printf("Cannot open render device, empty name\n"); > > + } > We do not get any warning when non-existing device is used, is it ok? In all cases you'll get info message. Non-existing device is fd == -1. > > +} > > + > > +static void print_card(struct igt_device_card *card) > > +{ > > + if (!card) > > + return; > > + > > + printf("subsystem : %s\n", card->subsystem); > > + printf("chipset : %x\n", card->chipset); > > + printf("drm card : %s\n", card->card); > > + printf("drm render : %s\n", card->render); > > +} > > + > > +int main(int argc, char *argv[]) > > +{ > > + static struct option long_options[] = { > > + {"print-detail", no_argument, NULL, OPT_PRINT_DETAIL}, > > + {"list-vendors", no_argument, NULL, OPT_LIST_VENDORS}, > > + {"list-filter-types", no_argument, NULL, OPT_LIST_FILTERS}, > > + {"device", required_argument, NULL, OPT_DEVICE}, > > + {"help", no_argument, NULL, OPT_HELP}, > > + {0, 0, 0, 0} > > + }; > > + int c, index = 0; > > + const char *env; > > + enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE; > > + > > + while ((c = getopt_long(argc, argv, "Dvld:h", > > + long_options, &index)) != -1) { > > + switch(c) { > > + > > + case OPT_PRINT_DETAIL: > > + printtype = IGT_PRINT_DETAIL; > > + break; > > + case OPT_LIST_VENDORS: > > + g_show_vendors = true; > > + break; > > + case OPT_LIST_FILTERS: > > + g_list_filters = true; > > + break; > > + case OPT_DEVICE: > > + g_device = true; > > + igt_device_filter_add(optarg); > > + break; > > + case OPT_HELP: > > + g_help = true; > > + break; > > + } > > + } > > + > > + if (g_help) { > > + printf("%s\n", usage_str); > > + exit(0); > > + } > > + > > + env = getenv("IGT_DEVICE"); > > + if (env) { > > + igt_device_filter_add(env); > > + g_device = true; > > + } > > + > > + if (g_show_vendors) { > > + igt_devices_print_vendors(); > > + return 0; > > + } > > + > > + if (g_list_filters) { > > + igt_device_print_filter_types(); > > + return 0; > > + } > > + > > + igt_devices_scan(false); > > + > > + if (g_device) { > > + int n = igt_device_filter_count(); > > + printf("=== Device filter list ===\n"); > > + for (int i = 0; i < n; i++) { > > + printf("[%2d]: %s\n", i, > > + igt_device_filter_get(i)); > > + } > > + printf("\n"); > > + > > + printf("=== Testing device open ===\n"); > > + for (int i = 0; i < n; i++) { > > + struct igt_device_card card; > > + const char *filter = igt_device_filter_get(i); > > + > > + if (!igt_device_card_match(filter, &card)) > > + continue; > > + print_card(&card); > > + test_device_open(&card); > > + printf("---\n"); > > + } > > + > > + return 0; > > + } > > + > > + igt_devices_print(printtype); > > + > > + return 0; > > +} > > diff --git a/tools/meson.build b/tools/meson.build > > index 6e72b263..9b3a2a69 100644 > > --- a/tools/meson.build > > +++ b/tools/meson.build > > @@ -36,6 +36,7 @@ tools_progs = [ > > 'intel_gem_info', > > 'intel_gvtg_test', > > 'dpcd_reg', > > + 'lsgpu', > > ] > > tool_deps = igt_deps > > Zbyszek/Arek - is there any decision about IGT documentation? Should we follow some > standards? I am asking because doc sections in this library are in various styles. I > know that documenting stuff is boring, but futureproof :) Should we unify doc > sections here? > What I mean is: should we have something like: > > /** > * function_name: > * @param1: > * @paramX: > * > * Description.... > * Return value > */ > > also here? Once sentence doc is beter then no doc, but... For API functions I provided gtk-doc format. For static I'm not sure but this info is not generated. And from my point of view adding comments for self-explained code is not good idea. > > Zbyszku - huge work done! > Kasia > Zbigniew _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT 2019-08-12 8:47 [igt-dev] [PATCH i-g-t v4 0/2] Add device selection methods Zbigniew Kempczyński 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API Zbigniew Kempczyński @ 2019-08-12 8:47 ` Zbigniew Kempczyński 2019-08-21 7:56 ` Arkadiusz Hiler 2019-08-21 12:08 ` Katarzyna Dec 2019-08-12 10:52 ` [igt-dev] ✓ Fi.CI.BAT: success for Add device selection methods (rev4) Patchwork 2019-08-12 13:29 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork 3 siblings, 2 replies; 13+ messages in thread From: Zbigniew Kempczyński @ 2019-08-12 8:47 UTC (permalink / raw) To: igt-dev; +Cc: Daniel Vetter, Petri Latvala 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@intel.com> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Petri Latvala <petri.latvala@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). + * + * 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) { + bool found; + struct igt_device_card card; + + found = __find_card_with_chipset(chipset, &card); + if (!found || !strlen(card.card)) + return -1; + + 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); +} + +/** + * 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; -- 2.21.0 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT Zbigniew Kempczyński @ 2019-08-21 7:56 ` Arkadiusz Hiler 2019-08-21 10:42 ` Zbigniew Kempczyński 2019-08-21 12:08 ` Katarzyna Dec 1 sibling, 1 reply; 13+ messages in thread From: Arkadiusz Hiler @ 2019-08-21 7:56 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev, Petri Latvala, Daniel Vetter 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@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Petri Latvala <petri.latvala@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); 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 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. > + > +/** > + * 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. -- Cheers, Arek _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT 2019-08-21 7:56 ` Arkadiusz Hiler @ 2019-08-21 10:42 ` Zbigniew Kempczyński 0 siblings, 0 replies; 13+ messages in thread From: Zbigniew Kempczyński @ 2019-08-21 10:42 UTC (permalink / raw) To: Arkadiusz Hiler; +Cc: igt-dev, Petri Latvala, Daniel Vetter 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@intel.com> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Petri Latvala <petri.latvala@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 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT Zbigniew Kempczyński 2019-08-21 7:56 ` Arkadiusz Hiler @ 2019-08-21 12:08 ` Katarzyna Dec 1 sibling, 0 replies; 13+ messages in thread From: Katarzyna Dec @ 2019-08-21 12:08 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev, Petri Latvala, Daniel Vetter 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@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Petri Latvala <petri.latvala@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). > + * > + * 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) { You do not use this variable any where more, so maybe: if (igt_device_filter_count()) ?? > + bool found; > + struct igt_device_card card; > + > + found = __find_card_with_chipset(chipset, &card); > + if (!found || !strlen(card.card)) > + return -1; > + > + 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) { Same here :) > + 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); > +} > + > +/** > + * 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; > -- > 2.21.0 Generally looks good :) Kasia > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for Add device selection methods (rev4) 2019-08-12 8:47 [igt-dev] [PATCH i-g-t v4 0/2] Add device selection methods Zbigniew Kempczyński 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API Zbigniew Kempczyński 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT Zbigniew Kempczyński @ 2019-08-12 10:52 ` Patchwork 2019-08-12 13:29 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork 3 siblings, 0 replies; 13+ messages in thread From: Patchwork @ 2019-08-12 10:52 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev == Series Details == Series: Add device selection methods (rev4) URL : https://patchwork.freedesktop.org/series/63553/ State : success == Summary == CI Bug Log - changes from CI_DRM_6683 -> IGTPW_3332 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://patchwork.freedesktop.org/api/1.0/series/63553/revisions/4/mbox/ Known issues ------------ Here are the changes found in IGTPW_3332 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@i915_selftest@live_workarounds: - fi-bsw-kefka: [PASS][1] -> [DMESG-WARN][2] ([fdo#111373]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/fi-bsw-kefka/igt@i915_selftest@live_workarounds.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/fi-bsw-kefka/igt@i915_selftest@live_workarounds.html * igt@kms_frontbuffer_tracking@basic: - fi-icl-u2: [PASS][3] -> [FAIL][4] ([fdo#103167]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: - fi-skl-6260u: [PASS][5] -> [INCOMPLETE][6] ([fdo#104108]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/fi-skl-6260u/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/fi-skl-6260u/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html #### Possible fixes #### * igt@gem_ctx_param@basic: - {fi-icl-guc}: [INCOMPLETE][7] ([fdo#107713]) -> [PASS][8] [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/fi-icl-guc/igt@gem_ctx_param@basic.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/fi-icl-guc/igt@gem_ctx_param@basic.html * igt@gem_exec_suspend@basic-s4-devices: - fi-blb-e6850: [INCOMPLETE][9] ([fdo#107718]) -> [PASS][10] [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html * igt@i915_module_load@reload-no-display: - {fi-icl-u4}: [DMESG-WARN][11] ([fdo#105602]) -> [PASS][12] [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/fi-icl-u4/igt@i915_module_load@reload-no-display.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/fi-icl-u4/igt@i915_module_load@reload-no-display.html * igt@kms_chamelium@hdmi-hpd-fast: - fi-kbl-7567u: [FAIL][13] ([fdo#109485]) -> [PASS][14] [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/fi-kbl-7567u/igt@kms_chamelium@hdmi-hpd-fast.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/fi-kbl-7567u/igt@kms_chamelium@hdmi-hpd-fast.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108 [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602 [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713 [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718 [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485 [fdo#111373]: https://bugs.freedesktop.org/show_bug.cgi?id=111373 Participating hosts (53 -> 46) ------------------------------ Additional (1): fi-icl-u3 Missing (8): fi-kbl-soraka fi-cml-u2 fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-y fi-byt-clapper Build changes ------------- * CI: CI-20190529 -> None * IGT: IGT_5127 -> IGTPW_3332 CI-20190529: 20190529 CI_DRM_6683: cc4894120704c613b5d343d3e72c11384f780f2a @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_3332: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/ IGT_5127: f43f5fa12ac1b93febfe3eeb9e9985f5f3e2eff0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/ _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* [igt-dev] ✗ Fi.CI.IGT: failure for Add device selection methods (rev4) 2019-08-12 8:47 [igt-dev] [PATCH i-g-t v4 0/2] Add device selection methods Zbigniew Kempczyński ` (2 preceding siblings ...) 2019-08-12 10:52 ` [igt-dev] ✓ Fi.CI.BAT: success for Add device selection methods (rev4) Patchwork @ 2019-08-12 13:29 ` Patchwork 3 siblings, 0 replies; 13+ messages in thread From: Patchwork @ 2019-08-12 13:29 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev == Series Details == Series: Add device selection methods (rev4) URL : https://patchwork.freedesktop.org/series/63553/ State : failure == Summary == CI Bug Log - changes from CI_DRM_6683_full -> IGTPW_3332_full ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with IGTPW_3332_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in IGTPW_3332_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://patchwork.freedesktop.org/api/1.0/series/63553/revisions/4/mbox/ Possible new issues ------------------- Here are the unknown changes that may have been introduced in IGTPW_3332_full: ### IGT changes ### #### Possible regressions #### * igt@kms_vblank@pipe-a-ts-continuation-modeset-hang: - shard-kbl: [PASS][1] -> [DMESG-WARN][2] +1 similar issue [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-kbl6/igt@kms_vblank@pipe-a-ts-continuation-modeset-hang.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-kbl6/igt@kms_vblank@pipe-a-ts-continuation-modeset-hang.html Known issues ------------ Here are the changes found in IGTPW_3332_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_bad_reloc@negative-reloc-bsd2: - shard-iclb: [PASS][3] -> [SKIP][4] ([fdo#109276]) +10 similar issues [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb4/igt@gem_bad_reloc@negative-reloc-bsd2.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb7/igt@gem_bad_reloc@negative-reloc-bsd2.html * igt@gem_exec_schedule@independent-bsd: - shard-iclb: [PASS][5] -> [SKIP][6] ([fdo#111325]) +2 similar issues [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb7/igt@gem_exec_schedule@independent-bsd.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb1/igt@gem_exec_schedule@independent-bsd.html * igt@i915_pm_rc6_residency@rc6-accuracy: - shard-kbl: [PASS][7] -> [SKIP][8] ([fdo#109271]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-kbl3/igt@i915_pm_rc6_residency@rc6-accuracy.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-kbl7/igt@i915_pm_rc6_residency@rc6-accuracy.html * igt@i915_suspend@debugfs-reader: - shard-kbl: [PASS][9] -> [INCOMPLETE][10] ([fdo#103665]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-kbl6/igt@i915_suspend@debugfs-reader.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-kbl3/igt@i915_suspend@debugfs-reader.html * igt@kms_cursor_crc@pipe-b-cursor-suspend: - shard-hsw: [PASS][11] -> [INCOMPLETE][12] ([fdo#103540]) [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-hsw2/igt@kms_cursor_crc@pipe-b-cursor-suspend.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-hsw7/igt@kms_cursor_crc@pipe-b-cursor-suspend.html * igt@kms_flip_tiling@flip-x-tiled: - shard-iclb: [PASS][13] -> [FAIL][14] ([fdo#108303]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb7/igt@kms_flip_tiling@flip-x-tiled.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb6/igt@kms_flip_tiling@flip-x-tiled.html * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-gtt: - shard-iclb: [PASS][15] -> [FAIL][16] ([fdo#103167]) +2 similar issues [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-gtt.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-gtt.html * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-mmap-cpu: - shard-glk: [PASS][17] -> [FAIL][18] ([fdo#103167]) [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-glk3/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-mmap-cpu.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-glk3/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-shrfb-draw-mmap-cpu.html * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes: - shard-kbl: [PASS][19] -> [DMESG-WARN][20] ([fdo#108566]) [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-kbl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-kbl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html * igt@kms_plane_lowres@pipe-a-tiling-x: - shard-iclb: [PASS][21] -> [FAIL][22] ([fdo#103166]) [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb8/igt@kms_plane_lowres@pipe-a-tiling-x.html * igt@kms_psr@psr2_basic: - shard-iclb: [PASS][23] -> [SKIP][24] ([fdo#109441]) +1 similar issue [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb2/igt@kms_psr@psr2_basic.html [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb6/igt@kms_psr@psr2_basic.html * igt@kms_vblank@pipe-c-ts-continuation-suspend: - shard-apl: [PASS][25] -> [DMESG-WARN][26] ([fdo#108566]) +8 similar issues [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-apl3/igt@kms_vblank@pipe-c-ts-continuation-suspend.html [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-apl5/igt@kms_vblank@pipe-c-ts-continuation-suspend.html #### Possible fixes #### * igt@gem_ctx_isolation@rcs0-s3: - shard-apl: [DMESG-WARN][27] ([fdo#108566]) -> [PASS][28] +2 similar issues [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-apl7/igt@gem_ctx_isolation@rcs0-s3.html [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-apl4/igt@gem_ctx_isolation@rcs0-s3.html * igt@gem_ctx_shared@exec-single-timeline-bsd: - shard-iclb: [SKIP][29] ([fdo#110841]) -> [PASS][30] [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb1/igt@gem_ctx_shared@exec-single-timeline-bsd.html [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb3/igt@gem_ctx_shared@exec-single-timeline-bsd.html * igt@gem_exec_balancer@smoke: - shard-iclb: [SKIP][31] ([fdo#110854]) -> [PASS][32] [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb3/igt@gem_exec_balancer@smoke.html [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb2/igt@gem_exec_balancer@smoke.html * igt@gem_exec_schedule@preempt-other-chain-bsd: - shard-iclb: [SKIP][33] ([fdo#111325]) -> [PASS][34] +4 similar issues [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb6/igt@gem_exec_schedule@preempt-other-chain-bsd.html * igt@i915_pm_rc6_residency@rc6-accuracy: - shard-snb: [SKIP][35] ([fdo#109271]) -> [PASS][36] [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-snb4/igt@i915_pm_rc6_residency@rc6-accuracy.html [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-snb4/igt@i915_pm_rc6_residency@rc6-accuracy.html * igt@kms_busy@extended-modeset-hang-newfb-render-c: - shard-iclb: [INCOMPLETE][37] ([fdo#107713]) -> [PASS][38] [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb7/igt@kms_busy@extended-modeset-hang-newfb-render-c.html [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb7/igt@kms_busy@extended-modeset-hang-newfb-render-c.html * igt@kms_flip@dpms-vs-vblank-race-interruptible: - shard-glk: [FAIL][39] ([fdo#103060]) -> [PASS][40] [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-glk2/igt@kms_flip@dpms-vs-vblank-race-interruptible.html [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-glk6/igt@kms_flip@dpms-vs-vblank-race-interruptible.html * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw: - shard-iclb: [FAIL][41] ([fdo#103167]) -> [PASS][42] [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html * igt@kms_psr2_su@page_flip: - shard-iclb: [SKIP][43] ([fdo#109642] / [fdo#111068]) -> [PASS][44] [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb4/igt@kms_psr2_su@page_flip.html [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb2/igt@kms_psr2_su@page_flip.html * igt@kms_psr@no_drrs: - shard-iclb: [FAIL][45] ([fdo#108341]) -> [PASS][46] [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb1/igt@kms_psr@no_drrs.html [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb4/igt@kms_psr@no_drrs.html * igt@kms_psr@psr2_cursor_plane_onoff: - shard-iclb: [SKIP][47] ([fdo#109441]) -> [PASS][48] +3 similar issues [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb5/igt@kms_psr@psr2_cursor_plane_onoff.html [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html * igt@kms_vblank@pipe-b-ts-continuation-suspend: - shard-kbl: [INCOMPLETE][49] ([fdo#103665]) -> [PASS][50] [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-kbl2/igt@kms_vblank@pipe-b-ts-continuation-suspend.html [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-kbl7/igt@kms_vblank@pipe-b-ts-continuation-suspend.html * igt@perf_pmu@rc6: - shard-kbl: [SKIP][51] ([fdo#109271]) -> [PASS][52] [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-kbl7/igt@perf_pmu@rc6.html [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-kbl1/igt@perf_pmu@rc6.html * igt@perf_pmu@rc6-runtime-pm-long: - shard-apl: [FAIL][53] ([fdo#105010]) -> [PASS][54] [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-apl5/igt@perf_pmu@rc6-runtime-pm-long.html [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-apl1/igt@perf_pmu@rc6-runtime-pm-long.html - shard-glk: [FAIL][55] ([fdo#105010]) -> [PASS][56] [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-glk7/igt@perf_pmu@rc6-runtime-pm-long.html [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-glk9/igt@perf_pmu@rc6-runtime-pm-long.html * igt@prime_vgem@fence-wait-bsd2: - shard-iclb: [SKIP][57] ([fdo#109276]) -> [PASS][58] +14 similar issues [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb5/igt@prime_vgem@fence-wait-bsd2.html [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb4/igt@prime_vgem@fence-wait-bsd2.html #### Warnings #### * igt@gem_mocs_settings@mocs-reset-bsd2: - shard-iclb: [SKIP][59] ([fdo#109276]) -> [FAIL][60] ([fdo#111330]) +1 similar issue [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-iclb3/igt@gem_mocs_settings@mocs-reset-bsd2.html [60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-iclb4/igt@gem_mocs_settings@mocs-reset-bsd2.html * igt@kms_content_protection@atomic-dpms: - shard-apl: [FAIL][61] ([fdo#110321] / [fdo#110336]) -> [INCOMPLETE][62] ([fdo#103927]) [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6683/shard-apl5/igt@kms_content_protection@atomic-dpms.html [62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/shard-apl2/igt@kms_content_protection@atomic-dpms.html [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060 [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166 [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540 [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665 [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927 [fdo#105010]: https://bugs.freedesktop.org/show_bug.cgi?id=105010 [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713 [fdo#108303]: https://bugs.freedesktop.org/show_bug.cgi?id=108303 [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341 [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276 [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441 [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642 [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321 [fdo#110336]: https://bugs.freedesktop.org/show_bug.cgi?id=110336 [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841 [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854 [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068 [fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325 [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330 Participating hosts (10 -> 6) ------------------------------ Missing (4): pig-skl-6260u shard-skl pig-hsw-4770r pig-glk-j5005 Build changes ------------- * CI: CI-20190529 -> None * IGT: IGT_5127 -> IGTPW_3332 * Piglit: piglit_4509 -> None CI-20190529: 20190529 CI_DRM_6683: cc4894120704c613b5d343d3e72c11384f780f2a @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_3332: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/ IGT_5127: f43f5fa12ac1b93febfe3eeb9e9985f5f3e2eff0 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3332/ _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-08-22 14:36 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-12 8:47 [igt-dev] [PATCH i-g-t v4 0/2] Add device selection methods Zbigniew Kempczyński 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API Zbigniew Kempczyński 2019-08-21 8:09 ` Arkadiusz Hiler 2019-08-21 11:01 ` Zbigniew Kempczyński 2019-08-21 11:04 ` Katarzyna Dec 2019-08-21 11:58 ` Katarzyna Dec 2019-08-22 7:23 ` Zbigniew Kempczyński 2019-08-12 8:47 ` [igt-dev] [PATCH i-g-t v4 2/2] Add multi-device selection for IGT Zbigniew Kempczyński 2019-08-21 7:56 ` Arkadiusz Hiler 2019-08-21 10:42 ` Zbigniew Kempczyński 2019-08-21 12:08 ` Katarzyna Dec 2019-08-12 10:52 ` [igt-dev] ✓ Fi.CI.BAT: success for Add device selection methods (rev4) Patchwork 2019-08-12 13:29 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox