* [PATCH v7 0/2] Better domain idle from device wakeup patterns @ 2021-01-13 20:15 Lina Iyer 2021-01-13 20:16 ` [PATCH v7 1/2] PM / domains: inform PM domain of a device's next wakeup Lina Iyer 2021-01-13 20:16 ` [PATCH v7 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer 0 siblings, 2 replies; 5+ messages in thread From: Lina Iyer @ 2021-01-13 20:15 UTC (permalink / raw) To: ulf.hansson, rjw; +Cc: linux-pm, linux-arm-msm, Lina Iyer Changes since v6 [6]; - Based on discussions on [6], this update simplifies the next wakeup of domains based on genpd flag GENPD_FLAG_MIN_RESIDENCY specified at init. - Assume next wakeup will be set by devices when the domain is not powered down. This could avoid locking requirements. - Update commit text. Changes since v5 [5]: - It was pointed out that we don't want to run through the unnecessary work for domains that do not need or support next-wakeup. So, patch #1 in this version, now uses a flag to detemine if the domain would support next-wakeup. - Other review comments addressed in patches #2, #3 Changes since v4 [4]: - Address review comments Changes since v3 [3]: - Move the next_wakeup info of the device deeper into the device's domain data. This should avoid overhead for devices that do not have a predictable wakeup pattern. Changes since v2: - Fix unwanted change Changes since v1 [2]: - Update documentation and commit text - Remove check for runtime PM when setting next_event - Fix kernel-test robot reported issue Changes since RFC [1]: - Organized the code to make it cleaner - Fixed some issues with idle state determination - Add documentation and update commit text Hello, I was looking for an option to do better power management for some domains where the devices enter runtime PM in a predictable fashion. For example a display device that sends a vsync interrupt every 16 ms for a 60 Hz panel. These interrupts are not timer interrupts but tend to interrupt periodically to service the workflow and the devices and domains may go back to idle soon after. Two domains are affected by this - the device's PM domain and the CPU PM domain. As a first step, I am looking to solve for the device's PM domain idle state (and hopefully solve for the CPU PM domains subsequently). The PM domain could have multiple idle states and/or the enter/exit latencies could be high. In either case, it may not always be beneficial to power off the domain, only to turn it back on before satisfying the idle state residency. When the wakeup is known for the device, we could use that to determine the worthiness of entering a domain idle state. Only the device can tell us when the future event would be and that could change as the usecase changes. Like, when the panel refresh rate increases to 120 Hz. If this information was made available to runtime PM, we could use that in the domain governor to determine a suitable idle state. This is the idea behind these patches. Would appreciate your thoughts on this. Thanks, Lina [1]. https://lore.kernel.org/linux-pm/010101746eccb270-05beb27f-e1e4-40eb-92da-ad1bb48feb41-000000@us-west-2.amazonses.com/T / [2]. https://lore.kernel.org/linux-pm/20201012223400.23609-3-ilina@codeaurora.org/T/#u [3]. https://lore.kernel.org/linux-pm/20201015193807.17423-1-ilina@codeaurora.org/ [4]. https://www.spinics.net/lists/linux-arm-msm/msg74322.html [5]. https://lore.kernel.org/linux-pm/20201106164811.3698-1-ilina@codeaurora.org/T/#t [6]. https://lore.kernel.org/linux-pm/20201130225039.15981-1-ilina@codeaurora.org/T/#t Lina Iyer (2): PM / domains: inform PM domain of a device's next wakeup PM / Domains: use device's next wakeup to determine domain idle state drivers/base/power/domain.c | 22 ++++++ drivers/base/power/domain_governor.c | 102 ++++++++++++++++++++++++--- include/linux/pm_domain.h | 24 +++++-- 3 files changed, 133 insertions(+), 15 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v7 1/2] PM / domains: inform PM domain of a device's next wakeup 2021-01-13 20:15 [PATCH v7 0/2] Better domain idle from device wakeup patterns Lina Iyer @ 2021-01-13 20:16 ` Lina Iyer 2021-01-15 12:30 ` Ulf Hansson 2021-01-13 20:16 ` [PATCH v7 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer 1 sibling, 1 reply; 5+ messages in thread From: Lina Iyer @ 2021-01-13 20:16 UTC (permalink / raw) To: ulf.hansson, rjw; +Cc: linux-pm, linux-arm-msm, Lina Iyer Some devices may have a predictable interrupt pattern while executing usecases. An example would be the VSYNC interrupt associated with display devices. A 60 Hz display could cause a interrupt every 16 ms. If the device were in a PM domain, the domain would need to be powered up for device to resume and handle the interrupt. Entering a domain idle state saves power, only if the residency of the idle state is met. Without knowing the idle duration of the domain, the governor would just choose the deepest idle state that matches the QoS requirements. The domain might be powered off just as the device is expecting to wake up. If devices could inform PM frameworks of their next event, the parent PM domain's idle duration can be determined. So let's add the dev_pm_genpd_set_next_wakeup() API for the device to inform PM domains of the impending wakeup. This information will be the domain governor to determine the best idle state given the wakeup. Signed-off-by: Lina Iyer <ilina@codeaurora.org> --- Changes in v7: - Simplify and set next-wakeup locklessly Changes in v6: - Update documentation Changes in v5: - Fix commit text as pointed by Ulf - Use -EOPNOTSUPP Changes in v4: - Use PM domain data to store next_wakeup - Drop runtime PM documentation Changes in v3: - Fix unwanted change Changes in v2: - Update documentation - Remove runtime PM enabled check - Update commit text --- drivers/base/power/domain.c | 22 ++++++++++++++++++++++ include/linux/pm_domain.h | 6 ++++++ 2 files changed, 28 insertions(+) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 9a14eedacb92..1a6174babe94 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -423,6 +423,27 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) } EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); +/** + * dev_pm_genpd_set_next_wakeup - Notify PM framework of an impending wakeup. + * + * @dev: Device to handle + * @next: impending interrupt/wakeup for the device + * + * Allow devices to inform of the next wakeup. But, if the domain were already + * powered off, we will not wakeup the domain to recompute it's idle duration. + * Although devices are expected to update the next_wakeup after the end of + * their usecase as well, it is possible the devices themselves may not know + * about that. Stale @next will be ignored when powering off the domain. + */ +void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) +{ + struct generic_pm_domain_data *gpd_data; + + gpd_data = to_gpd_data(dev->power.subsys_data->domain_data); + gpd_data->next_wakeup = next; +} +EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup); + static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) { unsigned int state_idx = genpd->state_idx; @@ -1465,6 +1486,7 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev) gpd_data->td.constraint_changed = true; gpd_data->td.effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS; gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; + gpd_data->next_wakeup = KTIME_MAX; spin_lock_irq(&dev->power.lock); diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 2ca919ae8d36..735583c0bc6d 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -9,6 +9,7 @@ #define _LINUX_PM_DOMAIN_H #include <linux/device.h> +#include <linux/ktime.h> #include <linux/mutex.h> #include <linux/pm.h> #include <linux/err.h> @@ -191,6 +192,7 @@ struct generic_pm_domain_data { struct notifier_block *power_nb; int cpu; unsigned int performance_state; + ktime_t next_wakeup; void *data; }; @@ -217,6 +219,7 @@ int pm_genpd_remove(struct generic_pm_domain *genpd); int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb); int dev_pm_genpd_remove_notifier(struct device *dev); +void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next); extern struct dev_power_governor simple_qos_governor; extern struct dev_power_governor pm_domain_always_on_gov; @@ -275,6 +278,9 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev) return -EOPNOTSUPP; } +static inline void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) +{ } + #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) #endif -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v7 1/2] PM / domains: inform PM domain of a device's next wakeup 2021-01-13 20:16 ` [PATCH v7 1/2] PM / domains: inform PM domain of a device's next wakeup Lina Iyer @ 2021-01-15 12:30 ` Ulf Hansson 0 siblings, 0 replies; 5+ messages in thread From: Ulf Hansson @ 2021-01-15 12:30 UTC (permalink / raw) To: Lina Iyer; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm On Wed, 13 Jan 2021 at 21:16, Lina Iyer <ilina@codeaurora.org> wrote: > > Some devices may have a predictable interrupt pattern while executing > usecases. An example would be the VSYNC interrupt associated with > display devices. A 60 Hz display could cause a interrupt every 16 ms. If > the device were in a PM domain, the domain would need to be powered up > for device to resume and handle the interrupt. > > Entering a domain idle state saves power, only if the residency of the > idle state is met. Without knowing the idle duration of the domain, the > governor would just choose the deepest idle state that matches the QoS > requirements. The domain might be powered off just as the device is > expecting to wake up. If devices could inform PM frameworks of their > next event, the parent PM domain's idle duration can be determined. > > So let's add the dev_pm_genpd_set_next_wakeup() API for the device to > inform PM domains of the impending wakeup. This information will be the > domain governor to determine the best idle state given the wakeup. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> One comment below about the documentation of the new interface. WIth that fixed, please add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > Changes in v7: > - Simplify and set next-wakeup locklessly > Changes in v6: > - Update documentation > Changes in v5: > - Fix commit text as pointed by Ulf > - Use -EOPNOTSUPP > Changes in v4: > - Use PM domain data to store next_wakeup > - Drop runtime PM documentation > Changes in v3: > - Fix unwanted change > Changes in v2: > - Update documentation > - Remove runtime PM enabled check > - Update commit text > --- > drivers/base/power/domain.c | 22 ++++++++++++++++++++++ > include/linux/pm_domain.h | 6 ++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 9a14eedacb92..1a6174babe94 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -423,6 +423,27 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state) > } > EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state); > > +/** > + * dev_pm_genpd_set_next_wakeup - Notify PM framework of an impending wakeup. > + * > + * @dev: Device to handle > + * @next: impending interrupt/wakeup for the device > + * > + * Allow devices to inform of the next wakeup. But, if the domain were already > + * powered off, we will not wakeup the domain to recompute it's idle duration. > + * Although devices are expected to update the next_wakeup after the end of > + * their usecase as well, it is possible the devices themselves may not know > + * about that. Stale @next will be ignored when powering off the domain. Sorry for nagging (same comment as for the previous version), but could you please extend the above with the below text, as to make it more clear: "It's assumed that the users guarantee that the genpd wouldn't be detached while this routine is getting called. Additionally, it's also assumed that @dev isn't runtime suspended (RPM_SUSPENDED)." With this, you don't need to mention the thing with the state of the PM domain, as it can't be powered off when this function is called. > + */ > +void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) > +{ > + struct generic_pm_domain_data *gpd_data; > + > + gpd_data = to_gpd_data(dev->power.subsys_data->domain_data); > + gpd_data->next_wakeup = next; > +} > +EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup); > + > static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > { > unsigned int state_idx = genpd->state_idx; > @@ -1465,6 +1486,7 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev) > gpd_data->td.constraint_changed = true; > gpd_data->td.effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS; > gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier; > + gpd_data->next_wakeup = KTIME_MAX; > > spin_lock_irq(&dev->power.lock); > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 2ca919ae8d36..735583c0bc6d 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -9,6 +9,7 @@ > #define _LINUX_PM_DOMAIN_H > > #include <linux/device.h> > +#include <linux/ktime.h> > #include <linux/mutex.h> > #include <linux/pm.h> > #include <linux/err.h> > @@ -191,6 +192,7 @@ struct generic_pm_domain_data { > struct notifier_block *power_nb; > int cpu; > unsigned int performance_state; > + ktime_t next_wakeup; > void *data; > }; > > @@ -217,6 +219,7 @@ int pm_genpd_remove(struct generic_pm_domain *genpd); > int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state); > int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb); > int dev_pm_genpd_remove_notifier(struct device *dev); > +void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next); > > extern struct dev_power_governor simple_qos_governor; > extern struct dev_power_governor pm_domain_always_on_gov; > @@ -275,6 +278,9 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev) > return -EOPNOTSUPP; > } > > +static inline void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) > +{ } > + > #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) > #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) > #endif > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v7 2/2] PM / Domains: use device's next wakeup to determine domain idle state 2021-01-13 20:15 [PATCH v7 0/2] Better domain idle from device wakeup patterns Lina Iyer 2021-01-13 20:16 ` [PATCH v7 1/2] PM / domains: inform PM domain of a device's next wakeup Lina Iyer @ 2021-01-13 20:16 ` Lina Iyer 2021-01-15 12:35 ` Ulf Hansson 1 sibling, 1 reply; 5+ messages in thread From: Lina Iyer @ 2021-01-13 20:16 UTC (permalink / raw) To: ulf.hansson, rjw; +Cc: linux-pm, linux-arm-msm, Lina Iyer Currently, a PM domain's idle state is determined based on whether the QoS requirements are met. However, even entering an idle state may waste power if the minimum residency requirements aren't fulfilled. CPU PM domains use the next timer wakeup for the CPUs in the domain to determine the sleep duration of the domain. This is compared with the idle state residencies to determine the optimal idle state. For other PM domains, determining the sleep length is not that straight forward. But if the device's next_event is available, we can use that to determine the sleep duration of the PM domain. Let's update the domain governor logic to check for idle state residency based on the next wakeup of devices as well as QoS constraints. But since, not all domains may contain devices capable of specifying the next wakeup, let's enable this additional check only if specified by the domain's flags when initializing the domain. Signed-off-by: Lina Iyer <ilina@codeaurora.org> --- Changes in v7: - Define GENPD_FLAG_MIN_RESIDENCY and check for min residency only if the flag is set. - Update commit text. Changes in v6: - Do not include power_on_latency_ns for next_wakeup determination. - Re-organize code to avoid multiple ktime_get() reads. - Check genpd flag if next_wakeup is useful for the domain. - Document why we ignore stale data Changes in v5: - Minor code changes suggested by Rafel Changes in v4: - Update to use next_wakeup from struct generic_pm_domain_data. Changes in v3: - None Changes in v2: - Fix state_idx type to hold negative value. - Update commit text. --- drivers/base/power/domain_governor.c | 102 ++++++++++++++++++++++++--- include/linux/pm_domain.h | 18 +++-- 2 files changed, 105 insertions(+), 15 deletions(-) diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c index 490ed7deb99a..c6c218758f0b 100644 --- a/drivers/base/power/domain_governor.c +++ b/drivers/base/power/domain_governor.c @@ -117,6 +117,55 @@ static bool default_suspend_ok(struct device *dev) return td->cached_suspend_ok; } +static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now) +{ + ktime_t domain_wakeup = KTIME_MAX; + ktime_t next_wakeup; + struct pm_domain_data *pdd; + struct gpd_link *link; + + if (!(genpd->flags & GENPD_FLAG_MIN_RESIDENCY)) + return; + + /* + * Devices that have a predictable wakeup pattern, may specify + * their next wakeup. Let's find the next wakeup from all the + * devices attached to this domain and from all the sub-domains. + * It is possible that component's a next wakeup may have become + * stale when we read that here. We will ignore to ensure the domain + * is able to enter its optimal idle state. + */ + list_for_each_entry(pdd, &genpd->dev_list, list_node) { + next_wakeup = to_gpd_data(pdd)->next_wakeup; + if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now)) + if (ktime_before(next_wakeup, domain_wakeup)) + domain_wakeup = next_wakeup; + } + + list_for_each_entry(link, &genpd->parent_links, parent_node) { + next_wakeup = link->child->next_wakeup; + if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now)) + if (ktime_before(next_wakeup, domain_wakeup)) + domain_wakeup = next_wakeup; + } + + genpd->next_wakeup = domain_wakeup; +} + +static bool next_wakeup_allows_state(struct generic_pm_domain *genpd, + unsigned int state, ktime_t now) +{ + ktime_t domain_wakeup = genpd->next_wakeup; + s64 idle_time_ns, min_sleep_ns; + + min_sleep_ns = genpd->states[state].power_off_latency_ns + + genpd->states[state].residency_ns; + + idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now)); + + return idle_time_ns >= min_sleep_ns; +} + static bool __default_power_down_ok(struct dev_pm_domain *pd, unsigned int state) { @@ -201,16 +250,41 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd, } /** - * default_power_down_ok - Default generic PM domain power off governor routine. + * _default_power_down_ok - Default generic PM domain power off governor routine. * @pd: PM domain to check. * * This routine must be executed under the PM domain's lock. */ -static bool default_power_down_ok(struct dev_pm_domain *pd) +static bool _default_power_down_ok(struct dev_pm_domain *pd, ktime_t now) { struct generic_pm_domain *genpd = pd_to_genpd(pd); + int state_idx = genpd->state_count - 1; struct gpd_link *link; + /* + * Find the next wakeup from devices that can determine their own wakeup + * to find when the domain would wakeup and do it for every device down + * the hierarchy. It is not worth while to sleep if the state's residency + * cannot be met. + */ + update_domain_next_wakeup(genpd, now); + if ((genpd->flags & GENPD_FLAG_MIN_RESIDENCY) && (genpd->next_wakeup != KTIME_MAX)) { + /* Let's find out the deepest domain idle state, the devices prefer */ + while (state_idx >= 0) { + if (next_wakeup_allows_state(genpd, state_idx, now)) { + genpd->max_off_time_changed = true; + break; + } + state_idx--; + } + + if (state_idx < 0) { + state_idx = 0; + genpd->cached_power_down_ok = false; + goto done; + } + } + if (!genpd->max_off_time_changed) { genpd->state_idx = genpd->cached_power_down_state_idx; return genpd->cached_power_down_ok; @@ -228,21 +302,30 @@ static bool default_power_down_ok(struct dev_pm_domain *pd) genpd->max_off_time_ns = -1; genpd->max_off_time_changed = false; genpd->cached_power_down_ok = true; - genpd->state_idx = genpd->state_count - 1; - /* Find a state to power down to, starting from the deepest. */ - while (!__default_power_down_ok(pd, genpd->state_idx)) { - if (genpd->state_idx == 0) { + /* + * Find a state to power down to, starting from the state + * determined by the next wakeup. + */ + while (!__default_power_down_ok(pd, state_idx)) { + if (state_idx == 0) { genpd->cached_power_down_ok = false; break; } - genpd->state_idx--; + state_idx--; } +done: + genpd->state_idx = state_idx; genpd->cached_power_down_state_idx = genpd->state_idx; return genpd->cached_power_down_ok; } +static bool default_power_down_ok(struct dev_pm_domain *pd) +{ + return _default_power_down_ok(pd, ktime_get()); +} + static bool always_on_power_down_ok(struct dev_pm_domain *domain) { return false; @@ -254,11 +337,12 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd) struct generic_pm_domain *genpd = pd_to_genpd(pd); struct cpuidle_device *dev; ktime_t domain_wakeup, next_hrtimer; + ktime_t now = ktime_get(); s64 idle_duration_ns; int cpu, i; /* Validate dev PM QoS constraints. */ - if (!default_power_down_ok(pd)) + if (!_default_power_down_ok(pd, now)) return false; if (!(genpd->flags & GENPD_FLAG_CPU_DOMAIN)) @@ -280,7 +364,7 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd) } /* The minimum idle duration is from now - until the next wakeup. */ - idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, ktime_get())); + idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, now)); if (idle_duration_ns <= 0) return false; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index 735583c0bc6d..8ff3cac87f88 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -56,13 +56,18 @@ * * GENPD_FLAG_RPM_ALWAYS_ON: Instructs genpd to always keep the PM domain * powered on except for system suspend. + * + * GENPD_FLAG_MIN_RESIDENCY: Enable the genpd governor to consider its + * components' next wakeup when determining the + * optimal idle state. */ -#define GENPD_FLAG_PM_CLK (1U << 0) -#define GENPD_FLAG_IRQ_SAFE (1U << 1) -#define GENPD_FLAG_ALWAYS_ON (1U << 2) -#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) -#define GENPD_FLAG_CPU_DOMAIN (1U << 4) -#define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5) +#define GENPD_FLAG_PM_CLK (1U << 0) +#define GENPD_FLAG_IRQ_SAFE (1U << 1) +#define GENPD_FLAG_ALWAYS_ON (1U << 2) +#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) +#define GENPD_FLAG_CPU_DOMAIN (1U << 4) +#define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5) +#define GENPD_FLAG_MIN_RESIDENCY (1U << 6) enum gpd_status { GENPD_STATE_ON = 0, /* PM domain is on */ @@ -130,6 +135,7 @@ struct generic_pm_domain { unsigned int state); struct gpd_dev_ops dev_ops; s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ + ktime_t next_wakeup; /* Maintained by the domain governor */ bool max_off_time_changed; bool cached_power_down_ok; bool cached_power_down_state_idx; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v7 2/2] PM / Domains: use device's next wakeup to determine domain idle state 2021-01-13 20:16 ` [PATCH v7 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer @ 2021-01-15 12:35 ` Ulf Hansson 0 siblings, 0 replies; 5+ messages in thread From: Ulf Hansson @ 2021-01-15 12:35 UTC (permalink / raw) To: Lina Iyer; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm On Wed, 13 Jan 2021 at 21:16, Lina Iyer <ilina@codeaurora.org> wrote: > > Currently, a PM domain's idle state is determined based on whether the > QoS requirements are met. However, even entering an idle state may waste > power if the minimum residency requirements aren't fulfilled. > > CPU PM domains use the next timer wakeup for the CPUs in the domain to > determine the sleep duration of the domain. This is compared with the > idle state residencies to determine the optimal idle state. For other PM > domains, determining the sleep length is not that straight forward. But > if the device's next_event is available, we can use that to determine > the sleep duration of the PM domain. > > Let's update the domain governor logic to check for idle state residency > based on the next wakeup of devices as well as QoS constraints. But > since, not all domains may contain devices capable of specifying the > next wakeup, let's enable this additional check only if specified by the > domain's flags when initializing the domain. > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> A minor comment about white spaces, see below. With that fixed, please add: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > Changes in v7: > - Define GENPD_FLAG_MIN_RESIDENCY and check for min residency > only if the flag is set. > - Update commit text. > Changes in v6: > - Do not include power_on_latency_ns for next_wakeup > determination. > - Re-organize code to avoid multiple ktime_get() reads. > - Check genpd flag if next_wakeup is useful for the domain. > - Document why we ignore stale data > Changes in v5: > - Minor code changes suggested by Rafel > Changes in v4: > - Update to use next_wakeup from struct generic_pm_domain_data. > Changes in v3: > - None > Changes in v2: > - Fix state_idx type to hold negative value. > - Update commit text. > --- > drivers/base/power/domain_governor.c | 102 ++++++++++++++++++++++++--- > include/linux/pm_domain.h | 18 +++-- > 2 files changed, 105 insertions(+), 15 deletions(-) > > diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c > index 490ed7deb99a..c6c218758f0b 100644 > --- a/drivers/base/power/domain_governor.c > +++ b/drivers/base/power/domain_governor.c > @@ -117,6 +117,55 @@ static bool default_suspend_ok(struct device *dev) > return td->cached_suspend_ok; > } > > +static void update_domain_next_wakeup(struct generic_pm_domain *genpd, ktime_t now) > +{ > + ktime_t domain_wakeup = KTIME_MAX; > + ktime_t next_wakeup; > + struct pm_domain_data *pdd; > + struct gpd_link *link; > + > + if (!(genpd->flags & GENPD_FLAG_MIN_RESIDENCY)) > + return; > + > + /* > + * Devices that have a predictable wakeup pattern, may specify > + * their next wakeup. Let's find the next wakeup from all the > + * devices attached to this domain and from all the sub-domains. > + * It is possible that component's a next wakeup may have become > + * stale when we read that here. We will ignore to ensure the domain > + * is able to enter its optimal idle state. > + */ > + list_for_each_entry(pdd, &genpd->dev_list, list_node) { > + next_wakeup = to_gpd_data(pdd)->next_wakeup; > + if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now)) > + if (ktime_before(next_wakeup, domain_wakeup)) > + domain_wakeup = next_wakeup; > + } > + > + list_for_each_entry(link, &genpd->parent_links, parent_node) { > + next_wakeup = link->child->next_wakeup; > + if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now)) > + if (ktime_before(next_wakeup, domain_wakeup)) > + domain_wakeup = next_wakeup; > + } > + > + genpd->next_wakeup = domain_wakeup; > +} > + > +static bool next_wakeup_allows_state(struct generic_pm_domain *genpd, > + unsigned int state, ktime_t now) > +{ > + ktime_t domain_wakeup = genpd->next_wakeup; > + s64 idle_time_ns, min_sleep_ns; > + > + min_sleep_ns = genpd->states[state].power_off_latency_ns + > + genpd->states[state].residency_ns; > + > + idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now)); > + > + return idle_time_ns >= min_sleep_ns; > +} > + > static bool __default_power_down_ok(struct dev_pm_domain *pd, > unsigned int state) > { > @@ -201,16 +250,41 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd, > } > > /** > - * default_power_down_ok - Default generic PM domain power off governor routine. > + * _default_power_down_ok - Default generic PM domain power off governor routine. > * @pd: PM domain to check. > * > * This routine must be executed under the PM domain's lock. > */ > -static bool default_power_down_ok(struct dev_pm_domain *pd) > +static bool _default_power_down_ok(struct dev_pm_domain *pd, ktime_t now) > { > struct generic_pm_domain *genpd = pd_to_genpd(pd); > + int state_idx = genpd->state_count - 1; > struct gpd_link *link; > > + /* > + * Find the next wakeup from devices that can determine their own wakeup > + * to find when the domain would wakeup and do it for every device down > + * the hierarchy. It is not worth while to sleep if the state's residency > + * cannot be met. > + */ > + update_domain_next_wakeup(genpd, now); > + if ((genpd->flags & GENPD_FLAG_MIN_RESIDENCY) && (genpd->next_wakeup != KTIME_MAX)) { > + /* Let's find out the deepest domain idle state, the devices prefer */ > + while (state_idx >= 0) { > + if (next_wakeup_allows_state(genpd, state_idx, now)) { > + genpd->max_off_time_changed = true; > + break; > + } > + state_idx--; > + } > + > + if (state_idx < 0) { > + state_idx = 0; > + genpd->cached_power_down_ok = false; > + goto done; > + } > + } > + > if (!genpd->max_off_time_changed) { > genpd->state_idx = genpd->cached_power_down_state_idx; > return genpd->cached_power_down_ok; > @@ -228,21 +302,30 @@ static bool default_power_down_ok(struct dev_pm_domain *pd) > genpd->max_off_time_ns = -1; > genpd->max_off_time_changed = false; > genpd->cached_power_down_ok = true; > - genpd->state_idx = genpd->state_count - 1; > > - /* Find a state to power down to, starting from the deepest. */ > - while (!__default_power_down_ok(pd, genpd->state_idx)) { > - if (genpd->state_idx == 0) { > + /* > + * Find a state to power down to, starting from the state > + * determined by the next wakeup. > + */ > + while (!__default_power_down_ok(pd, state_idx)) { > + if (state_idx == 0) { > genpd->cached_power_down_ok = false; > break; > } > - genpd->state_idx--; > + state_idx--; > } > > +done: > + genpd->state_idx = state_idx; > genpd->cached_power_down_state_idx = genpd->state_idx; > return genpd->cached_power_down_ok; > } > > +static bool default_power_down_ok(struct dev_pm_domain *pd) > +{ > + return _default_power_down_ok(pd, ktime_get()); > +} > + > static bool always_on_power_down_ok(struct dev_pm_domain *domain) > { > return false; > @@ -254,11 +337,12 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd) > struct generic_pm_domain *genpd = pd_to_genpd(pd); > struct cpuidle_device *dev; > ktime_t domain_wakeup, next_hrtimer; > + ktime_t now = ktime_get(); > s64 idle_duration_ns; > int cpu, i; > > /* Validate dev PM QoS constraints. */ > - if (!default_power_down_ok(pd)) > + if (!_default_power_down_ok(pd, now)) > return false; > > if (!(genpd->flags & GENPD_FLAG_CPU_DOMAIN)) > @@ -280,7 +364,7 @@ static bool cpu_power_down_ok(struct dev_pm_domain *pd) > } > > /* The minimum idle duration is from now - until the next wakeup. */ > - idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, ktime_get())); > + idle_duration_ns = ktime_to_ns(ktime_sub(domain_wakeup, now)); > if (idle_duration_ns <= 0) > return false; > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 735583c0bc6d..8ff3cac87f88 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -56,13 +56,18 @@ > * > * GENPD_FLAG_RPM_ALWAYS_ON: Instructs genpd to always keep the PM domain > * powered on except for system suspend. > + * > + * GENPD_FLAG_MIN_RESIDENCY: Enable the genpd governor to consider its > + * components' next wakeup when determining the > + * optimal idle state. > */ > -#define GENPD_FLAG_PM_CLK (1U << 0) > -#define GENPD_FLAG_IRQ_SAFE (1U << 1) > -#define GENPD_FLAG_ALWAYS_ON (1U << 2) > -#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) > -#define GENPD_FLAG_CPU_DOMAIN (1U << 4) > -#define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5) > +#define GENPD_FLAG_PM_CLK (1U << 0) > +#define GENPD_FLAG_IRQ_SAFE (1U << 1) > +#define GENPD_FLAG_ALWAYS_ON (1U << 2) > +#define GENPD_FLAG_ACTIVE_WAKEUP (1U << 3) > +#define GENPD_FLAG_CPU_DOMAIN (1U << 4) > +#define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5) Looks like the above are white space changes, probably carried forward from previous versions. Please fix it up. > +#define GENPD_FLAG_MIN_RESIDENCY (1U << 6) > > enum gpd_status { > GENPD_STATE_ON = 0, /* PM domain is on */ > @@ -130,6 +135,7 @@ struct generic_pm_domain { > unsigned int state); > struct gpd_dev_ops dev_ops; > s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ > + ktime_t next_wakeup; /* Maintained by the domain governor */ > bool max_off_time_changed; > bool cached_power_down_ok; > bool cached_power_down_state_idx; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-15 12:49 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-13 20:15 [PATCH v7 0/2] Better domain idle from device wakeup patterns Lina Iyer 2021-01-13 20:16 ` [PATCH v7 1/2] PM / domains: inform PM domain of a device's next wakeup Lina Iyer 2021-01-15 12:30 ` Ulf Hansson 2021-01-13 20:16 ` [PATCH v7 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer 2021-01-15 12:35 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).