[PATCH v2 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: Thursday, October 31, 2024 11:30 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 v2 2/2] tests/intel/xe_sysfs_timeslice_duration: Restore preempt timeout
> 
> Hi Jonathan,
> On 2024-10-31 at 16:31:22 +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)
> > 
> > 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 | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/intel/xe_sysfs_timeslice_duration.c b/tests/intel/xe_sysfs_timeslice_duration.c
> > index 914575a282..576be98ede 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));
> 
> Imho '_lt' - zero is an error?!

This is what is done in set_timeslice_duration.  I'm fairly certain it's because
zero can be returned in cases where the sysfs printf does not need to modify
the value, though if that is not correct, do you also want me to change it in
set_timeslice_duration?
-Jonathan Cavitt

> 
> > +	igt_sysfs_scanf(engine, "preempt_timeout_us", "%u", &saved);
> > +	igt_assert_eq(saved, old_pt);
> >  }
> >  
> >  igt_main
> > @@ -158,7 +162,7 @@ igt_main
> >  	int gt_count = 0;
> >  	int fd = -1, sys_fd, gt;
> >  	int engines_fd[8], gt_fd[8];
> > -	unsigned int tds[8];
> > +	unsigned int tds[8], pts[8];
> >  
> >  	igt_fixture {
> >  		fd = drm_open_driver(DRIVER_XE);
> > @@ -176,6 +180,9 @@ igt_main
> >  			igt_assert(igt_sysfs_scanf(engines_fd[gt_count],
> >  						   "timeslice_duration_us",
> >  						   "%u", &tds[gt_count]) == 1);
> > +			igt_assert(igt_sysfs_scanf(engines_fd[gt_count],
> 
> imho igt_require should be enough.
> 
> > +						   "preempt_timeout_us",
> > +						   "%u", &pts[gt_count]) == 1);
> >  			gt_count++;
> >  		}
> >  	}
> > @@ -194,6 +201,8 @@ igt_main
> >  	}
> >  	igt_fixture {
> >  		for (int i = gt_count - 1; i >= 0; i--) {
> > +			igt_assert_lte(0, igt_sysfs_printf(engines_fd[i], "preempt_timeout_us",
> 
> This should be '> 0' as printf return number of bytes written,
> also in case of error imho better use igt_abort()
> We do not want proceed with any test after a failure.
> 
> Maybe add also check here if old value was really restored?
> 
> > +							   "%u", pts[i]));
> >  			set_timeslice_duration(engines_fd[i], tds[i]);
> 
> Same problem is here, function makes checks and uses igt_assert(),
> instead of calling it, open code restoration and abort on error.
> 
> Or code that function to return bool instead of asserting?
> 
> Same goes for restoration in patch 1.
> 
> Regards,
> Kamil
> 
> >  			close(engines_fd[i]);
> >  			close(gt_fd[i]);
> > -- 
> > 2.43.0
> > 
>