On Tue, 20 Oct 2020 at 20:04, Lina Iyer <ilina@xxxxxxxxxxxxxx> 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 pm_genpd_set_next_wake() API for the device to notify
/s/pm_genpd_set_next_wake/dev_pm_genpd_set_next_wakeup
Also, please don't use the word "notifiy", but rather "inform" or
similar - to not confuse things with the kernel notifiers.
There are some more examples below in the patch where "notify" is
used, please have a look at those as well.
> 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@xxxxxxxxxxxxxx>
Other than the nitpicks above, this looks good to me:
Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> ---
> 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 | 36 ++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 8 ++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 743268996336..34b90e77e0cd 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -408,6 +408,41 @@ 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.
> + */
> +int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
> +{
> + struct generic_pm_domain *genpd;
> + struct generic_pm_domain_data *gpd_data;
> + int ret = -EINVAL;
> +
> + genpd = dev_to_genpd_safe(dev);
> + if (!genpd)
> + return -ENODEV;
> +
> + if (WARN_ON(!dev->power.subsys_data ||
> + !dev->power.subsys_data->domain_data))
> + return ret;
> +
> + genpd_lock(genpd);
> + gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> + if (ktime_before(ktime_get(), next)) {
> + gpd_data->next_wakeup = next;
> + ret = 0;
> + }
> + genpd_unlock(genpd);
> +
> + return ret;
> +}
> +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;
> @@ -1431,6 +1466,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 1ad0ec481416..e00c77b1efd8 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);
> +int 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,11 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev)
> return -ENOTSUPP;
> }
>
> +static inline int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
> +{
> + return -ENOTSUPP;
> +}
> +
> #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
>
