[PATCH v7 2/4] tests/intel/xe_sysfs*: Restore values on test failure
Cavitt, Jonathan
jonathan.cavitt at intel.com
Mon Dec 2 15:13:21 UTC 2024
-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com>
Sent: Tuesday, November 26, 2024 8:56 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 v7 2/4] tests/intel/xe_sysfs*: Restore values on test failure
>
> Hi Jonathan,
> On 2024-11-18 at 18:05:09 +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)
> >
> > v4:
> > - Fix igt_sysfs_scanf/printf usage in tests (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 | 58 +++++++++++++++++++----
> > tests/intel/xe_sysfs_timeslice_duration.c | 57 ++++++++++++++++++----
> > 2 files changed, 96 insertions(+), 19 deletions(-)
> >
> > diff --git a/tests/intel/xe_sysfs_preempt_timeout.c b/tests/intel/xe_sysfs_preempt_timeout.c
> > index 7fa0dfcdf7..841c04b215 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][XE_MAX_ENGINE_INSTANCE];
> >
> > igt_fixture {
> > fd = drm_open_driver(DRIVER_XE);
> > @@ -192,26 +195,61 @@ igt_main
> > sys_fd = igt_sysfs_open(fd);
> > igt_require(sys_fd != -1);
> > close(sys_fd);
> > +
> > + xe_for_each_gt(fd, gt) {
> > + int *list, size;
> > + 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);
> > +
> > + list = igt_sysfs_get_engine_list(engines_fd[gt_count], &size);
> > + igt_require(size > 0);
> > +
> > + for (int i = 0; i < size; i++)
> > + igt_require(igt_sysfs_scanf(list[i], "preempt_timeout_us", "%u",
> > + &pts[gt_count][i]) == 1);
> > +
> > + igt_sysfs_free_engine_list(list, size);
> > + 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--) {
> > + int *list, size;
> > + list = igt_sysfs_get_engine_list(engines_fd[i], &size);
> > +
> > + for (int j = size - 1; j >= 0; j--) {
> > + unsigned int store = UINT_MAX;
> > +
> > + igt_assert_lte(0, igt_sysfs_printf(list[j], "preempt_timeout_us",
>
> This should be abort.
This aborts later if the write fails, though. Unless igt_assert ends the fixture early?
Maybe we shouldn't be asserting anything at all here?
-Jonathan Cavitt
>
> > + "%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");
> > + }
> > +
> > + igt_sysfs_free_engine_list(list, size);
> > + 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..22c543692c 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][XE_MAX_ENGINE_INSTANCE];
> >
> > igt_fixture {
> > fd = drm_open_driver(DRIVER_XE);
> > @@ -164,25 +167,61 @@ igt_main
> > sys_fd = igt_sysfs_open(fd);
> > igt_require(sys_fd != -1);
> > close(sys_fd);
> > +
> > + xe_for_each_gt(fd, gt) {
> > + int *list, size;
> > + 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);
> > +
> > + list = igt_sysfs_get_engine_list(engines_fd[gt_count], &size);
> > + igt_require(size > 0);
> > +
> > + for (int i = 0; i < size; i++)
> > + igt_require(igt_sysfs_scanf(list[i], "timeslice_duration_us", "%u",
> > + &tds[gt_count][i]) == 1);
> > +
> > + igt_sysfs_free_engine_list(list, size);
> > + 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--) {
> > + int *list, size;
> > + list = igt_sysfs_get_engine_list(engines_fd[i], &size);
> > +
> > + for (int j = size - 1; j >= 0; j--) {
> > + unsigned int store = UINT_MAX;
> > +
> > + igt_assert_lte(0, igt_sysfs_printf(list[j], "timeslice_duration_us",
>
> Same here.
> With above fixed you can keep my r-b,
>
> Regards,
> Kamil
>
> > + "%u", tds[i][j]));
> > + igt_sysfs_scanf(list[j], "timeslice_duration_us", "%u", &store);
> > + igt_abort_on_f(store != tds[i][j],
> > + "timeslice_duration_us not restored!\n");
> > + }
> > +
> > + igt_sysfs_free_engine_list(list, size);
> > + close(engines_fd[i]);
> > + close(gt_fd[i]);
> > + }
> > +
> > drm_close_driver(fd);
> > }
> > }
> > --
> > 2.43.0
> >
>
More information about the igt-dev
mailing list