| Message ID | 20210115165004.22385-2-ilina@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Better domain idle from device wakeup patterns | expand |
On Fri, 15 Jan 2021 at 17:50, 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> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > Changes in v8: > - Update documentation. Add Reviewed-by tag. > 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 | 25 +++++++++++++++++++++++++ > include/linux/pm_domain.h | 6 ++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 9a14eedacb92..10a960bd3204 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -423,6 +423,30 @@ 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. 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)." > + * 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, so 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; Looks like you have dropped one of the needed sanity checks, to make sure the device is attached to a genpd. My apologies if I missed that in the previous version. So you need something like this: genpd = dev_to_genpd_safe(dev); if (!genpd) return; > + > + 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 +1489,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 > Kind regards Uffe
On Tue, Jan 19 2021 at 03:01 -0700, Ulf Hansson wrote: >On Fri, 15 Jan 2021 at 17:50, 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> >> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> Changes in v8: >> - Update documentation. Add Reviewed-by tag. >> 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 | 25 +++++++++++++++++++++++++ >> include/linux/pm_domain.h | 6 ++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index 9a14eedacb92..10a960bd3204 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -423,6 +423,30 @@ 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. 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)." >> + * 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, so 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; > >Looks like you have dropped one of the needed sanity checks, to make >sure the device is attached to a genpd. My apologies if I missed that >in the previous version. So you need something like this: > Oops. I assumed that we leave that to the driver to ensure that requirement. I will send another version with this. Thanks for catching this and the review. --Lina >genpd = dev_to_genpd_safe(dev); >if (!genpd) > return; > >> + >> + 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 +1489,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 >> > >Kind regards >Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 9a14eedacb92..10a960bd3204 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -423,6 +423,30 @@ 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. 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)." + * 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, so 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 +1489,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