[PATCH v10 3/4] tests/intel/xe_sysfs_timeslice_duration: Restore preempt timeout
Upadhyay, Tejas
tejas.upadhyay at intel.com
Fri Dec 6 05:46:35 UTC 2024
> -----Original Message-----
> From: Cavitt, Jonathan <jonathan.cavitt at intel.com>
> Sent: Thursday, December 5, 2024 8:47 PM
> To: Upadhyay, Tejas <tejas.upadhyay 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: 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.
I had thought on this again, two cases here :
1. User manually changes on their local systems, and subsequent time sensitive tests are failing, we should be ok, user should also expect same. We are good.
2. CI has manual change in these timeouts through sysfs (echo), test will fail. Changing manually on CI should not be allowed (through any IGT test is fine) or if manual change is done failures should not be reported. Then we are good here as well.
Overall it looks good here with above expectations,
Reviewed-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
Tejas
> -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
> >
> >
More information about the igt-dev
mailing list