[PATCH] tests/core_setmaster: Handle the test even if cards are non-continuous
Bommu, Krishnaiah
krishnaiah.bommu at intel.com
Wed Sep 4 06:47:40 UTC 2024
> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> Sent: Tuesday, September 3, 2024 5:34 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-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...
root at kbommu-desk:/home/kbommu/xe_public/igt-gpu-tools# git diff
diff --git a/tests/core_setmaster.c b/tests/core_setmaster.c
index a2758e2f2..3e506b5bf 100644
--- a/tests/core_setmaster.c
+++ b/tests/core_setmaster.c
@@ -107,13 +107,15 @@ static void check_drop_set(void)
drm_close_driver(master);
}
-static unsigned tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
+static int tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
{
char path[256];
struct stat st;
- unsigned i;
+ int i;
for (i = 0; i < max_perm; i++) {
+ int ret;
+
snprintf(path, sizeof(path), "/dev/dri/card%u", i);
/* Userspace cannot always have continuous card0...N due to
@@ -137,7 +139,12 @@ static unsigned tweak_perm(uint8_t *saved_perm, unsigned max_perm, bool save)
* - __drm_open_driver() can open another device, aka the
* failure may be irrelevant.
*/
- chmod(path, st.st_mode);
+ ret = chmod(path, st.st_mode);
+ if (ret)
+ return ret;
+
+ if (save)
+ return i;
}
return i;
}
@@ -162,7 +169,7 @@ igt_main
igt_subtest_group {
uint8_t saved_perm[255];
- unsigned num;
+ int num;
/* Upon dropping root we end up as random user, which
* a) is not in the video group, and
root at kbommu-desk:/home/kbommu/xe_public/igt-gpu-tools#
this changes your excepting for chmod return
>
> > +
> > + 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.
for restoring it is not required I believe because we are passing the number of cards which we got during the save(i.e I value in for loop in tweak_perm)
Regards,
Krishna.
>
> 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
> > > >
More information about the igt-dev
mailing list