Re: [PATCH v8 1/2] PM / domains: inform PM domain of a device's next wakeup

[Lina Iyer <ilina@codeaurora.org>]

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