[igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Thu Aug 22 07:23:02 UTC 2019
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
More information about the igt-dev
mailing list