[PATCH i-g-t v6 3/8] lib/drmtest: allow opening cards out of order

[Janusz Krzysztofik]

On Thursday, 1 February 2024 16:50:42 CET Kamil Konieczny wrote:
> Current __drm_open_driver_another() implementation prevents
> opening cards out of order, for example card 3 without previous
> opens of 0..2. This can be seen with three GPU cards system and
> gem_open_basic test:
> 
> sudo IGT_DEVICE=pci:vendor=Intel,card=all build/tests/gem_basic --r multigpu-create-close
> 
> IGT-Version: 1.28-g2548a539e (x86_64) (Linux: 6.7.0-rc5 x86_64)
> Using IGT_SRANDOM=1706630533 for randomisation
> Opened device: /dev/dri/card0
> Starting subtest: multigpu-create-close
> <g:0> Testing creating and closing an object.
> <g:1> Opened device: /dev/dri/card1
> <g:1> Testing creating and closing an object.
> gem_basic: ../lib/drmtest.c:341: _is_already_opened: Assertion `as_idx <= _opened_fds_count' failed.
> Received signal SIGABRT.
> 
> Relax that condition. Also relax condition for filtered devices and

Can we have those two parts split into two patches, please?  If not then could 
you please explain why (how they depend on each other)?

Thanks,
Janusz


> allow them to open already opened ones, basically it will allow
> running multi-GPU scenarios on single-GPU system with filter:
> 
> IGT_DEVICE=pci:vendor=Intel,card=0\;pci:vendor=Intel,card=0
> 
> Changes in __search_and_open() was suggested by Zbigniew.
> 
> v5:
>  removed caching for names of opened cards
>  relaxed opening of filtered devices
> v6:
>  fixed assingment in _is_already_open() (Janusz)
>  simplify use of this function
> 
> Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> ---
>  lib/drmtest.c | 66 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 73d9159af..5bb084508 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -322,35 +322,54 @@ static int _opened_fds_count;
>  static void _set_opened_fd(int idx, int fd)
>  {
>  	assert(idx < ARRAY_SIZE(_opened_fds));
> -	assert(idx <= _opened_fds_count);
> +	assert(idx >= 0);
>  
>  	_opened_fds[idx].fd = fd;
>  
>  	assert(fstat(fd, &_opened_fds[idx].stat) == 0);
>  
> -	_opened_fds_count = idx+1;
> +	for (int n = _opened_fds_count; n < idx; n++)
> +		_opened_fds[n].fd = -1;
> +
> +	if (idx >= _opened_fds_count)
> +		_opened_fds_count = idx + 1;
>  }
>  
> -static bool _is_already_opened(const char *path, int as_idx)
> +/* in *err returns -errno or index to _opened_fds[] */
> +static bool _is_already_opened(const char *path, int as_idx, int *err)
>  {
>  	struct stat new;
>  
>  	assert(as_idx < ARRAY_SIZE(_opened_fds));
> -	assert(as_idx <= _opened_fds_count);
> +	assert(as_idx >= 0);
>  
>  	/*
>  	 * we cannot even stat the device, so it's of no use - let's claim it's
>  	 * already opened
>  	 */
> -	if (igt_debug_on(stat(path, &new) != 0))
> +	if (igt_debug_on(stat(path, &new) != 0)) {
> +		if (err)
> +			*err = -errno;
> +
>  		return true;
> +	}
> +
> +	if (err)
> +		*err = 0;
> +
> +	for (int i = 0, end = min(_opened_fds_count, as_idx); i < end; ++i) {
> +		if (_opened_fds[i].fd == -1)
> +			continue;
>  
> -	for (int i = 0; i < as_idx; ++i) {
>  		/* did we cross filesystem boundary? */
>  		assert(_opened_fds[i].stat.st_dev == new.st_dev);
>  
> -		if (_opened_fds[i].stat.st_ino == new.st_ino)
> +		if (_opened_fds[i].stat.st_ino == new.st_ino) {
> +			if (err)
> +				*err = i;
> +
>  			return true;
> +		}
>  	}
>  
>  	return false;
> @@ -359,23 +378,38 @@ static bool _is_already_opened(const char *path, int as_idx)
>  static int __search_and_open(const char *base, int offset, unsigned int chipset, int as_idx)
>  {
>  	const char *forced;
> +	int err;
>  
>  	forced = forced_driver();
>  	if (forced)
>  		igt_debug("Force option used: Using driver %s\n", forced);
>  
> -	for (int i = 0; i < 16; i++) {
> +	for (int i = 0, idx = -1; i < 16 && idx < as_idx; i++) {
>  		char name[80];
>  		int fd;
>  
>  		sprintf(name, "%s%u", base, i + offset);
>  
> -		if (_is_already_opened(name, as_idx))
> -			continue;
> +		if (_is_already_opened(name, as_idx, &err)) {
> +			if (err < 0)
> +				continue;
> +
> +			if (idx + 1 < as_idx) {
> +				++idx;
> +				continue;
> +			}
> +		}
>  
>  		fd = __drm_open_device(name, chipset);
> -		if (fd != -1)
> +		if (fd != -1) {
> +			++idx;
> +			if (idx < as_idx) {
> +				close(fd);
> +				continue;
> +			}
> +
>  			return fd;
> +		}
>  	}
>  
>  	return -1;
> @@ -486,8 +520,7 @@ static bool __get_card_for_nth_filter(int idx, struct igt_device_card *card)
>   *   * 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
> @@ -542,11 +575,10 @@ 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
> +		else {
> +			_is_already_opened(card.card, idx, NULL); /* check only filesystem boundary */
>  			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);
>