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

[Piecielska, Katarzyna]

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 at intel.com<mailto:Katarzyna.piecielska at intel.com>

Kasia

From: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>
Sent: Friday, August 9, 2024 11:04 AM
To: Kamil Konieczny <kamil.konieczny at linux.intel.com>; igt-dev at lists.freedesktop.org; Gupta, Anshuman <anshuman.gupta at intel.com>; Piecielska, Katarzyna <katarzyna.piecielska at 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 at intel.com><mailto:sujaritha.sundaresan at intel.com>

Reviewed-by: Anshuman Gupta <anshuman.gupta at intel.com><mailto:anshuman.gupta at 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 at intel.com><mailto:katarzyna.piecielska at 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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20240809/a0596460/attachment-0001.htm>