Re: [PATCH i-g-t 3/3] tests/intel/xe_pm: S4 to go up to devices only

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, May 03, 2024 at 03:47:45PM -0700, Lucas De Marchi wrote:
> Testing S4 (hibernation) is typically painful as there's mixed support
> in OS versions and platforms in CI. Doing the entire dance of saving the
> image to swap (which sometimes is a swapfile) and communicate that to
> the kernel that is going to be booted (without initrd in the CI case) is
> often a case of problems.
> 
> Main goal of xe_pm is to test if the xe driver and the graphics card are
> working correctly, not that all the farm of machines correctly handle
> all the corner cases (which is even more problematic as we test early
> rc kernels).
> 
> Stop doing that and rather switch to going up to device shutdown +
> platform low power state (the default in /sys/power/disk). If that is
> acceptable and work out great, we may even do that unconditionally,
> passing SUSPEND_TEST_DEVICES as it should work in other cases too.
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1043
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  tests/intel/xe_pm.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
> index 5e79e80ec..9a0b362ab 100644
> --- a/tests/intel/xe_pm.c
> +++ b/tests/intel/xe_pm.c
> @@ -414,9 +414,12 @@ test_exec(device_t device, struct drm_xe_engine_class_instance *eci,
>  					INT64_MAX, 0, NULL));
>  		igt_assert_eq(data[i].data, 0xc0ffee);
>  
> -		if (i == n_execs / 2 && s_state != NO_SUSPEND)
> -			igt_system_suspend_autoresume(s_state,
> -						      SUSPEND_TEST_NONE);
> +		if (i == n_execs / 2 && s_state != NO_SUSPEND) {
> +			enum igt_suspend_test test = s_state == SUSPEND_STATE_DISK ?
> +				SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
> +
> +			igt_system_suspend_autoresume(s_state, test);
> +		}
>  	}
>  
>  	igt_assert(syncobj_wait(device.fd_xe, &sync[0].handle, 1, INT64_MAX, 0,
> @@ -662,8 +665,10 @@ igt_main
>  
>  	for (const struct s_state *s = s_states; s->name; s++) {
>  		igt_subtest_f("%s-basic", s->name) {
> -			igt_system_suspend_autoresume(s->state,
> -						      SUSPEND_TEST_NONE);
> +			enum igt_suspend_test test = s->state == SUSPEND_STATE_DISK ?
> +				SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
> +
> +			igt_system_suspend_autoresume(s->state, test);
>  		}
>  
>  		igt_subtest_f("%s-basic-exec", s->name) {
> @@ -673,8 +678,10 @@ igt_main
>  		}
>  
>  		igt_subtest_f("%s-exec-after", s->name) {
> -			igt_system_suspend_autoresume(s->state,
> -						      SUSPEND_TEST_NONE);
> +			enum igt_suspend_test test = s->state == SUSPEND_STATE_DISK ?
> +				SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;

perhaps a helper function for this long repeated line?

but up to you...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +
> +			igt_system_suspend_autoresume(s->state, test);
>  			xe_for_each_engine(device.fd_xe, hwe)
>  				test_exec(device, hwe, 1, 2, NO_SUSPEND,
>  					  NO_RPM, 0);
> -- 
> 2.43.0
>