<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 8/9/2024 4:28 PM, Piecielska, Katarzyna wrote: </div> <blockquote type="cite" cite="mid:IA1PR11MB60973966DEAC168058C09FFA86BA2@IA1PR11MB6097.namprd11.prod.outlook.com"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style>@font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face {font-family:Calibri; panose-1:2 15 5 2 2 2 4 3 2 4;}@font-face {font-family:Aptos;}@font-face {font-family:Consolas; panose-1:2 11 6 9 2 2 4 3 2 4;}p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0in; font-size:12.0pt; font-family:"Aptos",sans-serif;}a:link, span.MsoHyperlink {mso-style-priority:99; color:blue; text-decoration:underline;}pre {mso-style-priority:99; mso-style-link:"HTML Preformatted Char"; margin:0in; font-size:10.0pt; font-family:"Courier New";}span.HTMLPreformattedChar {mso-style-name:"HTML Preformatted Char"; mso-style-priority:99; mso-style-link:"HTML Preformatted"; font-family:Consolas;}span.EmailStyle21 {mso-style-type:personal-reply; font-family:"Aptos",sans-serif; color:#0F4761;}.MsoChpDefault {mso-style-type:export-only; font-size:10.0pt; mso-ligatures:none;}div.WordSection1 {page:WordSection1;}</style> <div class="WordSection1"> 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.<o:p></o:p> From this point of view LGTM Acked-by: Katarzyna Piecielska <a href="mailto:Katarzyna.piecielska@intel.com" moz-do-not-send="true" class="moz-txt-link-freetext">Katarzyna.piecielska@intel.com</a><o:p></o:p> <o:p> </o:p> Kasia </div> </blockquote> Hi Kasia, Thank you for the ack on the doc. -Suja <blockquote type="cite" cite="mid:IA1PR11MB60973966DEAC168058C09FFA86BA2@IA1PR11MB6097.namprd11.prod.outlook.com"> <div class="WordSection1"> <o:p></o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Sundaresan, Sujaritha <a class="moz-txt-link-rfc2396E" href="mailto:sujaritha.sundaresan@intel.com"><sujaritha.sundaresan@intel.com></a> Sent: Friday, August 9, 2024 11:04 AM To: Kamil Konieczny <a class="moz-txt-link-rfc2396E" href="mailto:kamil.konieczny@linux.intel.com"><kamil.konieczny@linux.intel.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:igt-dev@lists.freedesktop.org">igt-dev@lists.freedesktop.org</a>; Gupta, Anshuman <a class="moz-txt-link-rfc2396E" href="mailto:anshuman.gupta@intel.com"><anshuman.gupta@intel.com></a>; Piecielska, Katarzyna <a class="moz-txt-link-rfc2396E" href="mailto:katarzyna.piecielska@intel.com"><katarzyna.piecielska@intel.com></a> Subject: Re: [PATCH i-g-t, v3] tests/intel: Add tests to run suspend without display<o:p></o:p> </div> </div> <o:p> </o:p> <o:p> </o:p> <div> On 8/6/2024 6:20 PM, Kamil Konieczny wrote:<o:p></o:p> </div> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>Hi Sundaresan,,<o:p></o:p></pre> <pre>On 2024-08-06 at 11:50:46 +0530, Sundaresan, Sujaritha wrote:<o:p></o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre>On 8/1/2024 5:15 PM, Kamil Konieczny wrote:<o:p></o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>Hi Sujaritha,<o:p></o:p></pre> <pre>On 2024-07-30 at 17:05:08 +0530, Sujaritha Sundaresan wrote:<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>small nit about subject, you wrote:<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>[PATCH i-g-t, v3] tests/intel: Add tests to run suspend without display<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>imho this should be:<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>[PATCH i-g-t, v3] tests/intel/xe_pm: Add tests for suspend without display<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>More nits below.<o:p></o:p></pre> </blockquote> <pre><o:p> </o:p></pre> <pre>Hey Kamil,<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Sure this change I can make.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>Add tests to validate basic execution suspend/resume cycle<o:p></o:p></pre> <pre>without display module to rule out display related issues<o:p></o:p></pre> <pre>from the suspend/resume stack.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>v2: Add normal reload cycle after running test (Anshuman)<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>v3: Rebase<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Signed-off-by: Sujaritha Sundaresan <a href="mailto:sujaritha.sundaresan@intel.com" moz-do-not-send="true"><sujaritha.sundaresan@intel.com></a><o:p></o:p></pre> <pre>Reviewed-by: Anshuman Gupta <a href="mailto:anshuman.gupta@intel.com" moz-do-not-send="true"><anshuman.gupta@intel.com></a><o:p></o:p></pre> <pre>---<o:p></o:p></pre> <pre> tests/intel/xe_pm.c | 34 ++++++++++++++++++++++++++++++++++<o:p></o:p></pre> <pre> 1 file changed, 34 insertions(+)<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c<o:p></o:p></pre> <pre>index 8b115e2f6..03f742265 100644<o:p></o:p></pre> <pre>--- a/tests/intel/xe_pm.c<o:p></o:p></pre> <pre>+++ b/tests/intel/xe_pm.c<o:p></o:p></pre> <pre>@@ -17,6 +17,7 @@<o:p></o:p></pre> <pre> #include "igt.h"<o:p></o:p></pre> <pre> #include "lib/igt_device.h"<o:p></o:p></pre> <pre>+#include "lib/igt_kmod.h"<o:p></o:p></pre> <pre> #include "lib/igt_pm.h"<o:p></o:p></pre> <pre> #include "lib/igt_sysfs.h"<o:p></o:p></pre> <pre> #include "lib/igt_syncobj.h"<o:p></o:p></pre> <pre>@@ -229,6 +230,10 @@ static void close_fw_handle(int sig)<o:p></o:p></pre> <pre> * Description: suspend/autoresume on %arg[1] state and exec after RPM<o:p></o:p></pre> <pre> * Functionality: pm - %arg[1]<o:p></o:p></pre> <pre> *<o:p></o:p></pre> <pre>+ * SUBTEST: %s-without-display<o:p></o:p></pre> <pre>+ * Description: suspend/autoresume on %arg[1] state without display<o:p></o:p></pre> <pre>+ * Functionality: pm - %arg[1]<o:p></o:p></pre> </blockquote> <pre>I see you copy-pasted it but imho both Description and<o:p></o:p></pre> <pre>Functionality documentation fields should be static, here and<o:p></o:p></pre> <pre>in other places.<o:p></o:p></pre> <pre>+cc Katarzyna Piecielska <a href="mailto:katarzyna.piecielska@intel.com" moz-do-not-send="true"><katarzyna.piecielska@intel.com></a><o:p></o:p></pre> </blockquote> </blockquote> </blockquote> Hi Kamil,<o:p></o:p> Sorry I didn't get this change. This is inline with the rest of the file right ?<o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>+ *<o:p></o:p></pre> <pre> * arg[1]:<o:p></o:p></pre> <pre> *<o:p></o:p></pre> <pre> * @s2idle: s2idle<o:p></o:p></pre> <pre>@@ -681,6 +686,7 @@ igt_main<o:p></o:p></pre> <pre> struct drm_xe_engine_class_instance *hwe;<o:p></o:p></pre> <pre> device_t device;<o:p></o:p></pre> <pre> uint32_t d3cold_allowed;<o:p></o:p></pre> <pre>+ const char *opts;<o:p></o:p></pre> <pre> int sysfs_fd;<o:p></o:p></pre> <pre> const struct s_state {<o:p></o:p></pre> <pre>@@ -757,6 +763,34 @@ igt_main<o:p></o:p></pre> <pre> NO_RPM, 0);<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre>+ igt_subtest_f("%s-without-display", s->name) {<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ if (!drmModeGetResources(device.fd_xe))<o:p></o:p></pre> <pre>+ return;<o:p></o:p></pre> </blockquote> <pre>Why 'return' here?! Imho this should be checked in fixture<o:p></o:p></pre> <pre>or be a skip. Or other way around - what about a headless board<o:p></o:p></pre> <pre>or one without any connected display?<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Regards,<o:p></o:p></pre> <pre>Kamil<o:p></o:p></pre> </blockquote> <pre><o:p> </o:p></pre> <pre>I think this patch idea sort stemmed from the cases where we have a display<o:p></o:p></pre> <pre>connected and<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>want to make sure that the suspend/resume issues are not being caused by the<o:p></o:p></pre> <pre>display.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>But would you suggest expanding the test to have the headless/no display<o:p></o:p></pre> <pre>situations? If so what changes are you suggesting for that ?<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Thanks,<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Suja<o:p></o:p></pre> <pre><o:p> </o:p></pre> </blockquote> <pre><o:p> </o:p></pre> <pre>I would suggest turn this into a igt_skip_on_f(), not a return.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Regards,<o:p></o:p></pre> <pre>Kamil<o:p></o:p></pre> </blockquote> Sure I will switch this to <o:p></o:p> igt_skip_on(!drmModeGetResources(device.fd_xe))<o:p></o:p> Thanks,<o:p></o:p> Suja <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>+<o:p></o:p></pre> <pre>+ xe_for_each_engine(device.fd_xe, hwe) {<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ igt_debug("Reload w/o display\n");<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ igt_kmsg(KMSG_INFO "Unloading Xe\n");<o:p></o:p></pre> <pre>+ igt_assert_eq(igt_xe_driver_unload(), 0);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ igt_kmsg(KMSG_INFO "Re-loading Xe without display\n");<o:p></o:p></pre> <pre>+ igt_assert_eq(igt_xe_driver_load("enable_display=0"), 0);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ test_exec(device, hwe, 1, 2, s->state,<o:p></o:p></pre> <pre>+ NO_RPM, 0);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ igt_debug("Reload as normal\n");<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ igt_kmsg(KMSG_INFO "Unloading Xe\n");<o:p></o:p></pre> <pre>+ igt_assert_eq(igt_xe_driver_unload(), 0);<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre>+ igt_kmsg(KMSG_INFO "Re-loading Xe\n");<o:p></o:p></pre> <pre>+ igt_assert_eq(igt_xe_driver_load(opts), 0);<o:p></o:p></pre> <pre>+ }<o:p></o:p></pre> <pre>+ }<o:p></o:p></pre> <pre>+<o:p></o:p></pre> <pre> for (const struct vm_op *op = vm_op; op->name; op++) {<o:p></o:p></pre> <pre> igt_subtest_f("%s-vm-bind-%s", s->name, op->name) {<o:p></o:p></pre> <pre> xe_for_each_engine(device.fd_xe, hwe)<o:p></o:p></pre> <pre>-- <o:p></o:p></pre> <pre>2.34.1<o:p></o:p></pre> <pre><o:p> </o:p></pre> </blockquote> </blockquote> </blockquote> </blockquote> </div> </blockquote> </body> </html>