[PATCH v4 1/2] tests/intel/xe_sysfs*: Restore values on test failure
Cavitt, Jonathan
jonathan.cavitt at intel.com
Thu Nov 7 22:52:51 UTC 2024
-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
Sent: Thursday, November 7, 2024 10:04 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>
Cc: igt-dev at lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; Belgaumkar, Vinay <vinay.belgaumkar at intel.com>
Subject: Re: [PATCH v4 1/2] tests/intel/xe_sysfs*: Restore values on test failure
>
> Hi Jonathan,
> On 2024-11-05 at 20:11:42 +0000, Jonathan Cavitt wrote:
> > The tests xe_sysfs_preempt_timeout and xe_sysfs_timeslice_duration
> > modify the values of preempt_timeout_us and timeslice_duration_us,
> > respectively. However, on a test failure, it is possible that these
> > values may remain in their modified states, resulting in the values
> > being used in future tests and causing unexpected behavior.
> >
> > Save the respective modified values before starting the test and attempt
> > to restore the values on test exit.
> >
> > v2:
> > - Fix some formatting issues (Kamil)
> > - Abort if value restore fails (Kamil)
> > - Directly call igt_sysfs_printf on exit to avoid duplicating on helper
> > (Kamil)
> >
> > v3:
> > - Do not compare potentially unassigned variable (Kamil)
> > - Whitespace and commit name fixes (Kamil)
> >
> > Suggested-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > CC: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> > Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > ---
> > tests/intel/xe_sysfs_preempt_timeout.c | 44 +++++++++++++++++------
>
> Please look at XeFULL results, tests now skips:
>
> xe_sysfs_preempt_timeout at preempt_timeout_us-timeout
>
> It looks like there is another level of dirs in 'engines',
> for example on Lunar Lake:
>
> $ ls /sys/devices/pci0000:00/0000:00:02.0/tile0/gt0/engines
> bcs ccs rcs
>
> So it is more complicated.
Understood. I went back and read through the current implementation
and compared it to what's being done internally, and it turns out the
individual engines are being scraped in igt_sysfs_engines before being
passed to the tests. I had missed this part and had thought
igt_sysfs_engines was just the test function name, apparently. So, in
order to get the test to work, we'll need to iterate over the list of
engines and use said engines instead of the engine_fd as we are
currently.
Unfortunately, it seems that there's no interface to iterate over the
engines in the sysfs/engines directory yet, so I'll need to create that
first. I have already created the patch series with said interface and
all related necessary changes, and I will be posting it shortly.
Thank you for your time and effort.
-Jonathan Cavitt
>
> Regards,
> Kamil
>
> > tests/intel/xe_sysfs_timeslice_duration.c | 43 +++++++++++++++++-----
> > 2 files changed, 68 insertions(+), 19 deletions(-)
> >
> > diff --git a/tests/intel/xe_sysfs_preempt_timeout.c b/tests/intel/xe_sysfs_preempt_timeout.c
> > index 7fa0dfcdf7..cca7e19796 100644
> > --- a/tests/intel/xe_sysfs_preempt_timeout.c
> > +++ b/tests/intel/xe_sysfs_preempt_timeout.c
> > @@ -170,6 +170,7 @@ static void test_timeout(int fd, int engine, const char **property, uint16_t cla
> > set_preempt_timeout(engine, saved);
> > }
> >
> > +#define MAX_GTS 8
> > igt_main
> > {
> > static const struct {
> > @@ -183,8 +184,10 @@ igt_main
> > "preempt_timeout_min",
> > "preempt_timeout_max"}, };
> > int count = sizeof(property) / sizeof(property[0]);
> > + int gt_count = 0;
> > int fd = -1, sys_fd, gt;
> > - int engines_fd = -1, gt_fd = -1;
> > + int engines_fd[MAX_GTS], gt_fd[MAX_GTS];
> > + unsigned int pts[MAX_GTS];
> >
> > igt_fixture {
> > fd = drm_open_driver(DRIVER_XE);
> > @@ -192,26 +195,47 @@ igt_main
> > sys_fd = igt_sysfs_open(fd);
> > igt_require(sys_fd != -1);
> > close(sys_fd);
> > +
> > + xe_for_each_gt(fd, gt) {
> > + igt_require(gt_count < MAX_GTS);
> > +
> > + gt_fd[gt_count] = xe_sysfs_gt_open(fd, gt);
> > + igt_require(gt_fd[gt_count] != -1);
> > + engines_fd[gt_count] = openat(gt_fd[gt_count], "engines", O_RDONLY);
> > + igt_require(engines_fd[gt_count] != -1);
> > + igt_require(igt_sysfs_scanf(engines_fd[gt_count],
> > + "preempt_timeout_us",
> > + "%u", &pts[gt_count]) == 1);
> > + gt_count++;
> > + }
> > }
> >
> > for (int i = 0; i < count; i++) {
> > for (typeof(*tests) *t = tests; t->name; t++) {
> > igt_subtest_with_dynamic_f("%s-%s", property[i][0], t->name) {
> > + int j = 0;
> > xe_for_each_gt(fd, gt) {
> > - gt_fd = xe_sysfs_gt_open(fd, gt);
> > - igt_require(gt_fd != -1);
> > - engines_fd = openat(gt_fd, "engines", O_RDONLY);
> > - igt_require(engines_fd != -1);
> > -
> > - igt_sysfs_engines(fd, engines_fd, gt, 1, property[i],
> > - t->fn);
> > - close(engines_fd);
> > - close(gt_fd);
> > + int e = engines_fd[j];
> > +
> > + igt_sysfs_engines(fd, e, gt, 1, property[i], t->fn);
> > + j++;
> > }
> > }
> > }
> > }
> > igt_fixture {
> > + for (int i = gt_count - 1; i >= 0; i--) {
> > + unsigned int store = UINT_MAX;
> > +
> > + igt_assert_lte(0, igt_sysfs_printf(engines_fd[i], "preempt_timeout_us",
> > + "%u", pts[i]));
> > + igt_sysfs_scanf(engines_fd[i], "preempt_timeout_us", "%u", &store);
> > + igt_abort_on_f(store != pts[i], "preempt_timeout_us not restored!\n");
> > +
> > + close(engines_fd[i]);
> > + close(gt_fd[i]);
> > + }
> > +
> > drm_close_driver(fd);
> > }
> > }
> > diff --git a/tests/intel/xe_sysfs_timeslice_duration.c b/tests/intel/xe_sysfs_timeslice_duration.c
> > index cf95a3ac1c..dfcdd7335f 100644
> > --- a/tests/intel/xe_sysfs_timeslice_duration.c
> > +++ b/tests/intel/xe_sysfs_timeslice_duration.c
> > @@ -142,6 +142,7 @@ static void test_timeout(int fd, int engine, const char **property, uint16_t cla
> > set_timeslice_duration(engine, saved);
> > }
> >
> > +#define MAX_GTS 8
> > igt_main
> > {
> > static const struct {
> > @@ -155,8 +156,10 @@ igt_main
> > "timeslice_duration_min",
> > "timeslice_duration_max"}, };
> > int count = sizeof(property) / sizeof(property[0]);
> > + int gt_count = 0;
> > int fd = -1, sys_fd, gt;
> > - int engines_fd = -1, gt_fd = -1;
> > + int engines_fd[MAX_GTS], gt_fd[MAX_GTS];
> > + unsigned int tds[MAX_GTS];
> >
> > igt_fixture {
> > fd = drm_open_driver(DRIVER_XE);
> > @@ -164,25 +167,47 @@ igt_main
> > sys_fd = igt_sysfs_open(fd);
> > igt_require(sys_fd != -1);
> > close(sys_fd);
> > +
> > + xe_for_each_gt(fd, gt) {
> > + igt_require(gt_count < MAX_GTS);
> > +
> > + gt_fd[gt_count] = xe_sysfs_gt_open(fd, gt);
> > + igt_require(gt_fd[gt_count] != -1);
> > + engines_fd[gt_count] = openat(gt_fd[gt_count], "engines", O_RDONLY);
> > + igt_require(engines_fd[gt_count] != -1);
> > + igt_require(igt_sysfs_scanf(engines_fd[gt_count],
> > + "timeslice_duration_us",
> > + "%u", &tds[gt_count]) == 1);
> > + gt_count++;
> > + }
> > }
> >
> > for (int i = 0; i < count; i++) {
> > for (typeof(*tests) *t = tests; t->name; t++) {
> > igt_subtest_with_dynamic_f("%s-%s", property[i][0], t->name) {
> > + int j = 0;
> > xe_for_each_gt(fd, gt) {
> > - gt_fd = xe_sysfs_gt_open(fd, gt);
> > - igt_require(gt_fd != -1);
> > - engines_fd = openat(gt_fd, "engines", O_RDONLY);
> > - igt_require(engines_fd != -1);
> > - igt_sysfs_engines(fd, engines_fd, gt, 1, property[i],
> > - t->fn);
> > - close(engines_fd);
> > - close(gt_fd);
> > + int e = engines_fd[j];
> > +
> > + igt_sysfs_engines(fd, e, gt, 1, property[i], t->fn);
> > + j++;
> > }
> > }
> > }
> > }
> > igt_fixture {
> > + for (int i = gt_count - 1; i >= 0; i--) {
> > + unsigned int store = UINT_MAX;
> > +
> > + igt_assert_lte(0, igt_sysfs_printf(engines_fd[i], "timeslice_duration_us",
> > + "%u", tds[i]));
> > + igt_sysfs_scanf(engines_fd[i], "timeslice_duration_us", "%u", &store);
> > + igt_abort_on_f(store != tds[i], "timeslice_duration_us not restored!\n");
> > +
> > + close(engines_fd[i]);
> > + close(gt_fd[i]);
> > + }
> > +
> > drm_close_driver(fd);
> > }
> > }
> > --
> > 2.43.0
> >
>
More information about the igt-dev
mailing list