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

[Arkadiusz Hiler]

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 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>
> ---
>  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