[PATCH i-g-t, v3] tests/intel: Add tests to run suspend without display
Sundaresan, Sujaritha
sujaritha.sundaresan at intel.com
Fri Aug 9 11:41:11 UTC 2024
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 at intel.com
>
> Kasia
>
Hi Kasia,
Thank you for the ack on the doc.
-Suja
> *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/b9d38227/attachment-0001.htm>
More information about the igt-dev
mailing list