[PATCH i-g-t v1] drmtest: drop legacy path from filtered opens

[Janusz Krzysztofik]

Hi Kamil,

On Tuesday, 13 February 2024 18:57:22 CET Kamil Konieczny wrote:
> Function __drm_open_driver_another() has two paths, first being
> for filtered devices, second for non-filtered ones. Separate
> them and do not fallback into legacy path when index for card
> exceeds number of filtered devices. Use local opened fds table
> only for legacy calls as this table can become stale after
> driver reload. In filtered one do not use opened fds guards as
> this is pointless, we already have info about card wanted by user
> and we do not need to check if we ancounter already opened one.
> Also allow opening the same device if user wants it, for example
> with repeating same card number in --device or IGT_DEVICE:
> 
> sudo IGT_DEVICE=pci:vendor=Intel,card=0\;pci:vendor=Intel,card=0 \
> 	build/tests/gem_basic --r multigpu-create-close

While the code below looks good to me, I'm missing an explicit why part from 
your commit description, not to be deduced from the what part.  Potential 
candidates:
- _opened_fds table can become stale after driver reload, we need to fix it,
- users need the ability to test their multi-gpu code on single-gpu 
  configurations (e.g., because of limited availability of multi-gpu hardware 
  for testing),
- to avoid confusion, a (sub)set of cards used by a multi-gpu tests should be 
  predictable: if the test may exercise all available cards in any order then 
  no device filters are needed, otherwise user provided device filters must 
  enumerate every card the test is going to exercise and in correct order.

Thanks,
Janusz

> 
> Cc: Arkadiusz Hiler <arek at hiler.eu>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> Cc: Petri Latvala <adrinael at adrinael.net>
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> ---
>  lib/drmtest.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 52b5a2020..307c44ef6 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -481,13 +481,12 @@ static bool __get_card_for_nth_filter(int idx, struct 
igt_device_card *card)
>   *
>   * This function opens device in the following order:
>   *
> - * 1. when --device arguments are present:
> + * 1. when --device arguments are present or IGT_DEVICE env var is used:
>   *   * device scanning is executed,
>   *   * idx-th filter (starting with 0, filters are semicolon separated) is 
used
> - *   * if there is no idx-th filter, goto 2
> + *   * if there is no idx-th filter fail with -1
>   *   * first device maching the filter is selected
> - *   * if it's already opened (for indexes = 0..idx-1) we fail with -1
> - *   * otherwise open the device and return the fd
> + *   * open the device and return the fd
>   *
>   * 2. compatibility mode - open the first DRM device we can find that is 
not
>   *    already opened for indexes 0..idx-1, searching up to 16 device nodes
> @@ -528,10 +527,17 @@ int __drm_open_driver_another(int idx, int chipset)
>  {
>  	int fd = -1;
>  
> -	if (chipset != DRIVER_VGEM && igt_device_filter_count() > idx) {
> +	if (chipset != DRIVER_VGEM && igt_device_filter_count()) {
> +		int count = igt_device_filter_count();
>  		struct igt_device_card card;
>  		bool found;
>  
> +		if (idx < 0 || idx >= count) {
> +			igt_warn("Cannot open, index into filter out of 
bounds: %d, count=%d\n",
> +				 idx, count);
> +			return -1;
> +		}
> +
>  		found = __get_card_for_nth_filter(idx, &card);
>  
>  		if (!found) {
> @@ -542,19 +548,17 @@ int __drm_open_driver_another(int idx, int chipset)
>  		if (!found || !strlen(card.card))
>  			igt_warn("No card matches the filter! [%s]\n",
>  				 igt_device_filter_get(idx));
> -		else if (_is_already_opened(card.card, idx))
> -			igt_warn("card maching filter %d is already 
opened\n", idx);
>  		else
>  			fd = __open_driver_exact(card.card, chipset);
>  
>  	} else {
>  		/* no filter for device idx, let's open whatever is 
available */
>  		fd = __open_driver("/dev/dri/card", 0, chipset, idx);
> +		if (fd >= 0)
> +			_set_opened_fd(idx, fd);
>  	}
>  
>  	if (fd >= 0) {
> -		_set_opened_fd(idx, fd);
> -
>  		/* Cache xe_device struct. */
>  		if (is_xe_device(fd))
>  			xe_device_get(fd);
>