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

[Kamil Konieczny]

Hi,

On 2024-10-31 at 14:29:43 +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
> Sent: Wednesday, October 30, 2024 5:51 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] tests/intel/xe_sysfs_timeslice_duration: Restore preempt timeout
> > 
> > Hi Jonathan,
> > On 2024-10-29 at 21:36:21 +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.
> > > 
> > > 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>
> > > ---
> > >  tests/intel/xe_sysfs_timeslice_duration.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/intel/xe_sysfs_timeslice_duration.c b/tests/intel/xe_sysfs_timeslice_duration.c
> > > index cf95a3ac1c..6912f166b4 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));
> > 
> > Looks good but it will not reach this line if any
> > igt_assert/require triggers before.
> 
> Out of fairness, timeslice_duration_us also has this problem in this test,
> and preempt_timeout_us also has this problem in xe_sysfs_preempt_timeout.c.
> -Jonathan Cavitt
> 

Right, so there is still risk it will end up not restored.
What we need is restoration of sysfs values to changed attributes
when test fail due to some assert/require _after_ a modification
was done. Could you add a /* TODO */ note in source?

With or without this,
Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

> > 
> > Regards,
> > Kamil
> > 
> > > +	igt_sysfs_scanf(engine, "preempt_timeout_us", "%u", &saved);
> > > +	igt_assert_eq(saved, old_pt);
> > >  }
> > >  
> > >  igt_main
> > > -- 
> > > 2.43.0
> > > 
> >