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 5:12 PM, Sundaresan, Sujaritha wrote:
>
> 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

Hi Kamil,

Just a gentle ping on the above question.

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