Re: [PATCH i-g-t, v4] tests/intel/xe_pm: Add tests for suspend without display

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

On 8/23/2024 2:31 AM, Lucas De Marchi wrote:
> On Thu, Aug 22, 2024 at 02:45:15PM GMT, Gustavo Sousa wrote:
>> Quoting Kamil Konieczny (2024-08-22 13:52:43-03:00)
>>> Hi Sujaritha,
>>> On 2024-08-22 at 14:50:58 +0530, Sujaritha Sundaresan wrote:
>>>> 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
>>>>
>>>> v4: Change commit name/header (Kamil)
>>>>
>>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>>> Acked-by: Katarzyna Piecielska 
>>>> Katarzyna.piecielska@intel.com<Katarzyna.piecielska@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 eee89428c..1623a56c7 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]
>>>> + *
>>>>   * 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;
>>>
>>> This should be skip or save it (as global?) and run test on headless.
>>>
>>>> +
>>>> +                        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);
>>>
>>> This param was renamed to probe_display, see
>>> https://patchwork.freedesktop.org/series/137096/
>>> tests/intel/xe_module_load: Rename enable_display module param
>>> Submitted by Lucas De Marchi on Aug. 9, 2024, 5:30 p.m.
>>>
>>> +Cc Lucas
>>
>> And disable_display=1 might be more appropriate, because probe_display
>> would simply skip the probe for display?
>
> Good point. For platforms with display, is it expected that suspend works
> if DMC is not loaded? Otherwise we will actually have to touch display
> to disable it to be allowed to suspend.
>
> Lucas De Marchi

Hi all,

Thanks for all of the comments. The need for this test mostly stemmed from

ruling out display as being the reason for a lot of the random 
suspend/resume bugs we see.

So what do you think should be the approach for this?

Thanks,

Suja