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

[Bommu, Krishnaiah]

> -----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);
+
+               if (save)
+                       return i;
        }
        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
> >