[igt-dev] [PATCH i-g-t v7 1/3] Introduce device selection API
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Thu Sep 12 10:59:46 UTC 2019
On Wed, Sep 11, 2019 at 06:00:49AM +0200, Zbigniew Kempczyński wrote:
> Change adds device selection API based on scanning drm subsystem
> using udev library.
>
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: Petri Latvala <petri.latvala at intel.com>
> ---
> .../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 +
> lib/Makefile.sources | 2 +
> lib/igt_device_scan.c | 1394 +++++++++++++++++
> lib/igt_device_scan.h | 74 +
> lib/meson.build | 1 +
> 5 files changed, 1472 insertions(+)
> create mode 100644 lib/igt_device_scan.c
> create mode 100644 lib/igt_device_scan.h
>
> 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 cf094ab8..a9e9557a 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..7a0c0e50
> --- /dev/null
> +++ b/lib/igt_device_scan.c
> @@ -0,0 +1,1394 @@
> +/*
> + * 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: Device scanning and selection
> + * @title: 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 allow 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 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.
> + *
> + * 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.
> + */
> +
> +#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 */
> + const 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
use /* */
or even better, name them "all" and "filtered"
> + bool devs_scanned;
> +};
> +
> +/* Scanned devices holder */
> +static struct igt_devices igt_devs;
> +
> +static const struct vendor_spec v_intel = {
> + .vendor = "Intel",
> + .chipset = DRIVER_INTEL
> +};
> +
> +static const struct vendor_spec v_amd = {
> + .vendor = "AMD",
> + .chipset = DRIVER_AMDGPU
> +};
> +
> +static const struct vendor_spec v_vgem = {
> + .vendor = "Virtual-GEM",
> + .chipset = DRIVER_VGEM
> +};
> +
> +static const 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.
> + */
> +const 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, const 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)
> +{
> + const 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));
> + }
> +}
The only part of the whole codebase that seems to really benefit from
glib are the hashtables for device pops and attrs. Everything else can
just easily be written in plain C + [igt_list].
The amount of glib code is the main reason why it is so hard to get
anyone to review this. Most people working on IGT/kernel are not used to
work with it and it increases cognitive strain significantly. I have
bounced off myself few times on the more intense glib action.
I believe you have been warned about this ahead of time.
[igt_list]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/lib/igt_list.h
> +
> +static struct name_value filter_definition_list[];
> +static void populate_gpu_data(void)
> +{
> + if (gpu_vendor_ht)
> + return;
> + 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;
> +
> + if (!view->len) {
> + printf("No GPU devices found\n");
> + 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;
> +
> + if (!view->len) {
> + printf("No GPU devices found\n");
> + 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 *view);
> +} 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 can be used by external tool to print device array in simple
> + * or detailed form. This function is added here 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)
> +{
> + const 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 {
this is a bit more than a filter function, filter_definiton?
> + 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 || !spec)
> + return false;
> +
> + memset(spec, 0, sizeof(*spec));
> +
> + 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;
> + }
> + }
this looks suspiciously similar to igt_device_find and probably can be
based on it
> +
> + 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 const struct filter_func f_drm = {
> + .filter_function = filter_drm,
> + .help = "drm:/dev/dri/* path",
> + .detail = "find drm device by /dev/dri/* node\n",
> +};
> +
> +static const 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 const 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 const 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 const 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",
> +};
> +
> +static 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;
> +}
> +
> +/* Check does filter is valid. It checks:
> + * 1. /sys/... path first
> + * 2. filter name from filter definition
> + */
> +static bool is_filter_valid(const char *filter)
> +{
> + struct filter_spec fspec;
> + struct filter_func *ffunc;
> + int ret;
> +
> + populate_gpu_data();
> +
> + ret = get_filter_spec(filter, &fspec);
> + if (!ret)
> + return false;
> +
> + /* Check if /sys path is valid */
> + if (!strncmp(fspec.data, "/sys", 4)) {
> + struct stat st;
> + if (stat(fspec.data, &st)) {
> + igt_warn("is_filter_valid: syspath [%s], err: %s\n",
> + fspec.data, strerror(errno));
> + return false;
> + } else {
> + return true;
> + }
> + }
> +
> + ffunc = g_hash_table_lookup(filter_definition_ht, fspec.name);
> + if (!ffunc) {
> + igt_warn("No filter with name [%s]\n", fspec.name);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/**
> + * 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();
> + populate_gpu_data();
> +
> + strv = g_strsplit(filter, ";", -1);
> +
> + s = strv;
> + while (*s) {
> + bool is_valid = is_filter_valid(*s);
> + igt_warn_on(!is_valid);
> + if (is_valid)
> + g_ptr_array_add(device_filters, strdup(*s));
> + s++;
> + }
> + g_strfreev(strv);
> +
> + return c;
> +}
> +
> +/**
> + * igt_device_filter_free_all
> + *
> + * Free all filters within array.
> + */
> +void igt_device_filter_free_all(void)
> +{
> + if (!device_filters)
> + return;
> +
> + g_ptr_array_set_free_func(device_filters, free);
> + g_ptr_array_unref(device_filters);
> + device_filters = NULL;
> +}
> +
> +/**
> + * 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;
> +
> + 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);
I don't like that we have special case for *:/sys here and in
is_filter_valid().
How about instead of using hash table for matching fspec.name doing
something like that:
extend filter_definition aka (filter_func) with:
bool (*is_the_right_filter)(struct filter_spec *fspec);
char *name;
bool (*is_valid)(struct filter_spec *fspec, struct filter_definition *fdef);
Instead of hashtable you can keep them in array (order matters), and:
is_the_right_filter for sys would be:
return !strncmp(fspec.data, "/sys", 4))
and the a single function that matches name for the others
filter_definition *get_filter_for_spec(*fspec) {
for (def in filter_definitions) {
if (def->is_the_right_filter && def->is_the_right_filter(fspec))
return def;
}
return NULL;
}
And then:
bool is_filter_valid(*fspec) {
struct filter_defintion def = NULL;
def = get_filter_for_spec(fspec);
if (def != NULL && def->is_valid != NULL)
return def->is_valid(fspec);
retrun false;
}
this limits the need for glib magic (ht, gpointers) and makes the magic
checks more localized to the filter definition.
> + 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) do { \
> + if ((dst) && (src)) \
> + strncpy((dst), (src), (size)); \
> + } while (0)
I would go with if (src != NULL) for the three uses down there (we
already make sure that (card != NULL), so dst is ok).
> +/**
> + * 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);
A better option would be to use sizeof(card->subsystem), etc.
Cheers,
Arek
More information about the igt-dev
mailing list