[PATCH] tests/core_setmaster: Handle the test even if cards are non-continuous

[Kamil Konieczny]

Hi Bommu,,
On 2024-09-03 at 08:32:00 +0000, Bommu, Krishnaiah wrote:
> 
> 
> > -----Original Message-----
> > From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > Sent: Thursday, August 22, 2024 10:16 PM
> > To: igt-dev at lists.freedesktop.org
> > Cc: Bommu, Krishnaiah <krishnaiah.bommu at intel.com>; Emil Velikov
> > <emil.l.velikov at gmail.com>; Ghimiray, Himal Prasad
> > <himal.prasad.ghimiray at intel.com>
> > Subject: Re: [PATCH] tests/core_setmaster: Handle the test even if cards are
> > non-continuous
> > 
> > Hi Bommu,
> > On 2024-08-12 at 10:54:43 +0530, Bommu Krishnaiah wrote:
> > > Although most of userspace assumes drm cards to be populated
> > > continously,
> > >
> > 
> > Remove blank line.
> I will remove 
> > 
> > > this assumption may not be always true. After hot unpluging and
> > > repluging of card
> > >
> > 
> > Remove blank.
> I will remove
> > 
> > > the card may be non-continous.  To address such scenarios where the
> > > cards can be non-continous
> > >
> > 
> > Remove blank.
> I will remove
> > 
> > > modify the tweak_perm function to loop all possible
> > > cardnumbers(card0-card255) to confirm are they populated or not.
> > -----^
> > s/cardnumbers/card numbers/
> I will changes 
> > 
> > imho test is checking only first card so looping all is not optimal.
> > 
> > >
> > > Problem Description:
> > > The test fails after running the `core_hotunplug at hot*` subtest, where
> > > card0 is missing.
> Here card0 is populated as card1
> > 
> > imho this could be discussed if that is ok or driver error, imho valid case would
> > be to have multi-card and unpluging first.
> > 
> > > This leads to a failure in the subsequent `master-drop-set-user` test.
> > >
> > > Command sequence for reproducing the issue:
> > > - Check available cards: `ls /dev/dri/`
> > > 	- Output: by-path  card0  renderD128
> > > - Run the test: `# ./core_hotunplug --r hotrebind-lateclose`
> > > - Check again: ‘ls /dev/dri/’
> > > 	- Output after failure: by-path  card1  renderD129
> > 
> > s/failure/core_hotunplug failure/
> I will change 
> > 
> > > - Run the test: `# ./core_setmaster --r master-drop-set-user`
> > >
> > > This failures on both XE and i915 for above sequence.
> > >
> > > Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
> > > Cc: Emil Velikov <emil.l.velikov at gmail.com>
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> > > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > > ---
> > >  tests/core_setmaster.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c index
> > > 9c2083f66..a2758e2f2 100644
> > > --- a/tests/core_setmaster.c
> > > +++ b/tests/core_setmaster.c
> > > @@ -116,9 +116,10 @@ static unsigned tweak_perm(uint8_t *saved_perm,
> > unsigned max_perm, bool save)
> > >  	for (i = 0; i < max_perm; i++) {
> > >  		snprintf(path, sizeof(path), "/dev/dri/card%u", i);
> > >
> > > -		/* Existing userspace assumes there's no gaps, do the same. */
> > > +		/* Userspace cannot always have continuous card0...N due to
> > > +		 * dynamic unloading or removal of cards */
> > >  		if (stat(path, &st) != 0)
> > > -			break;
> > > +			continue;
> > 
> > imho this is ok but you should also break after first save/restore just before end
> > of for loop, so no need for setting all perms when you will test only first one.
> > 
> > Also imho you should fill info in saved_perm about which card was found and
> > use it instead of calling __drm_open_driver().
> > 
> 
> kbommu at kbommu-desk:~/xe_public/igt-gpu-tools$ git diff
> diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c
> index a2758e2f2..b0bf4a5d3 100644
> --- a/tests/core_setmaster.c
> +++ b/tests/core_setmaster.c
> @@ -138,6 +138,9 @@ static unsigned tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
>                  * failure may be irrelevant.
>                  */
>                 chmod(path, st.st_mode);

Just noticed a problem here, return code for chmod() is not checked...

> +
> +               if (save)
> +                       return i;

It should also apply for restoring, not only setting.
Imho also save an index at which you found a card for testing
so you could restore it later and also use it for opening for
testing.

Regards,
Kamil

>         }
>         return i;
>  }
> kbommu at kbommu-desk:~/xe_public/igt-gpu-tools$
> 
> this is the changes your excepting right ?
> 
> Regards,
> Krishna.
> 
> > Regards,
> > Kamil
> > 
> > >
> > >  		if (save) {
> > >  			/* Save and toggle */
> > > --
> > > 2.25.1
> > >