[igt-dev] [PATCH i-g-t 1/6] lib/igt_pm: igt lib helper routines to support DC5/6 tests
Imre Deak
imre.deak at intel.com
Mon Aug 12 12:04:33 UTC 2019
On Tue, Jun 18, 2019 at 11:26:12PM +0530, Anshuman Gupta wrote:
> From: Jyoti Yadav <jyoti.r.yadav at intel.com>
>
> This patch does the following chnegs to lib/igt_pm.c
>
> -dmc_loaded() will be used by new test i915_pm_dc.c which will validate
> Display C States. So moving the same to igt_pm library.
>
> -Introduced igt_disable_runtime_pm() in order to disable runtime suspend
> for the function which support dc9.
>
> -Changed the igt_pm_enable_sata_link_power_management() and
> igt_pm_restore_sata_link_power_management() in order to save
> and restore the sata link power policy by an exit handler.
>
> v2: Simplify the comment section.
> v3: Remove . from the subject line.
> v4: Rebased, resolve conflicts in pm_rpm.c
> Included patch set version change log.
> v5: Listing actual change in patch set changelog to make review easier.
> v6: igt's lib added support for disabling runtime suspend,
> change in commit log. rebased due to test name pm_rpm changed
> to i915_pm_rpm.
> v7: Addressed review comment by saving POWER_DIR values in
> igt_disable_runtime_pm(). [Imre]
> v8: Addressed the review comment, igt_pm_enable_sata_link_power_management
> function to restore the original SATA link power policy if things fail
> by using an exit handler. [Imre]
> v9: IGT failure fixture in i915_pm_backlight and i915_pm_rpm.
>
> Signed-off-by: Jyoti Yadav <jyoti.r.yadav at intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> ---
> lib/igt_pm.c | 190 ++++++++++++++++++++++++++-------
> lib/igt_pm.h | 6 +-
> tests/i915/i915_pm_backlight.c | 6 +-
> tests/i915/i915_pm_rpm.c | 24 +----
> 4 files changed, 159 insertions(+), 67 deletions(-)
>
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index fd22273a..9b83fbb5 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -38,6 +38,7 @@
> #include "drmtest.h"
> #include "igt_pm.h"
> #include "igt_aux.h"
> +#include "igt_sysfs.h"
>
> /**
> * SECTION:igt_pm
> @@ -63,11 +64,15 @@ enum {
> #define MIN_POWER_STR "min_power\n"
> /* Remember to fix this if adding longer strings */
> #define MAX_POLICY_STRLEN strlen(MAX_PERFORMANCE_STR)
> +int8_t *__sata_link_pm_policies;
>
> static char __igt_pm_audio_runtime_power_save[64];
> static char * __igt_pm_audio_runtime_control_path;
> static char __igt_pm_audio_runtime_control[64];
>
> +static void __igt_pm_sata_link_pm_exit_handler(int sig);
> +static void __igt_pm_restore_sata_link_power_management(void);
> +
> static int __igt_pm_audio_restore_runtime_pm(void)
> {
> int fd;
> @@ -280,28 +285,12 @@ void igt_pm_enable_audio_runtime_pm(void)
> igt_debug("Failed to enable audio runtime PM! (%d)\n", -err);
> }
>
> -/**
> - * igt_pm_enable_sata_link_power_management:
> - *
> - * Enable the min_power policy for SATA link power management.
> - * Without this we cannot reach deep runtime power states.
> - *
> - * We don't have any assertions on open since the system might not have
> - * a SATA host.
> - *
> - * Returns:
> - * An opaque pointer to the data needed to restore the default values
> - * after the test has terminated, or NULL if SATA link power management
> - * is not supported. This pointer should be freed when no longer used
> - * (typically after having called restore_sata_link_power_management()).
> - */
> -int8_t *igt_pm_enable_sata_link_power_management(void)
> +static void __igt_pm_enable_sata_link_power_management(void)
> {
> int fd, i;
> ssize_t len;
> char *buf;
> char *file_name;
> - int8_t *link_pm_policies = NULL;
>
> file_name = malloc(PATH_MAX);
> buf = malloc(MAX_POLICY_STRLEN + 1);
> @@ -313,6 +302,9 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
> "/sys/class/scsi_host/host%d/link_power_management_policy",
> i);
>
> + /* We don't have any assertions on open since the system
> + * might not have a SATA host.
> + */
> fd = open(file_name, O_RDWR);
> if (fd < 0)
> break;
> @@ -333,11 +325,13 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
> policy = POLICY_UNKNOWN;
>
> if (!(i % 256))
> - link_pm_policies = realloc(link_pm_policies,
> - (i / 256 + 1) * 256 + 1);
> + __sata_link_pm_policies =
> + realloc(__sata_link_pm_policies,
> + (i / 256 + 1) * 256 + 1);
Missing indentation.
>
> - link_pm_policies[i] = policy;
> - link_pm_policies[i + 1] = 0;
Not added by this patch, but can't see the purpose of this 0
termination.
> + __sata_link_pm_policies[i] = policy;
> + __sata_link_pm_policies[i + 1] = 0;
> + igt_install_exit_handler(__igt_pm_sata_link_pm_exit_handler);
This would restore uninitialized values to subsequent SATA hosts, if
some failure would happen before the loop ends. So I'd rather:
for (sata_host_count = 0; ; sata_host_count++)
save the link policiy
install exit handler
for (i = 0; i < sata_host_count; i++)
set link policy to min power
>
> /* If the policy is something we don't know about,
> * don't touch it, since we might potentially break things.
> @@ -355,39 +349,27 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
> }
> free(buf);
> free(file_name);
> -
> - return link_pm_policies;
> }
>
> -/**
> - * igt_pm_restore_sata_link_power_management:
> - * @pm_data: An opaque pointer with saved link PM policies;
> - * If NULL is passed we force enable the "max_performance" policy.
> - *
> - * Restore the link power management policies to the values
> - * prior to enabling min_power.
> - *
> - * Caveat: If the system supports hotplugging and hotplugging takes
> - * place during our testing so that the hosts change numbers
> - * we might restore the settings to the wrong hosts.
> - */
> -void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
> -
> +static void __igt_pm_restore_sata_link_power_management(void)
> {
> int fd, i;
> char *file_name;
>
> + if (!__sata_link_pm_policies)
> + return;
> +
> /* Disk runtime PM policies. */
> file_name = malloc(PATH_MAX);
> for (i = 0; ; i++) {
> int8_t policy;
>
> - if (!pm_data)
> + if (!__sata_link_pm_policies)
This can't happen now.
> policy = POLICY_MAX_PERFORMANCE;
> - else if (pm_data[i] == POLICY_UNKNOWN)
> + else if (__sata_link_pm_policies[i] == POLICY_UNKNOWN)
> continue;
> else
> - policy = pm_data[i];
> + policy = __sata_link_pm_policies[i];
>
> snprintf(file_name, PATH_MAX,
> "/sys/class/scsi_host/host%d/link_power_management_policy",
> @@ -421,7 +403,53 @@ void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
> close(fd);
> }
> free(file_name);
> + free(__sata_link_pm_policies);
> + __sata_link_pm_policies = NULL;
> +}
> +
> +/**
> + * igt_pm_enable_sata_link_power_management:
> + *
> + * Enable the min_power policy for SATA link power management.
> + * Without this we cannot reach deep runtime power states.
> + * Returns: Void
> + */
> +void igt_pm_enable_sata_link_power_management(void)
> +{
> + int err;
Unused var.
> +
> + /* Check if has been already saved. */
> + if (__sata_link_pm_policies)
> + return;
> +
> + __igt_pm_enable_sata_link_power_management();
> +}
> +
> +/**
> + * igt_pm_restore_sata_link_power_management:
> + *
> + * Restore the link power management policies to the values
> + * prior to enabling min_power.
> + *
> + * Caveat: If the system supports hotplugging and hotplugging takes
> + * place during our testing so that the hosts change numbers
> + * we might restore the settings to the wrong hosts.
> + */
> +void igt_pm_restore_sata_link_power_management(void)
> +{
> + int err;
Ditto.
> +
> + if (!__sata_link_pm_policies)
> + return;
> +
> + __igt_pm_restore_sata_link_power_management();
> }
> +
> +static void __igt_pm_sata_link_pm_exit_handler(int sig)
> +{
> + __igt_pm_restore_sata_link_power_management();
> +}
> +
> #define POWER_DIR "/sys/devices/pci0000:00/0000:00:02.0/power"
> /* We just leak this on exit ... */
> int pm_status_fd = -1;
> @@ -585,6 +613,61 @@ bool igt_setup_runtime_pm(void)
> return true;
> }
>
> +bool igt_disable_runtime_pm(void)
> +{
> + int fd;
> + ssize_t size;
> + char buf[6];
> +
> + if (pm_status_fd < 0) {
> + fd = open(POWER_DIR "/autosuspend_delay_ms", O_RDWR);
> + if (fd < 0)
> + return false;
> +
> + size = read(fd, __igt_pm_runtime_autosuspend,
> + sizeof(__igt_pm_runtime_autosuspend));
> +
> + if (size <= 0) {
> + close(fd);
> + return false;
> + }
> +
> + strchomp(__igt_pm_runtime_autosuspend);
> + igt_install_exit_handler(__igt_pm_runtime_exit_handler);
> + close(fd);
> + }
> +
> + /* We know we support runtime PM, let's try to disable it now. */
> + fd = open(POWER_DIR "/control", O_RDWR);
> + igt_assert_f(fd >= 0, "Can't open " POWER_DIR "/control\n");
> +
> + if (pm_status_fd < 0) {
> + igt_assert(read(fd, __igt_pm_runtime_control,
> + sizeof(__igt_pm_runtime_control)) > 0);
> + strchomp(__igt_pm_runtime_control);
> +
> + igt_debug("Saved runtime power management as '%s' and '%s'\n",
> + __igt_pm_runtime_autosuspend,
> + __igt_pm_runtime_control);
> + }
> +
> + size = write(fd, "on\n", 3);
> + igt_assert(size == 3);
> + lseek(fd, 0, SEEK_SET);
> + size = read(fd, buf, ARRAY_SIZE(buf));
> + igt_assert(size == 3);
> + igt_assert(strncmp(buf, "on\n", 3) == 0);
> + close(fd);
> +
> + if (pm_status_fd < 0) {
> + pm_status_fd = open(POWER_DIR "/runtime_status", O_RDONLY);
> + igt_assert_f(pm_status_fd >= 0,
> + "Can't open " POWER_DIR "/runtime_status\n");
> + }
> +
> + return true;
> +}
> +
> /**
> * igt_get_runtime_pm_status:
> *
> @@ -628,3 +711,30 @@ bool igt_wait_for_pm_status(enum igt_runtime_pm_status status)
> {
> return igt_wait(igt_get_runtime_pm_status() == status, 10000, 100);
> }
> +
> +/**
> + * dmc_loaded:
> + * @debugfs: fd to the debugfs dir.
> +
> + * Check whether DMC FW is loaded or not. DMC FW is require for few Display C
> + * states like DC5 and DC6. FW does the Context Save and Restore during Display
> + * C States entry and exit.
> + *
> + * Returns:
> + * True if DMC FW is loaded otherwise false.
> + */
> +bool igt_pm_dmc_loaded(int debugfs)
> +{
> + igt_require(debugfs != -1);
This is misplaced and probably not needed.
> + char buf[15];
> + int len;
> +
> + len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
> + if (len < 0)
> + return true; /* no CSR support, no DMC requirement */
> +
> + buf[len] = '\0';
> +
> + igt_info("DMC: %s\n", buf);
> + return strstr(buf, "fw loaded: yes");
> +}
> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> index 10cc6794..979b3463 100644
> --- a/lib/igt_pm.h
> +++ b/lib/igt_pm.h
> @@ -25,8 +25,8 @@
> #define IGT_PM_H
>
> void igt_pm_enable_audio_runtime_pm(void);
> -int8_t *igt_pm_enable_sata_link_power_management(void);
> -void igt_pm_restore_sata_link_power_management(int8_t *pm_data);
> +void igt_pm_enable_sata_link_power_management(void);
> +void igt_pm_restore_sata_link_power_management(void);
>
> /**
> * igt_runtime_pm_status:
> @@ -47,8 +47,10 @@ enum igt_runtime_pm_status {
> };
>
> bool igt_setup_runtime_pm(void);
> +bool igt_disable_runtime_pm(void);
> void igt_restore_runtime_pm(void);
> enum igt_runtime_pm_status igt_get_runtime_pm_status(void);
> bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
> +bool igt_pm_dmc_loaded(int debugfs);
>
> #endif /* IGT_PM_H */
> diff --git a/tests/i915/i915_pm_backlight.c b/tests/i915/i915_pm_backlight.c
> index 4c1bff5b..9a5f4c37 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -47,7 +47,6 @@ struct context {
> #define FADESPEED 100 /* milliseconds between steps */
>
> IGT_TEST_DESCRIPTION("Basic backlight sysfs test");
> -static int8_t *pm_data = NULL;
>
> static int backlight_read(int *result, const char *fname)
> {
> @@ -235,7 +234,7 @@ igt_main
> igt_plane_set_fb(primary, &fb);
>
> igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> - pm_data = igt_pm_enable_sata_link_power_management();
> + igt_pm_enable_sata_link_power_management();
> }
>
> igt_subtest("basic-brightness")
> @@ -255,8 +254,7 @@ igt_main
>
> igt_display_fini(&display);
> igt_remove_fb(display.drm_fd, &fb);
> - igt_pm_restore_sata_link_power_management(pm_data);
> - free(pm_data);
> + igt_pm_restore_sata_link_power_management();
> close(display.drm_fd);
> }
> }
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index a2c9d0ed..2d2f77be 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -122,8 +122,6 @@ struct modeset_params lpsp_mode_params;
> struct modeset_params non_lpsp_mode_params;
> struct modeset_params *default_mode_params;
>
> -static int8_t *pm_data = NULL;
> -
> static int modprobe(const char *driver)
> {
> return igt_kmod_load(driver, NULL);
> @@ -710,21 +708,6 @@ static void setup_pc8(void)
> has_pc8 = true;
> }
>
> -static bool dmc_loaded(void)
> -{
> - char buf[15];
> - int len;
> -
> - len = igt_sysfs_read(debugfs, "i915_dmc_info", buf, sizeof(buf) - 1);
> - if (len < 0)
> - return true; /* no CSR support, no DMC requirement */
> -
> - buf[len] = '\0';
> -
> - igt_info("DMC: %s\n", buf);
> - return strstr(buf, "fw loaded: yes");
> -}
> -
> static void dump_file(int dir, const char *filename)
> {
> char *contents;
> @@ -751,7 +734,7 @@ static bool setup_environment(void)
>
> init_mode_set_data(&ms_data);
>
> - pm_data = igt_pm_enable_sata_link_power_management();
> + igt_pm_enable_sata_link_power_management();
>
> has_runtime_pm = igt_setup_runtime_pm();
> setup_pc8();
> @@ -759,7 +742,7 @@ static bool setup_environment(void)
> igt_info("Runtime PM support: %d\n", has_runtime_pm);
> igt_info("PC8 residency support: %d\n", has_pc8);
> igt_require(has_runtime_pm);
> - igt_require(dmc_loaded());
> + igt_require(igt_pm_dmc_loaded(debugfs));
>
> out:
> disable_all_screens(&ms_data);
> @@ -776,8 +759,7 @@ static void teardown_environment(void)
>
> igt_restore_runtime_pm();
>
> - igt_pm_restore_sata_link_power_management(pm_data);
> - free(pm_data);
> + igt_pm_restore_sata_link_power_management();
>
> fini_mode_set_data(&ms_data);
>
> --
> 2.21.0
>
More information about the igt-dev
mailing list