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

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

[-- Attachment #1: Type: text/plain, Size: 4064 bytes --]


On 8/26/2024 8:06 PM, Gustavo Sousa wrote:
> Quoting Lucas De Marchi (2024-08-22 18:01:19-03:00)
>> 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 PiecielskaKatarzyna.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.
> Well, for s2idle (s0ix for intel), DC6 is required, which in turn
> requires the DMC.
>
> For s3, I'm not really sure what DC state would be required, but I would
> assume that we would need the driver to put the display in a low-power
> state, so disable_display=1 seems to be needed here. Someone with more
> experience could chime in to confirm this.
>
> --
> Gustavo Sousa

Is there someone else specific that can be brought into this 
conversation to help answer these questions ?

Thanks,

Suja

[-- Attachment #2: Type: text/html, Size: 5424 bytes --]