[igt-dev] [PATCH i-g-t v4 1/2] Introduce multi-device selection API
Zbigniew Kempczyński
zbigniew.kempczynski at intel.com
Wed Aug 21 11:01:01 UTC 2019
On Wed, Aug 21, 2019 at 11:09:11AM +0300, Arkadiusz Hiler wrote:
> On Mon, Aug 12, 2019 at 10:47:17AM +0200, Zbigniew Kempczyński wrote:
> > Change adds device selection based on scanning drm subsystem
> > using udev library.
> >
> > Tool 'lsgpu' which uses device scanning and selection feature was added.
> >
<cut>
> > +
> > +/**
> > + * SECTION:igt_device_scan
> > + * @short_description: Multi-device scanning and selection
>
> Multi-device selection is not wrong but the initial interpretation (in my
> case) was "selecting multiple drm devices at the same time".
>
> I think it would work better without "multi", just "device scanning and
> selection".
>
> Any reason why we have this as a separate section in documentation? I
> think we can merge this with [igt_device] - it has just 3 functions and
> virtually no documentation of it's own.
>
> [igt_device]: https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-igt-device.html
I agree regarding 'multi' word. I've chosen another file (not igt_device.c)
because technically it is related to scanning udev, not access to device
itself. I assumed that igt_device code is related to doing operations
on drm device fd.
> > + * For tests which opens more than one device device filter collection API
> > + * can be used. You can add filter to the array using igt_device_filter_add(),
> > + * get nth filter stored using igt_device_filter_get() and return
> > + * #igt_device_card using filter in igt_device_card_match().
>
> I would reword that slightly:
>
> * For tests which opens more than one device device filter collection API
> * can be used. You can add a filter to the array using igt_device_filter_add(),
> * get the nth filter stored using igt_device_filter_get() and get
> * #igt_device_card using igt_device_card_match() with that filter.
>
> But that aside - extending the point from the review of patch 2:
>
> To handle multiple devices in a single test you would have to:
> * check existing filters
> * add any missing ones
> * open all the cards using 1,2...n filters
>
> Which still feels awkward and quite choreful. I don't think that
> managing filters from inside a test is the greatest idea. Library
> should hide that.
>
> If you have use cases for this thing hit me with some pseudocode, I would
> like to understand.
Library helpers can be implemented in drmtest.c/h and in my opinion
there's appropriate place for that. Device selection adds a code you
can use for that.
<cut>
> > + * Both selects first VC4 device. At the moment Rasperry PI has only
> > + * one gpu, but filter is ready if there will be more.
> > + *
> > + * # lsgpu
> > + *
> > + * Scanned devices can be displayed using 'lsgpu' tool. Tool also displays
> > + * properties and sysattrs (-D switch, means detail) which can be used during
> > + * filter implementation.
>
> "The devices can be scanned and displayed"
>
> "The tool also displays"
>
> "-D switch, for details"
>
> Filter implementation? I am not sure but I think this paragraph should
> read "which can be helpful when writing filters".
Ok.
>
> > + *
> > + * Tool can also be used to check does the implemented filter works as expected.
> > + * To select device using filter use '-d' or '--device' argument like:
>
> "lsgpu can also be used to try out filters"
Ok.
>
> > + *
> > + * |[<!-- language="plain" -->
> > + * ./lsgpu -d 'pci:vendor=Intel'
> > + * === Device filter list ===
> > + * [ 0]: pci:vendor=Intel
> > +
> > + * === Testing device open ===
> > + * subsystem : pci
> > + * chipset : 1
> > + * drm card : /dev/dri/card0
> > + * drm render : /dev/dri/renderD128
> > + * Device /dev/dri/card0 successfully opened
> > + * Device /dev/dri/renderD128 successfully opened
> > + * ]|
> > + *
> > + * Apart of selecting device lsgpu try to open card and render nodes.
>
> I think this could use some context on why, e.g.
> "Additionally lsgpu tries to open the card and render nodes to verify
> permissions..."
Ok.
>
> > +*/
> > +
> > +//#define DEBUG_DEVICE_SCAN
>
> leftover define
I'll remove that.
>
> > +#ifdef DEBUG_DEVICE_SCAN
> > +#define DBG(...) \
> > +{ \
> > + struct timeval tm; \
> > + gettimeofday(&tm, NULL); \
> > + printf("%10ld.%03ld: ", tm.tv_sec, tm.tv_usec); \
> > + printf(__VA_ARGS__); \
> > + }
> > +
> > +#else
> > +#define DBG(...) {}
> > +#endif
> > +
> > +#define strequal(x, y) ((x) && (y) && !strcmp((x), (y)))
> > +#define IGT_DRM_PATH "/dev/dri"
> > +
> > +static GHashTable *sysattrs_blacklist_ht; //sysattrs we don't want to read
> > +static GHashTable *gpu_vendor_ht; //search id -> vendor_spec mapping
> > +static GHashTable *filter_definition_ht; //supported filters (pci=..., etc.)
>
> /* we are using plain old C-style comments, even at the end of the line */
This is not true at whole project but ok, I'll change that.
>
> <SNIP>
>
> I'll review the actual code in more detail once I have enough caffeine in my
> bloodstream to handle glib :-)
>
> > diff --git a/tools/lsgpu.c b/tools/lsgpu.c
> > new file mode 100644
> > index 00000000..e723f0b2
> > --- /dev/null
> > +++ b/tools/lsgpu.c
> > @@ -0,0 +1,183 @@
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + */
> > +
> > +#include "igt_device_scan.h"
> > +#include "igt.h"
> > +#include <sys/ioctl.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <signal.h>
> > +
> > +enum {
> > + OPT_PRINT_DETAIL = 'D',
> > + OPT_LIST_VENDORS = 'v',
> > + OPT_LIST_FILTERS = 'l',
> > + OPT_DEVICE = 'd',
> > + OPT_HELP = 'h'
> > +};
> > +
> > +bool g_show_vendors;
> > +bool g_list_filters;
> > +bool g_device;
> > +bool g_help;
> > +
> > +static const char *usage_str =
> > + "usage: lsgpu [options]\n\n"
> > + "Options:\n"
> > + " -D, --print-details Print devices with details\n"
> > + " -v, --list-vendors List recognized vendors\n"
> > + " -l, --list-filter-types List registered device filters types\n"
> > + " -d, --device filter Device filter, can be given multiple times\n"
> > + " -h, --help Show this help message and exit\n";
> > +
> > +static void test_device_open(struct igt_device_card *card)
> > +{
> > + int fd;
> > +
> > + if (!card)
> > + return;
> > +
> > + fd = drm_open_card(card);
> > + if (fd >= 0) {
> > + printf("Device %s successfully opened\n", card->card);
> > + close(fd);
> > + } else {
> > + if (strlen(card->card))
> > + printf("Cannot open card %s device\n", card->card);
> > + else
> > + printf("Cannot open card device, empty name\n");
> > + }
> > +
> > + fd = drm_open_render(card);
> > + if (fd >= 0) {
> > + printf("Device %s successfully opened\n", card->render);
> > + close(fd);
> > + } else {
> > + if (strlen(card->render))
> > + printf("Cannot open render %s device\n", card->render);
> > + else
> > + printf("Cannot open render device, empty name\n");
> > + }
> > +}
> > +
> > +static void print_card(struct igt_device_card *card)
> > +{
> > + if (!card)
> > + return;
> > +
> > + printf("subsystem : %s\n", card->subsystem);
> > + printf("chipset : %x\n", card->chipset);
> > + printf("drm card : %s\n", card->card);
> > + printf("drm render : %s\n", card->render);
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + static struct option long_options[] = {
> > + {"print-detail", no_argument, NULL, OPT_PRINT_DETAIL},
> > + {"list-vendors", no_argument, NULL, OPT_LIST_VENDORS},
> > + {"list-filter-types", no_argument, NULL, OPT_LIST_FILTERS},
> > + {"device", required_argument, NULL, OPT_DEVICE},
> > + {"help", no_argument, NULL, OPT_HELP},
> > + {0, 0, 0, 0}
> > + };
> > + int c, index = 0;
> > + const char *env;
> > + enum igt_devices_print_type printtype = IGT_PRINT_SIMPLE;
> > +
> > + while ((c = getopt_long(argc, argv, "Dvld:h",
> > + long_options, &index)) != -1) {
> > + switch(c) {
> > +
> > + case OPT_PRINT_DETAIL:
> > + printtype = IGT_PRINT_DETAIL;
> > + break;
> > + case OPT_LIST_VENDORS:
> > + g_show_vendors = true;
> > + break;
> > + case OPT_LIST_FILTERS:
> > + g_list_filters = true;
> > + break;
> > + case OPT_DEVICE:
> > + g_device = true;
> > + igt_device_filter_add(optarg);
> > + break;
> > + case OPT_HELP:
> > + g_help = true;
> > + break;
> > + }
> > + }
> > +
> > + if (g_help) {
> > + printf("%s\n", usage_str);
> > + exit(0);
> > + }
> > +
> > + env = getenv("IGT_DEVICE");
> > + if (env) {
> > + igt_device_filter_add(env);
> > + g_device = true;
> > + }
> > +
> > + if (g_show_vendors) {
> > + igt_devices_print_vendors();
> > + return 0;
> > + }
> > +
> > + if (g_list_filters) {
> > + igt_device_print_filter_types();
> > + return 0;
> > + }
> > +
> > + igt_devices_scan(false);
> > +
> > + if (g_device) {
> > + int n = igt_device_filter_count();
> > + printf("=== Device filter list ===\n");
> > + for (int i = 0; i < n; i++) {
> > + printf("[%2d]: %s\n", i,
> > + igt_device_filter_get(i));
> > + }
> > + printf("\n");
> > +
> > + printf("=== Testing device open ===\n");
> > + for (int i = 0; i < n; i++) {
> > + struct igt_device_card card;
> > + const char *filter = igt_device_filter_get(i);
> > +
> > + if (!igt_device_card_match(filter, &card))
> > + continue;
> > + print_card(&card);
> > + test_device_open(&card);
> > + printf("---\n");
> > + }
> > +
> > + return 0;
> > + }
> > +
> > + igt_devices_print(printtype);
> > +
> > + return 0;
> > +}
> > diff --git a/tools/meson.build b/tools/meson.build
> > index 6e72b263..9b3a2a69 100644
> > --- a/tools/meson.build
> > +++ b/tools/meson.build
> > @@ -36,6 +36,7 @@ tools_progs = [
> > 'intel_gem_info',
> > 'intel_gvtg_test',
> > 'dpcd_reg',
> > + 'lsgpu',
> > ]
> > tool_deps = igt_deps
>
> Oh, I already like --device and lsgpu :-)
>
> Finally I'll be able to run kms_ tests on i915 on my personal rig, which
> also has a discrete card. i915 usually gets enumerated second for me...
>
> Thanks for doing this!
>
> --
> Cheers,
> Arek
I'll push v5 soon.
Zbigniew
More information about the igt-dev
mailing list