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

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

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


On 8/9/2024 4:28 PM, Piecielska, Katarzyna wrote:
>
> For documentation – everything looks correct, a little bit different 
> then in most of cases, but correct. I’ve generated docs and all needed 
> tests are visible.
>
> From this point of view LGTM Acked-by: Katarzyna Piecielska 
> Katarzyna.piecielska@intel.com
>
> Kasia
>
Hi Kasia,

Thank you for the ack on the doc.

-Suja

> *From:*Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
> *Sent:* Friday, August 9, 2024 11:04 AM
> *To:* Kamil Konieczny <kamil.konieczny@linux.intel.com>; 
> igt-dev@lists.freedesktop.org; Gupta, Anshuman 
> <anshuman.gupta@intel.com>; Piecielska, Katarzyna 
> <katarzyna.piecielska@intel.com>
> *Subject:* Re: [PATCH i-g-t, v3] tests/intel: Add tests to run suspend 
> without display
>
> 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>  <mailto:sujaritha.sundaresan@intel.com>
>
>                 Reviewed-by: Anshuman Gupta<anshuman.gupta@intel.com>  <mailto: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>  <mailto:katarzyna.piecielska@intel.com>
>
> Hi Kamil,
>
> Sorry I didn't get this change. This is inline with the rest of the 
> file right ?
>
>                 + *
>
>                     * 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
>
>                 +
>
>                 +                    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
>

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