Re: [igt-dev] [EARLY RFC 1/1] Introduce new method of device selection

Arkadiusz Hiler
2019-07-03T07:57:10Z

On Mon, Jul 01, 2019 at 03:03:26PM +0200, Zbigniew Kempczyński wrote:
> Change adds device selection based on implemented filters. Currently
> drm, dev and pci filters were added to address different device
> selection possibilities.
>
> drm filter allows selection /dev/dri/cardN node by using filter
> drm:card=N
> where N is number of card.
>
> dev filter allows selection of N-th device in all gpus avilable
> (PCI devices at the moment). Regardless of driver load time
> and /dev/dri/cardN this filter will select same device until
> new card will be added. Filter syntax is as follows:
> dev:card=N
>
> pci filter gives vendor / device / card number selection. Only
> cards which match this filter will be returned. Filter syntax:
> pci:vendor=xxxx,device=yyyy,card=N
> where xxxx, yyyy are hex numbers which identify vendor and device,
> card selects N-th matching card.

We also use VKMS which is a paltform device, so this is probably the
next filter to get support. Looking at the code it should be pretty easy
to add 'platform:' selector later.

> New device selection uses --device filter or IGT_DEVICE environent
> variable.
>
> Tool 'lsgpu' which uses device scanning feature was added.
>
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Petri Latvala <petri.latvala@intel.com>
> ---
> lib/Makefile.sources | 2 +
> lib/drmtest.c | 53 ++-
> lib/drmtest.h | 1 +
> lib/igt_core.c | 12 +
> lib/igt_core.h | 1 +
> lib/igt_device_scan.c | 936 +++++++++++++++++++++++++++++++++++++++++
> lib/igt_device_scan.h | 64 +++
> lib/meson.build | 1 +
> tools/Makefile.sources | 1 +
> tools/lsgpu.c | 222 ++++++++++
> tools/meson.build | 1 +
> 11 files changed, 1292 insertions(+), 2 deletions(-)
> create mode 100644 lib/igt_device_scan.c
> create mode 100644 lib/igt_device_scan.h
> create mode 100644 tools/lsgpu.c
>
> 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/drmtest.c b/lib/drmtest.c
> index 25f20353..eaffb9b3 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -55,6 +55,7 @@
> #include "igt_gt.h"
> #include "igt_kmod.h"
> #include "igt_sysfs.h"
> +#include "igt_device_scan.h"
> #include "version.h"
> #include "config.h"
> #include "intel_reg.h"
> @@ -296,22 +297,70 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
> return __search_and_open(base, offset, chipset);
> }
>
> +static int __open_driver_exact(const char *name, unsigned int chipset)
> +{
> + static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> + int fd;
> +
> + fd = open_device(name, chipset);
> + if (fd != -1)
> + return fd;
> +
> + pthread_mutex_lock(&mutex);
> + for (const struct module *m = modules; m->module; m++) {
> + if (chipset & m->bit) {
> + if (m->modprobe)
> + m->modprobe(m->module);
> + else
> + modprobe(m->module);
> + }
> + }
> + pthread_mutex_unlock(&mutex);
> +
> + return open_device(name, chipset);
> +}
> +
> +
> /**
> * __drm_open_driver:
> * @chipset: OR'd flags for each chipset to search, eg. #DRIVER_INTEL
> *
> - * Open the first DRM device we can find, searching up to 16 device nodes
> + * Function opens device with following order:
> + * 1. when --device argument is present device scanning will be executed,
> + * then device which matches filter argument will be selected.
> + * 2. compatibility mode - open the first DRM device we can find,
> + * searching up to 16 device nodes.
> *
> * Returns:
> * An open DRM fd or -1 on error
> */
> int __drm_open_driver(int chipset)
> {
> + if (igt_device_arg) {
> + struct igt_device_card card;
> + int match;
> + igt_devices_scan(IGT_SCAN_PCI, false);
> + match = igt_device_card_match(igt_device_arg, &card);
> + if (match < 0 || (card.chipset != chipset && chipset != DRIVER_ANY))
> + return -1;
> + return __open_driver_exact(card.card, chipset);
> + }
> +
> return __open_driver("/dev/dri/card", 0, chipset);
> }
>
> -static int __drm_open_driver_render(int chipset)
> +int __drm_open_driver_render(int chipset)
> {
> + if (igt_device_arg) {
> + struct igt_device_card card;
> + int match;
> + igt_devices_scan(IGT_SCAN_PCI, false);
> + match = igt_device_card_match(igt_device_arg, &card);
> + if (match < 0 || (card.chipset != chipset && chipset != DRIVER_ANY))
> + return -1;
> + return __open_driver_exact(card.render, chipset);
> + }
> +
> return __open_driver("/dev/dri/renderD", 128, chipset);
> }

It would be nice to have have feedback (igt_info?) on a failed selection
for chipset, especially with forced device.

Let's say the code opens() with DRIVER_INTEL but we --device to VKMS.
This will naturally skip on the following igt_require(drm_fd), but
people may have hard time understading the reason at times.

Loging information about the conflict/lack of matches due to filter
would help with understanding the reason for skipping.

Other than that looks pretty solid and I think we can drop the RFC from
title to encourage people to do more thorough reviews :-)

But please keep in mind that this is mid the holiday season, so some
people may come a bit late to the party with their feedback.

--
Cheers,
Arek