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

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

On 8/9/2024 3:40 PM, Kamil Konieczny wrote:
> Hi Sundaresan,,
> On 2024-08-09 at 14:34:23 +0530, Sundaresan, Sujaritha wrote:
>> On 8/6/2024 6:20 PM, Kamil Konieczny wrote:
>>> Hi Sundaresan,,
>>> On 2024-08-06 at 11:50:46 +0530, Sundaresan, Sujaritha wrote:
>>>> 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>
>> Hi Kamil,
>>
>> Sorry I didn't get this change. This is inline with the rest of the file
>> right ?
> Yes but this looks strange, so I added Katarzyna to discussion.
>
>>>>>> + *
>>>>>>      * 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
>>>>
>>> I would suggest turn this into a igt_skip_on_f(), not a return.
>>>
>>> Regards,
>>> Kamil
>> Sure I will switch this to
>>
>> igt_skip_on(!drmModeGetResources(device.fd_xe))
>>
>> Thanks,
>>
>> Suja
>>
> On the second look - can we just ignore this and proceed with test?
> It should do no harm when you have no display connected?
>
> Regards,
> Kamil

  So just go back to the original return right ?

Yeah I guess that wouldn't cause any problems necessarily.

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