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

[Lucas De Marchi]

On Mon, May 06, 2024 at 10:14:34AM GMT, Rodrigo Vivi wrote:
>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 at 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...

I didn't bother because I think we may actually go the other direction,
unconditionally passing SUSPEND_TEST_DEVICES regardless of S state.

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

thanks
Lucas De Marchi

>
>> +
>> +			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
>>