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

[Cavitt, Jonathan]

-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
Sent: Thursday, October 31, 2024 8:49 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,
> 
> 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>

Thank you for the review!  I'm going to attempt to create a patch that
restores the sysfs values on test exit, so if the test fails the sysfs values
remain unchanged.  If that's unsuccessful, we'll at least have this patch
to fall back on.
-Jonathan Cavitt

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