[PATCH v10 3/4] tests/intel/xe_sysfs_timeslice_duration: Restore preempt timeout

[Cavitt, Jonathan]

-----Original Message-----
From: Upadhyay, Tejas <tejas.upadhyay at intel.com> 
Sent: Thursday, December 5, 2024 4:15 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; igt-dev at lists.freedesktop.org; Brost, Matthew <matthew.brost at intel.com>
Cc: Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; kamil.konieczny at linux.intel.com; Belgaumkar, Vinay <vinay.belgaumkar at intel.com>
Subject: RE: [PATCH v10 3/4] tests/intel/xe_sysfs_timeslice_duration: Restore preempt timeout
> > -----Original Message-----
> > From: Cavitt, Jonathan <jonathan.cavitt at intel.com>
> > Sent: Thursday, December 5, 2024 3: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>;
> > kamil.konieczny at linux.intel.com; Belgaumkar, Vinay
> > <vinay.belgaumkar at intel.com>; Upadhyay, Tejas
> > <tejas.upadhyay at intel.com>
> > Subject: [PATCH v10 3/4] tests/intel/xe_sysfs_timeslice_duration: Restore
> > preempt timeout
> > 
> > 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)
> 
> Restoring timeouts after test looks ok here, you can add my r-o-b for that. But if there is value already set via sysfs manually which is not default values then we still have chance to hit the issues we are fixing here. I have tested this manually what I am talking here.
> 
> I think restoring or setting up these timeout values to default before other igt tests like igt at xe_vm@bind-execqueues-independent is very important or else low values of these timeout set manually via sysfs (echo value > timeout) might fail the test.
> 
> Although I am not very sure storing restoring timeout values should be based on default values or not. @Brost, Matthew any input here?

Do any other tests change these sysfs parameters?  Because if not, the restored value and the default value
should be the same when running IGT.

As for the manual case, where user changes may occur before testing, it should be in our best interest to
restore any user settings we change during testing.
-Jonathan Cavitt

> 
> Tejas
> > 
> > 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>
> > Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > ---
> >  tests/intel/xe_sysfs_timeslice_duration.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/intel/xe_sysfs_timeslice_duration.c
> > b/tests/intel/xe_sysfs_timeslice_duration.c
> > index 899093a9df..752672691f 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,6 +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 pts[MAX_GTS][XE_MAX_ENGINE_INSTANCE];
> >  	unsigned int tds[MAX_GTS][XE_MAX_ENGINE_INSTANCE];
> >  	int *engine_list[MAX_GTS];
> > 
> > @@ -184,6 +189,8 @@ igt_main
> >  			while (list[i] != -1) {
> >  				igt_require(igt_sysfs_scanf(list[i],
> > "timeslice_duration_us", "%u",
> >  							    &tds[gt_count][i])
> > == 1);
> > +				igt_require(igt_sysfs_scanf(list[i],
> > "preempt_timeout_us", "%u",
> > +							    &pts[gt_count][i])
> > == 1);
> >  				i++;
> >  			}
> > 
> > @@ -215,8 +222,16 @@ igt_main
> >  			while (list[j] != -1) {
> >  				unsigned int store = UINT_MAX;
> > 
> > +				igt_sysfs_printf(list[j], "preempt_timeout_us",
> > +						 "%u", pts[i][j]);
> > +				igt_sysfs_scanf(list[j], "preempt_timeout_us",
> > +						"%u", &store);
> > +				igt_abort_on_f(store != pts[i][j],
> > +					       "preempt_timeout_us not
> > restored!\n");
> > +
> > +				store = UINT_MAX;
> >  				igt_sysfs_printf(list[j],
> > "timeslice_duration_us",
> > -						"%u", tds[i][j]);
> > +						 "%u", tds[i][j]);
> >  				igt_sysfs_scanf(list[j],
> > "timeslice_duration_us",
> >  						"%u", &store);
> >  				igt_abort_on_f(store != tds[i][j],
> > --
> > 2.43.0
> 
>