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

[Cavitt, Jonathan]

-----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

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