[PATCH v3 2/2] tests/intel/xe_sysfs_timeslice_duration: Restore preempt timeout

[Cavitt, Jonathan]

-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
Sent: Tuesday, November 5, 2024 9:54 AM
To: igt-dev at lists.freedesktop.org
Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Belgaumkar, Vinay <vinay.belgaumkar at intel.com>
Subject: Re: [PATCH v3 2/2] tests/intel/xe_sysfs_timeslice_duration: Restore preempt timeout
> 
> Hi Jonathan,
> On 2024-11-05 at 15:48:23 +0000, Jonathan Cavitt wrote:
> > The subtests of sysfs_timeslice_duration modify the preempt_timeout_us
> > and timeslice_duration_us values.  However, while the test does restore
> > the timeslice_duration_us value at the end of execution, it does not do
> > the same for preempt_timeout_us.  Because the value is not properly
> > restored, future tests can end up using the unexpected preempt timeout
> > value and thus have unexpected behavior.
> > 
> > Save and restore the preempt_timeout_us value during the test.
> > 
> > This fix does not apply to xe_sysfs_preempt_timeout because only the
> > preempt_timeout_us is modified during those tests, and the value is
> > correcty restored before the tests end.
> > 
> > v2: Also restore preempt_timeout_us on test failure (Kamil)
> > 
> > v3: Abort on restore failure (Kamil)
> > 
> > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2976
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > CC: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> > CC: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > ---
> >  tests/intel/xe_sysfs_timeslice_duration.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/intel/xe_sysfs_timeslice_duration.c b/tests/intel/xe_sysfs_timeslice_duration.c
> > index b34d78a784..ef11acca13 100644
> > --- a/tests/intel/xe_sysfs_timeslice_duration.c
> > +++ b/tests/intel/xe_sysfs_timeslice_duration.c
> > @@ -115,10 +115,11 @@ static uint64_t __test_timeout(int fd, int engine, unsigned int timeout, uint16_
> >  static void test_timeout(int fd, int engine, const char **property, uint16_t class, int gt)
> >  {
> >  	uint64_t delays[] = { 1000, 50000, 100000, 500000 };
> > -	unsigned int saved;
> > +	unsigned int saved, old_pt;
> >  	uint64_t elapsed;
> >  	uint64_t epsilon;
> >  
> > +	igt_assert(igt_sysfs_scanf(engine, "preempt_timeout_us", "%u", &old_pt) == 1);
> >  	igt_require(igt_sysfs_printf(engine, "preempt_timeout_us", "%u", 1) == 1);
> >  	igt_assert(igt_sysfs_scanf(engine, property[0], "%u", &saved) == 1);
> >  	igt_debug("Initial %s:%u\n", property[0], saved);
> > @@ -140,6 +141,9 @@ static void test_timeout(int fd, int engine, const char **property, uint16_t cla
> >  	}
> >  
> >  	set_timeslice_duration(engine, saved);
> > +	igt_assert_lte(0, igt_sysfs_printf(engine, "preempt_timeout_us", "%u", old_pt));
> > +	igt_sysfs_scanf(engine, "preempt_timeout_us", "%u", &saved);
> > +	igt_assert_eq(saved, old_pt);
> >  }
> >  
> >  #define	MAX_GTS	8
> > @@ -159,7 +163,7 @@ igt_main
> >  	int gt_count = 0;
> >  	int fd = -1, sys_fd, gt;
> >  	int engines_fd[MAX_GTS], gt_fd[MAX_GTS];
> > -	unsigned int tds[MAX_GTS];
> > +	unsigned int tds[MAX_GTS], pts[MAX_GTS];
> >  
> >  	igt_fixture {
> >  		fd = drm_open_driver(DRIVER_XE);
> > @@ -178,6 +182,9 @@ igt_main
> >  			igt_require(igt_sysfs_scanf(engines_fd[gt_count],
> >  						    "timeslice_duration_us",
> >  						    "%u", &tds[gt_count]) == 1);
> > +			igt_require(igt_sysfs_scanf(engines_fd[gt_count],
> > +						    "preempt_timeout_us",
> > +						    "%u", &pts[gt_count]) == 1);
> >  			gt_count++;
> >  		}
> >  	}
> > @@ -198,6 +205,11 @@ igt_main
> >  		for (int i = gt_count - 1; i >= 0; i--) {
> >  			unsigned int store;
> >  
> > +			igt_assert_lte(0, igt_sysfs_printf(engines_fd[i], "preempt_timeout_us",
> 
> Imho better abort.

I don't think failing the assertion here is necessarily igt breaking.  What we're more
concerned with is if the sysfs value is restored to its original value, and if the printf
fails here, then it may have also failed in the test itself, meaning the value was never
modified to begin with.

> 
> > +							   "%u", pts[i]));
> 
> What about setting first:
> 			store = pts[i] - 1;
> so it will differ in case scanf didn't read anything?

I don't think the preempt_timeout_us or timeslice_duration_us values can ever
be less than zero, so I'll set the store value to -1 before the read.
-Jonathan Cavitt

> 
> With or without above
> Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> 
> > +			igt_sysfs_scanf(engines_fd[i], "preempt_timeout_us", "%u", &store);
> > +			igt_abort_on_f(store != pts[i], "preempt_timeout_us not restored!\n");
> > +
> >  			igt_assert_lte(0, igt_sysfs_printf(engines_fd[i], "timeslice_duration_us",
> >  							   "%u", tds[i]));
> >  			igt_sysfs_scanf(engines_fd[i], "timeslice_duration_us", "%u", &store);
> > -- 
> > 2.43.0
> > 
>