[PATCH v7 2/4] tests/intel/xe_sysfs*: Restore values on test failure
Kamil Konieczny
kamil.konieczny at linux.intel.com
Tue Nov 26 16:55:41 UTC 2024
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.
> + "%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