[igt-dev] [PATCH i-g-t v12 1/6] lib/igt_pm: igt lib helper routines to support DC5/6 tests
Petri Latvala
petri.latvala at intel.com
Mon Aug 26 09:52:01 UTC 2019
On Fri, Aug 23, 2019 at 05:25:40PM +0300, Imre Deak wrote:
> On Fri, Aug 16, 2019 at 03:57:34PM +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.
> > v10:Review comment fixup in sata_link_power_management
> > lib functions. [Imre]
> >
> > 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 | 210 +++++++++++++++++++++++++--------
> > lib/igt_pm.h | 6 +-
> > tests/i915/i915_pm_backlight.c | 6 +-
> > tests/i915/i915_pm_rpm.c | 24 +---
> > 4 files changed, 172 insertions(+), 74 deletions(-)
> >
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > index fd22273a..b91a0f96 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,16 @@ 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_pm_policies;
> > +int __scsi_host_cnt;
> >
> > 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,39 +286,26 @@ 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;
> > + int8_t policy;
> >
> > file_name = malloc(PATH_MAX);
> > buf = malloc(MAX_POLICY_STRLEN + 1);
> >
> > - for (i = 0; ; i++) {
> > - int8_t policy;
> > -
> > + for (__scsi_host_cnt = 0; ; __scsi_host_cnt++) {
> > snprintf(file_name, PATH_MAX,
> > "/sys/class/scsi_host/host%d/link_power_management_policy",
> > - i);
> > + __scsi_host_cnt);
> >
> > + /*
> > + * 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;
> > @@ -332,12 +325,26 @@ int8_t *igt_pm_enable_sata_link_power_management(void)
> > else
> > policy = POLICY_UNKNOWN;
> >
> > - if (!(i % 256))
> > - link_pm_policies = realloc(link_pm_policies,
> > - (i / 256 + 1) * 256 + 1);
> > + if (!(__scsi_host_cnt % 256))
> > + __sata_pm_policies = realloc(__sata_pm_policies,
> > + (__scsi_host_cnt / 256 + 1)
> > + * 256 + 1);
> >
> > - link_pm_policies[i] = policy;
> > - link_pm_policies[i + 1] = 0;
> > + __sata_pm_policies[__scsi_host_cnt] = policy;
> > + close(fd);
> > + }
> > +
> > + igt_install_exit_handler(__igt_pm_sata_link_pm_exit_handler);
> > +
> > + for (i = 0; i < __scsi_host_cnt; i++) {
> > + snprintf(file_name, PATH_MAX,
> > + "/sys/class/scsi_host/host%d/link_power_management_policy",
> > + i);
> > + fd = open(file_name, O_RDWR);
> > + if (fd < 0)
> > + break;
> > +
> > + policy = __sata_pm_policies[i];
> >
> > /* If the policy is something we don't know about,
> > * don't touch it, since we might potentially break things.
> > @@ -355,39 +362,25 @@ 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_pm_policies)
> > + return;
> > +
> > /* Disk runtime PM policies. */
> > file_name = malloc(PATH_MAX);
> > - for (i = 0; ; i++) {
> > + for (i = 0; i < __scsi_host_cnt; i++) {
> > int8_t policy;
> >
> > - if (!pm_data)
> > - policy = POLICY_MAX_PERFORMANCE;
> > - else if (pm_data[i] == POLICY_UNKNOWN)
> > + if (__sata_pm_policies[i] == POLICY_UNKNOWN)
> > continue;
> > else
> > - policy = pm_data[i];
> > + policy = __sata_pm_policies[i];
> >
> > snprintf(file_name, PATH_MAX,
> > "/sys/class/scsi_host/host%d/link_power_management_policy",
> > @@ -421,7 +414,49 @@ void igt_pm_restore_sata_link_power_management(int8_t *pm_data)
> > close(fd);
> > }
> > free(file_name);
> > + free(__sata_pm_policies);
> > + __sata_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
>
> No need for a comment on void returns.
>
> > + */
> > +void igt_pm_enable_sata_link_power_management(void)
> > +{
> > + /* Check if has been already saved. */
> > + if (__sata_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)
> > +{
> > + if (!__sata_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 +620,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);
> > + }
>
> We require to call igt_setup_runtime_pm() before
> igt_disable_runtime_pm(), so the original state will be saved away
> already, so instead of the above duplication of saving the state we
> should just igt_assert(pm_status_fd >= 0);
Just a drive-by comment here, use igt_assert_fd(pm_status_fd) instead
of writing it out.
--
Petri Latvala
More information about the igt-dev
mailing list