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

[Kamil Konieczny]

Hi igt-dev,
On 2024-02-19 at 10:22:25 +0100, Janusz Krzysztofik wrote:
> Hi Kamil,
> 
> On Friday, 16 February 2024 20:05:34 CET 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;
> > +		}
> 
> I'm wondering why we need this check now while it was not here before.  Is 
> that really related to dropping the call to _is_already_opened() below?  If 
> idx < 0 then in any case that seems to be a bug on the caller side.
> 
> I think the most clean approach would be to declare the idx argument as 
> unsigned int.  If not then I would place this check at the top of the function 
> body, above the if block, and use something more strong than igt_debug() 
> (igt_fail() if we may expect users to be prepared for long jumps, igt_warn() 
> otherwise).
> 
> How about moving that to a separate patch?  If not than maybe a "while being 
> at it" note in commit description would be sufficient.
> 
> Thanks,
> Janusz
> 

You are right, I will remove it - for negative idx it will enter
filtered path and there filter with return NULL - so no need to
check for it.

Regards,
Kamil

> > +
> >  		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);
> >  
> > 
> 
> 
> 
>