[PATCH i-g-t] tests/intel/xe_pm: s3 vs s2idle: Validate only the platform default
Rodrigo Vivi
rodrigo.vivi at intel.com
Mon May 20 20:17:47 UTC 2024
On Mon, May 20, 2024 at 02:56:11PM -0500, Lucas De Marchi wrote:
> On Mon, May 20, 2024 at 03:08:57PM GMT, Rodrigo Vivi wrote:
> > It is very common to reach issues from other drivers when testing
> > the full non-default cycle.
> >
> > Get back to i915 approach and validate only the default mem_sleep
> > level of the mem sleep state.
> >
> > However, keep the option in place where the developer can force
> > the validation of the state using an extra test argument.
>
> so this will read /sys/power/mem_sleep and execute only that level
>
> why not "up to that level" so we still catch and differentiate issues
> about failing to freeze vs failing to suspend?
>
> what current issue is this actually fixing?
well, this is not fixing anything. But I have encountered cases where
the non-default mem_state was failing badly due to issues in other
components, then graphics.
I guess it is because they are less validated and less used.
Recently I noticed the trend of getting only s2idle available for laptops,
but this is not something broad.
Than this patch came to mind when Francois told me he was facing some
strange issues non graphics related on some tests and that was exactly
in the non-default configuration:
[ 650.495418] ACPI: \_SB_.BAT0: _BST evaluation failed: AE_TIME
[ 653.033673] ACPI Error: AE_TIME, Returned by Handler for [EmbeddedControl] (20230628/evregion-300)
[ 653.033717] ACPI Error: Timeout from EC hardware or EC device driver (20230628/evregion-310)
[ 653.033751] ACPI Error: Aborting method \_TZ.GTTP due to previous error (AE_TIME) (20230628/psparse-529)
[ 653.033788] ACPI Error: Aborting method \_TZ.LOCZ._TMP due to previous error (AE_TIME) (20230628/psparse-529)
[ 653.033813] ACPI Error: Aborting method \_SB.PC00.LPCB.EC0.SEN2._TMP due to previous error (AE_TIME) (20230628/psparse-529)
So, we could test all, or up to certain level, but we will continue to get noise I'm afraid.
>
> >
> > v2: Fix Subject (Kamil)
> >
> > Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > Cc: Francois Dugast <francois.dugast at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> > lib/igt_aux.c | 31 +++++++++++++++++++++-----
> > lib/igt_aux.h | 2 ++
> > tests/intel/xe_pm.c | 54 ++++++++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 80 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> > index 3407cc4f2..7770a05fc 100644
> > --- a/lib/igt_aux.c
> > +++ b/lib/igt_aux.c
> > @@ -852,7 +852,15 @@ static bool is_state_supported(int power_dir, enum igt_suspend_state state)
> > return str;
> > }
> >
> > -static int get_mem_sleep(void)
> > +/**
> > + * igt_system_get_mem_sleep:
> > + *
> > + * Used to query the current configured mem_sleep. In special to stash the
> > + * default of the system before any test operation.
> > + *
> > + * Return: Current mem_sleep configured.
> > + */
> > +enum igt_mem_sleep igt_system_get_mem_sleep(void)
> > {
> > char *mem_sleep_states;
> > char *mem_sleep_state;
> > @@ -891,14 +899,25 @@ static int get_mem_sleep(void)
> > return mem_sleep;
> > }
> >
> > -static void set_mem_sleep(int power_dir, enum igt_mem_sleep sleep)
> > +/**
> > + * igt_system_set_mem_sleep:
> > + *
> > + * Used to configure the mem_sleep. Useful to return the platform to its default.
> > + */
> > +void igt_system_set_mem_sleep(enum igt_mem_sleep sleep)
> > {
> > + int power_dir;
> > +
> > igt_assert(sleep < MEM_SLEEP_NUM);
> >
> > + igt_require((power_dir = open("/sys/power", O_RDONLY)) >= 0);
> > +
> > igt_assert_eq(faccessat(power_dir, "mem_sleep", W_OK, 0), 0);
> >
> > igt_assert(igt_sysfs_set(power_dir, "mem_sleep",
> > mem_sleep_name[sleep]));
> > +
> > + close(power_dir);
> > }
> >
> > static bool is_mem_sleep_state_supported(int power_dir, enum igt_mem_sleep state)
> > @@ -1018,11 +1037,11 @@ void igt_system_suspend_autoresume(enum igt_suspend_state state,
> > igt_aux_enable_pm_suspend_dbg(power_dir);
> >
> > if (state == SUSPEND_STATE_S3) {
> > - orig_mem_sleep = get_mem_sleep();
> > + orig_mem_sleep = igt_system_get_mem_sleep();
> > igt_skip_on_f(!is_mem_sleep_state_supported(power_dir, MEM_SLEEP_DEEP),
> > "S3 not supported in this system.\n");
> > - set_mem_sleep(power_dir, MEM_SLEEP_DEEP);
> > - igt_skip_on_f(get_mem_sleep() != MEM_SLEEP_DEEP,
> > + igt_system_set_mem_sleep(MEM_SLEEP_DEEP);
> > + igt_skip_on_f(igt_system_get_mem_sleep() != MEM_SLEEP_DEEP,
> > "S3 not possible in this system.\n");
> > }
> >
> > @@ -1034,7 +1053,7 @@ void igt_system_suspend_autoresume(enum igt_suspend_state state,
> > suspend_via_sysfs(power_dir, state);
> >
> > if (orig_mem_sleep)
> > - set_mem_sleep(power_dir, orig_mem_sleep);
> > + igt_system_set_mem_sleep(orig_mem_sleep);
> >
> > set_pm_test(power_dir, orig_test);
> > close(power_dir);
> > diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> > index 4664559dc..8fd21c37b 100644
> > --- a/lib/igt_aux.h
> > +++ b/lib/igt_aux.h
> > @@ -214,6 +214,8 @@ enum igt_mem_sleep {
> > MEM_SLEEP_NUM,
> > };
> >
> > +enum igt_mem_sleep igt_system_get_mem_sleep(void);
> > +void igt_system_set_mem_sleep(enum igt_mem_sleep sleep);
> > void igt_system_suspend_autoresume(enum igt_suspend_state state,
> > enum igt_suspend_test test);
> > void igt_set_autoresume_delay(int delay_secs);
> > diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
> > index b4a8c4d15..29d576522 100644
> > --- a/tests/intel/xe_pm.c
> > +++ b/tests/intel/xe_pm.c
> > @@ -44,11 +44,18 @@ typedef struct {
> > struct pci_device *pci_root;
> > char pci_slot_name[NAME_MAX];
> > drmModeResPtr res;
> > + enum igt_mem_sleep default_mem_sleep;
> > } device_t;
> >
> > uint64_t orig_threshold;
> > int fw_handle = -1;
> >
> > +static struct param {
> > + bool ignore_default_mem_sleep;
> > +} params = {
> > + .ignore_default_mem_sleep = false,
> > +};
> > +
> > static void dpms_on_off(device_t device, int mode)
> > {
> > int i;
> > @@ -221,6 +228,21 @@ static void close_fw_handle(int sig)
> > close(fw_handle);
> > }
> >
> > +static void assert_default_mem_sleep(device_t device, enum igt_suspend_state s_state)
> > +{
> > + if (s_state == NO_SUSPEND)
> > + return;
> > +
> > + if (params.ignore_default_mem_sleep)
> > + return;
> > +
> > + if (device.default_mem_sleep == MEM_SLEEP_S2IDLE)
> > + igt_require(s_state == SUSPEND_STATE_FREEZE);
> > +
> > + if (device.default_mem_sleep == MEM_SLEEP_DEEP)
>
> else if ...
ack
>
>
> > + igt_require(s_state == SUSPEND_STATE_S3);
>
> isn't this actually killing the S4 tests (which right now are up to
> device suspend only...)?
doh! indeed.
we should probably have a
if (s_state == SUSPEND_STATE_S3)
return;
before the igt_reuquire block.
Thanks a lot,
Rodrigo.
>
>
> Lucas De Marchi
>
> > +}
> > +
> > #define MAX_VMAS 2
> > /**
> > * SUBTEST: %s-basic
> > @@ -327,6 +349,8 @@ test_exec(device_t device, struct drm_xe_engine_class_instance *eci,
> > bool check_rpm = (d_state == IGT_ACPI_D3Hot ||
> > d_state == IGT_ACPI_D3Cold);
> >
> > + assert_default_mem_sleep(device, s_state);
> > +
> > igt_assert(n_exec_queues <= MAX_N_EXEC_QUEUES);
> > igt_assert(n_execs > 0);
> >
> > @@ -636,6 +660,8 @@ static void test_mocs_suspend_resume(device_t device, int s_state)
> > {
> > int gt;
> >
> > + assert_default_mem_sleep(device, s_state);
> > +
> > xe_for_each_gt(device.fd_xe, gt) {
> > char path[256];
> >
> > @@ -669,7 +695,28 @@ static void test_mocs_suspend_resume(device_t device, int s_state)
> > }
> > }
> >
> > -igt_main
> > +static int opt_handler(int opt, int opt_index, void *data)
> > +{
> > + switch(opt) {
> > + case 'm':
> > + params.ignore_default_mem_sleep = true;
> > + igt_debug("Ignoring default mem_state!\n");
> > + break;
> > + default:
> > + return IGT_OPT_HANDLER_ERROR;
> > + }
> > + return IGT_OPT_HANDLER_SUCCESS;
> > +}
> > +
> > +struct option long_options[] = {
> > + {"ignore-default-mem-sleep", no_argument, NULL, 'm'},
> > + {0, 0, 0, 0}
> > +};
> > +
> > +const char *help_str =
> > + " --ignore-default-mem-sleep\tUsed to enable s3 tests on systems where s2idle is the default and vice-versa\n";
> > +
> > +igt_main_args("", long_options, help_str, opt_handler, NULL)
> > {
> > struct drm_xe_engine_class_instance *hwe;
> > device_t device;
> > @@ -705,6 +752,7 @@ igt_main
> >
> > igt_fixture {
> > memset(&device, 0, sizeof(device));
> > + device.default_mem_sleep = igt_system_get_mem_sleep();
> > device.fd_xe = drm_open_driver(DRIVER_XE);
> > device.pci_xe = igt_device_get_pci_device(device.fd_xe);
> > device.pci_root = igt_device_get_pci_root_port(device.fd_xe);
> > @@ -725,6 +773,7 @@ igt_main
> > enum igt_suspend_test test = s->state == SUSPEND_STATE_DISK ?
> > SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
> >
> > + assert_default_mem_sleep(device, s->state);
> > igt_system_suspend_autoresume(s->state, test);
> > }
> >
> > @@ -738,6 +787,7 @@ igt_main
> > enum igt_suspend_test test = s->state == SUSPEND_STATE_DISK ?
> > SUSPEND_TEST_DEVICES : SUSPEND_TEST_NONE;
> >
> > + assert_default_mem_sleep(device, s->state);
> > igt_system_suspend_autoresume(s->state, test);
> > xe_for_each_engine(device.fd_xe, hwe)
> > test_exec(device, hwe, 1, 2, NO_SUSPEND,
> > @@ -848,5 +898,7 @@ igt_main
> > igt_restore_runtime_pm();
> > drmModeFreeResources(device.res);
> > drm_close_driver(device.fd_xe);
> > + igt_system_set_mem_sleep(device.default_mem_sleep);
> > }
> > +
> > }
> > --
> > 2.44.0
> >
More information about the igt-dev
mailing list