diff mbox series

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

Message ID 20210115165004.22385-2-ilina@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Better domain idle from device wakeup patterns | expand

Commit Message

Lina Iyer Jan. 15, 2021, 4:50 p.m. UTC
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(+)

Comments

Ulf Hansson Jan. 19, 2021, 10 a.m. UTC | #1
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
Lina Iyer Jan. 19, 2021, 4:05 p.m. UTC | #2
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 mbox series

Patch

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