Re: [PATCH i-g-t, v3] tests/intel: Add tests to run suspend without display

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

On 8/1/2024 5:15 PM, Kamil Konieczny wrote:
> Hi Sujaritha,
> On 2024-07-30 at 17:05:08 +0530, Sujaritha Sundaresan wrote:
>
> small nit about subject, you wrote:
>
> [PATCH i-g-t, v3] tests/intel: Add tests to run suspend without display
>
> imho this should be:
>
> [PATCH i-g-t, v3] tests/intel/xe_pm: Add tests for suspend without display
>
> More nits below.

Hey Kamil,

Sure this change I can make.

>
>> Add tests to validate basic execution suspend/resume cycle
>> without display module to rule out display related issues
>> from the suspend/resume stack.
>>
>> v2: Add normal reload cycle after running test (Anshuman)
>>
>> v3: Rebase
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
>> ---
>>   tests/intel/xe_pm.c | 34 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
>> index 8b115e2f6..03f742265 100644
>> --- a/tests/intel/xe_pm.c
>> +++ b/tests/intel/xe_pm.c
>> @@ -17,6 +17,7 @@
>>   
>>   #include "igt.h"
>>   #include "lib/igt_device.h"
>> +#include "lib/igt_kmod.h"
>>   #include "lib/igt_pm.h"
>>   #include "lib/igt_sysfs.h"
>>   #include "lib/igt_syncobj.h"
>> @@ -229,6 +230,10 @@ static void close_fw_handle(int sig)
>>    * Description: suspend/autoresume on %arg[1] state and exec after RPM
>>    * Functionality: pm - %arg[1]
>>    *
>> + * SUBTEST: %s-without-display
>> + * Description: suspend/autoresume on %arg[1] state without display
>> + * Functionality: pm - %arg[1]
> I see you copy-pasted it but imho both Description and
> Functionality documentation fields should be static, here and
> in other places.
> +cc Katarzyna Piecielska <katarzyna.piecielska@intel.com>
>
>> + *
>>    * arg[1]:
>>    *
>>    * @s2idle:	s2idle
>> @@ -681,6 +686,7 @@ igt_main
>>   	struct drm_xe_engine_class_instance *hwe;
>>   	device_t device;
>>   	uint32_t d3cold_allowed;
>> +	const char *opts;
>>   	int sysfs_fd;
>>   
>>   	const struct s_state {
>> @@ -757,6 +763,34 @@ igt_main
>>   					  NO_RPM, 0);
>>   		}
>>   
>> +		igt_subtest_f("%s-without-display", s->name) {
>> +
>> +			if (!drmModeGetResources(device.fd_xe))
>> +				return;
> Why 'return' here?! Imho this should be checked in fixture
> or be a skip. Or other way around - what about a headless board
> or one without any connected display?
>
> Regards,
> Kamil

I think this patch idea sort stemmed from the cases where we have a 
display connected and

want to make sure that the suspend/resume issues are not being caused by 
the display.

But would you suggest expanding the test to have the headless/no display 
situations? If so what changes are you suggesting for that ?

Thanks,

Suja

>
>> +
>> +			xe_for_each_engine(device.fd_xe, hwe) {
>> +
>> +				igt_debug("Reload w/o display\n");
>> +
>> +				igt_kmsg(KMSG_INFO "Unloading Xe\n");
>> +				igt_assert_eq(igt_xe_driver_unload(), 0);
>> +
>> +				igt_kmsg(KMSG_INFO "Re-loading Xe without display\n");
>> +				igt_assert_eq(igt_xe_driver_load("enable_display=0"), 0);
>> +
>> +				test_exec(device, hwe, 1, 2, s->state,
>> +					  NO_RPM, 0);
>> +
>> +				igt_debug("Reload as normal\n");
>> +
>> +				igt_kmsg(KMSG_INFO "Unloading Xe\n");
>> +				igt_assert_eq(igt_xe_driver_unload(), 0);
>> +
>> +				igt_kmsg(KMSG_INFO "Re-loading Xe\n");
>> +				igt_assert_eq(igt_xe_driver_load(opts), 0);
>> +			}
>> +		}
>> +
>>   		for (const struct vm_op *op = vm_op; op->name; op++) {
>>   			igt_subtest_f("%s-vm-bind-%s", s->name, op->name) {
>>   				xe_for_each_engine(device.fd_xe, hwe)
>> -- 
>> 2.34.1
>>