[igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Wed Aug 21 08:09:11 UTC 2019
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 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 | 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
More information about the igt-dev
mailing list