[PATCH i-g-t v4] lib/drmtest: drop legacy check from filtered opens

[Zbigniew Kempczyński]

On Fri, Feb 16, 2024 at 08:05:34PM +0100, Kamil Konieczny wrote:
> For some multi-GPU tests user can check them on single-GPU board
> with the help of repeating same card in --device option or
> environment var IGT_DEVICE:
> 
> sudo IGT_DEVICE=pci:vendor=Intel,card=0\;pci:vendor=Intel,card=0 \
> 	build/tests/gem_basic --r multigpu-create-close
> 
> but is unable to use it due to current implementation of device
> opening in function __drm_open_driver_another().
> 
> To allow this drop legacy check for already opened device when
> using filters.
> 
> v2: improving description, using filtered_count var (Janusz)
>   changing drmtest: to lib/drmtest: (Kamil)
>   adding Lukasz Laguna to Cc: (Kamil)
> v3: for out-of-bound index print debug, not a warning (Kamil)
> v4: restored using filters number for filtered path and restored
>   setting opened fds for legacy tests, changed description (Kamil)
> 
> Cc: Arkadiusz Hiler <arek at hiler.eu>
> Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> Cc: Lukasz Laguna <lukasz.laguna at 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>
> Reviewed-by: "Zbigniew Kempczyński" <zbigniew.kempczynski at intel.com>
> ---
>  lib/drmtest.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 52b5a2020..43e6c0158 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
>   *   * 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
> @@ -526,12 +525,19 @@ static bool __get_card_for_nth_filter(int idx, struct igt_device_card *card)
>   */
>  int __drm_open_driver_another(int idx, int chipset)
>  {
> +	int filter_count = igt_device_filter_count();
>  	int fd = -1;
>  
> -	if (chipset != DRIVER_VGEM && igt_device_filter_count() > idx) {
> +	if (chipset != DRIVER_VGEM && idx < filter_count) {
>  		struct igt_device_card card;
>  		bool found;
>  
> +		if (idx < 0) {
> +			igt_debug("Cannot open, index out of bounds: %d, filter_count=%d\n",
> +				 idx, filter_count);
> +			return -1;
> +		}
> +

Instead of above condition I think you should assert on the beginning
of the function:

igt_assert_lte(idx, 0);

Negative idx is invalid and we should catch this early.

--
Zbigniew

>  		found = __get_card_for_nth_filter(idx, &card);
>  
>  		if (!found) {
> @@ -542,8 +548,6 @@ 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);
>  
> -- 
> 2.42.0
>