[PATCH v6 0/3] Better domain idle from device wakeup patterns

[PATCH v6 0/3] Better domain idle from device wakeup patterns

[Lina Iyer]

Hi,

The v5[1] of the series brought out some interesting discussions. The
most important being is it worth adding the additional expense to all PM
domains even if no wakeup pattern is available. It seems like
maintaining a domain specific flag that the governor could check is a
generic enough option. That should disable additional overhead for
domains that do not need this feature.

Ulf suggested that we could allow wakeups only if any of the domain idle
state specifies a residency. However, we don't want to check for next
wakeup everytime the domain enters idle just because the domain
specifies an idle state with residency. This is also not desired.

Also, if the domain checks for next wakeup, should the parent domains of
the domain also check for next wakeup? And when do we set that up? These
are questions that we don't know the answers yet. So, let's enable the
domain governor only if the domain sets up the flag or when the device
in the domain specifies the next wakeup.

The previous post of the series explaining why this is a useful feature
is v5[1]. Please let me know what you think.

Thanks,
Lina

[1]. https://www.spinics.net/lists/linux-arm-msm/msg75555.html

Lina Iyer (3):
  PM / Domains: add domain feature flag for next wakeup
  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          |  59 ++++++++++++++++
 drivers/base/power/domain_governor.c | 102 ++++++++++++++++++++++++---
 include/linux/pm_domain.h            |  37 ++++++++--
 3 files changed, 183 insertions(+), 15 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v6 1/3] PM / Domains: add domain feature flag for next wakeup

[Lina Iyer]

PM domains may support entering multiple power down states when the
component devices and sub-domains are suspended. Also, they may specify
the residency value for an idle state, only after which the idle state
may provide power benefits. If the domain does not specify the residency
for any of its idle states, the governor's choice is much simplified.

Let's make this optional with the use of a PM domain feature flag.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v6:
	- New patch based on discussions on v5 of the series
---
 drivers/base/power/domain.c | 18 ++++++++++++++++++
 include/linux/pm_domain.h   | 28 ++++++++++++++++++++++------
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 1b0c9ccbbe1f..1e6c0bf1c945 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1748,6 +1748,24 @@ int dev_pm_genpd_remove_notifier(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_remove_notifier);
 
+/**
+ * genpd_enable_next_wakeup - Enable genpd gov to use next_wakeup
+ *
+ * @genpd: The genpd to be updated
+ * @enable: Enable/disable genpd gov to use next wakeup
+ */
+void genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable)
+{
+	genpd_lock(genpd);
+	if (enable)
+		genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;
+	else
+		genpd->flags &= ~GENPD_FLAG_GOV_NEXT_WAKEUP;
+	genpd->next_wakeup = KTIME_MAX;
+	genpd_unlock(genpd);
+}
+EXPORT_SYMBOL_GPL(genpd_enable_next_wakeup);
+
 static int genpd_add_subdomain(struct generic_pm_domain *genpd,
 			       struct generic_pm_domain *subdomain)
 {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 2ca919ae8d36..1f359bd19f77 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -55,13 +55,19 @@
  *
  * GENPD_FLAG_RPM_ALWAYS_ON:	Instructs genpd to always keep the PM domain
  *				powered on except for system suspend.
+ *
+ * GENPD_FLAG_GOV_NEXT_WAKEUP:	Enable the genpd governor to consider its
+ *				components' next wakeup when  determining the
+ *				optimal idle state. This is setup only if the
+ *				domain's idle state specifies a residency.
  */
-#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_GOV_NEXT_WAKEUP (1U << 6)
 
 enum gpd_status {
 	GENPD_STATE_ON = 0,	/* PM domain is on */
@@ -205,6 +211,11 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
 	return to_gpd_data(dev->power.subsys_data->domain_data);
 }
 
+static inline bool genpd_may_use_next_wakeup(struct generic_pm_domain *genpd)
+{
+	return genpd->flags & GENPD_FLAG_GOV_NEXT_WAKEUP;
+}
+
 int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
 int pm_genpd_remove_device(struct device *dev);
 int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
@@ -217,6 +228,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 genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable);
 
 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -275,6 +287,10 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev)
 	return -EOPNOTSUPP;
 }
 
+static void genpd_enable_next_wakeup(struct generic_pm_domain *genpd,
+				     bool enable)
+{ }
+
 #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

[PATCH v6 2/3] PM / domains: inform PM domain of a device's next wakeup

[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 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 | 41 +++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  8 ++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 1e6c0bf1c945..191539a8e06d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -408,6 +408,46 @@ 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.
+ */
+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;
+		genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;
+		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;
@@ -1450,6 +1490,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 1f359bd19f77..cc27d3d88849 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>
@@ -197,6 +198,7 @@ struct generic_pm_domain_data {
 	struct notifier_block *power_nb;
 	int cpu;
 	unsigned int performance_state;
+	ktime_t	next_wakeup;
 	void *data;
 };
 
@@ -229,6 +231,7 @@ 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 genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable);
+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;
@@ -291,6 +294,11 @@ static void genpd_enable_next_wakeup(struct generic_pm_domain *genpd,
 				     bool enable)
 { }
 
+static inline int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
+{
+	return -EOPNOTSUPP;
+}
+
 #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

[PATCH v6 3/3] PM / Domains: use device's next wakeup to determine domain idle state

[Lina Iyer]

Currently, a PM domain's idle state is determined based on whether the
QoS requirements are met. This may not save power, if the idle state
residency requirements are not met.

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.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
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            |   1 +
 2 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 490ed7deb99a..2afb7fa90d5d 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_may_use_next_wakeup(genpd))
+		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->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 cc27d3d88849..a41aea9d1c06 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -136,6 +136,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

Re: [PATCH v6 0/3] Better domain idle from device wakeup patterns

[Rafael J. Wysocki]

On Mon, Nov 30, 2020 at 11:51 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
> Hi,
>
> The v5[1] of the series brought out some interesting discussions. The
> most important being is it worth adding the additional expense to all PM
> domains even if no wakeup pattern is available. It seems like
> maintaining a domain specific flag that the governor could check is a
> generic enough option. That should disable additional overhead for
> domains that do not need this feature.
>
> Ulf suggested that we could allow wakeups only if any of the domain idle
> state specifies a residency. However, we don't want to check for next
> wakeup everytime the domain enters idle just because the domain
> specifies an idle state with residency. This is also not desired.
>
> Also, if the domain checks for next wakeup, should the parent domains of
> the domain also check for next wakeup? And when do we set that up? These
> are questions that we don't know the answers yet. So, let's enable the
> domain governor only if the domain sets up the flag or when the device
> in the domain specifies the next wakeup.
>
> The previous post of the series explaining why this is a useful feature
> is v5[1]. Please let me know what you think.

Ulf had comments on the previous versions, so waiting for him to
respond here, thanks!

Re: [PATCH v6 0/3] Better domain idle from device wakeup patterns

[Ulf Hansson]

On Tue, 8 Dec 2020 at 18:26, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Nov 30, 2020 at 11:51 PM Lina Iyer <ilina@codeaurora.org> wrote:
> >
> > Hi,
> >
> > The v5[1] of the series brought out some interesting discussions. The
> > most important being is it worth adding the additional expense to all PM
> > domains even if no wakeup pattern is available. It seems like
> > maintaining a domain specific flag that the governor could check is a
> > generic enough option. That should disable additional overhead for
> > domains that do not need this feature.
> >
> > Ulf suggested that we could allow wakeups only if any of the domain idle
> > state specifies a residency. However, we don't want to check for next
> > wakeup everytime the domain enters idle just because the domain
> > specifies an idle state with residency. This is also not desired.
> >
> > Also, if the domain checks for next wakeup, should the parent domains of
> > the domain also check for next wakeup? And when do we set that up? These
> > are questions that we don't know the answers yet. So, let's enable the
> > domain governor only if the domain sets up the flag or when the device
> > in the domain specifies the next wakeup.
> >
> > The previous post of the series explaining why this is a useful feature
> > is v5[1]. Please let me know what you think.
>
> Ulf had comments on the previous versions, so waiting for him to
> respond here, thanks!

Yes, I will have a look, but please allow me some more time - it's a
busy period for me.

Kind regards
Uffe

Re: [PATCH v6 0/3] Better domain idle from device wakeup patterns

[Lina Iyer]

On Wed, Dec 09 2020 at 03:37 -0700, Ulf Hansson wrote:
>On Tue, 8 Dec 2020 at 18:26, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Mon, Nov 30, 2020 at 11:51 PM Lina Iyer <ilina@codeaurora.org> wrote:
>> >
>> > Hi,
>> >
>> > The v5[1] of the series brought out some interesting discussions. The
>> > most important being is it worth adding the additional expense to all PM
>> > domains even if no wakeup pattern is available. It seems like
>> > maintaining a domain specific flag that the governor could check is a
>> > generic enough option. That should disable additional overhead for
>> > domains that do not need this feature.
>> >
>> > Ulf suggested that we could allow wakeups only if any of the domain idle
>> > state specifies a residency. However, we don't want to check for next
>> > wakeup everytime the domain enters idle just because the domain
>> > specifies an idle state with residency. This is also not desired.
>> >
>> > Also, if the domain checks for next wakeup, should the parent domains of
>> > the domain also check for next wakeup? And when do we set that up? These
>> > are questions that we don't know the answers yet. So, let's enable the
>> > domain governor only if the domain sets up the flag or when the device
>> > in the domain specifies the next wakeup.
>> >
>> > The previous post of the series explaining why this is a useful feature
>> > is v5[1]. Please let me know what you think.
>>
>> Ulf had comments on the previous versions, so waiting for him to
>> respond here, thanks!
>
>Yes, I will have a look, but please allow me some more time - it's a
>busy period for me.
>
Thank you.

-- Lina

Re: [PATCH v6 1/3] PM / Domains: add domain feature flag for next wakeup

[Ulf Hansson]

On Mon, 30 Nov 2020 at 23:51, Lina Iyer <ilina@codeaurora.org> wrote:
>
> PM domains may support entering multiple power down states when the
> component devices and sub-domains are suspended. Also, they may specify
> the residency value for an idle state, only after which the idle state
> may provide power benefits. If the domain does not specify the residency
> for any of its idle states, the governor's choice is much simplified.
>
> Let's make this optional with the use of a PM domain feature flag.
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v6:
>         - New patch based on discussions on v5 of the series
> ---
>  drivers/base/power/domain.c | 18 ++++++++++++++++++
>  include/linux/pm_domain.h   | 28 ++++++++++++++++++++++------
>  2 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 1b0c9ccbbe1f..1e6c0bf1c945 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1748,6 +1748,24 @@ int dev_pm_genpd_remove_notifier(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_genpd_remove_notifier);
>
> +/**
> + * genpd_enable_next_wakeup - Enable genpd gov to use next_wakeup
> + *
> + * @genpd: The genpd to be updated
> + * @enable: Enable/disable genpd gov to use next wakeup
> + */
> +void genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable)
> +{
> +       genpd_lock(genpd);
> +       if (enable)
> +               genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;
> +       else
> +               genpd->flags &= ~GENPD_FLAG_GOV_NEXT_WAKEUP;
> +       genpd->next_wakeup = KTIME_MAX;
> +       genpd_unlock(genpd);
> +}
> +EXPORT_SYMBOL_GPL(genpd_enable_next_wakeup);
> +

Please drop this, as I don't think we need a dedicated function to
enable this feature.

Instead, it seems like a better idea to let the genpd provider set it,
before it calls pm_genpd_init(), along with its other genpd
configurations.

>  static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>                                struct generic_pm_domain *subdomain)
>  {
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 2ca919ae8d36..1f359bd19f77 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -55,13 +55,19 @@
>   *
>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM domain
>   *                             powered on except for system suspend.
> + *
> + * GENPD_FLAG_GOV_NEXT_WAKEUP: Enable the genpd governor to consider its
> + *                             components' next wakeup when  determining the
> + *                             optimal idle state. This is setup only if the
> + *                             domain's idle state specifies a residency.
>   */
> -#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_GOV_NEXT_WAKEUP (1U << 6)

Nitpick.

To me, the configuration is something that corresponds to the genpd,
rather than the governor (even if it affects it in this case). That
said, how about just naming the flag something like
"GENPD_FLAG_MIN_RESIDENCY", as to indicate that the genpd's power off
states have minimum residencies values that may deserve to be
considered, while power off.

>
>  enum gpd_status {
>         GENPD_STATE_ON = 0,     /* PM domain is on */
> @@ -205,6 +211,11 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>         return to_gpd_data(dev->power.subsys_data->domain_data);
>  }
>
> +static inline bool genpd_may_use_next_wakeup(struct generic_pm_domain *genpd)
> +{
> +       return genpd->flags & GENPD_FLAG_GOV_NEXT_WAKEUP;
> +}

This can probably be moved into drivers/base/power/domain_governor.c.

> +
>  int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
>  int pm_genpd_remove_device(struct device *dev);
>  int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
> @@ -217,6 +228,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 genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable);
>
>  extern struct dev_power_governor simple_qos_governor;
>  extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -275,6 +287,10 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev)
>         return -EOPNOTSUPP;
>  }
>
> +static void genpd_enable_next_wakeup(struct generic_pm_domain *genpd,
> +                                    bool enable)
> +{ }
> +
>  #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

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

[Ulf Hansson]

On Mon, 30 Nov 2020 at 23:51, 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>
> ---
> 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 | 41 +++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  8 ++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 1e6c0bf1c945..191539a8e06d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -408,6 +408,46 @@ 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.

How about clarifying this as:

"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. Stale @next will be ignored when powering off the domain.
> + */
> +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;
> +               genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;

I don't think we should set this here, but instead leave this to be
set by the genpd provider at initialization.

> +               ret = 0;
> +       }
> +       genpd_unlock(genpd);

I would suggest to simplify the above into:

gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
gpd_data->next_wakeup = next;

Then there is no need for locks because:
*) We assume the device remains attached to the genpd.
**) The device isn't runtime suspended, hence its corresponding genpd
is powered on and thus the genpd governor can't be looking at
"gpd_data->next_wakeup" simulantfsfulsy.

Moreover, as we agreed to ignore stale values for "next", there is no
reason to validate it against the current ktime, so let's just skip
it.

> +
> +       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;
> @@ -1450,6 +1490,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;

When looking at patch3, I wonder if it perhaps could be easier to use
"zero" as the default value? What do you think, just an idea?

>
>         spin_lock_irq(&dev->power.lock);
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 1f359bd19f77..cc27d3d88849 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>
> @@ -197,6 +198,7 @@ struct generic_pm_domain_data {
>         struct notifier_block *power_nb;
>         int cpu;
>         unsigned int performance_state;
> +       ktime_t next_wakeup;
>         void *data;
>  };
>
> @@ -229,6 +231,7 @@ 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 genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable);
> +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;
> @@ -291,6 +294,11 @@ static void genpd_enable_next_wakeup(struct generic_pm_domain *genpd,
>                                      bool enable)
>  { }
>
> +static inline int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  #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

Re: [PATCH v6 3/3] PM / Domains: use device's next wakeup to determine domain idle state

[Ulf Hansson]

On Mon, 30 Nov 2020 at 23:50, Lina Iyer <ilina@codeaurora.org> wrote:
>
> Currently, a PM domain's idle state is determined based on whether the
> QoS requirements are met. This may not save power, if the idle state

/s/not save power/waste energy

/state/state's

> residency requirements are not met.

perhaps change to:
"minimum residency requirement can't be 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.

I suggest to clarify that this is being done, only when the genpd has
the corresponding config bit/flag set, that was added in patch1.

>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

Other than the above (and potential updates needed from comments on
patch1 and patch2), this looks good to me!

Kind regards
Uffe

> ---
> 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            |   1 +
>  2 files changed, 94 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> index 490ed7deb99a..2afb7fa90d5d 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_may_use_next_wakeup(genpd))
> +               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->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 cc27d3d88849..a41aea9d1c06 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -136,6 +136,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
>

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

[Lina Iyer]

On Tue, Dec 22 2020 at 06:10 -0700, Ulf Hansson wrote:
>On Mon, 30 Nov 2020 at 23:51, 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>
>> ---
>> 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 | 41 +++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  8 ++++++++
>>  2 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 1e6c0bf1c945..191539a8e06d 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -408,6 +408,46 @@ 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.
>
>How about clarifying this as:
>
>"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).
>
Sure.

>> + * 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.
>> + */
>> +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;
>> +               genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;
>
>I don't think we should set this here, but instead leave this to be
>set by the genpd provider at initialization.
>
But, we don't want it to be enabled by default. It is possible that
domains may have multiple idle states but none of the devices in the
domain have the need for next wakeup. We could optimize out the next
wakeup for those cases.
Or, the domain needs to call genpd_enable_next_wakeup() (patch #1) to
allow this feature. Is that acceptable?

>> +               ret = 0;
>> +       }
>> +       genpd_unlock(genpd);
>
>I would suggest to simplify the above into:
>
>gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
>gpd_data->next_wakeup = next;
>
>Then there is no need for locks because:
>*) We assume the device remains attached to the genpd.
>**) The device isn't runtime suspended, hence its corresponding genpd
>is powered on and thus the genpd governor can't be looking at
>"gpd_data->next_wakeup" simulantfsfulsy.
>
:)
>Moreover, as we agreed to ignore stale values for "next", there is no
>reason to validate it against the current ktime, so let's just skip
>it.
>
Okay.

>> +
>> +       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;
>> @@ -1450,6 +1490,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;
>
>When looking at patch3, I wonder if it perhaps could be easier to use
>"zero" as the default value? What do you think, just an idea?
>
Hmm.. Let me think it over.

Thanks,
Lina

>>
>>         spin_lock_irq(&dev->power.lock);
>>
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 1f359bd19f77..cc27d3d88849 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>
>> @@ -197,6 +198,7 @@ struct generic_pm_domain_data {
>>         struct notifier_block *power_nb;
>>         int cpu;
>>         unsigned int performance_state;
>> +       ktime_t next_wakeup;
>>         void *data;
>>  };
>>
>> @@ -229,6 +231,7 @@ 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 genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable);
>> +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;
>> @@ -291,6 +294,11 @@ static void genpd_enable_next_wakeup(struct generic_pm_domain *genpd,
>>                                      bool enable)
>>  { }
>>
>> +static inline int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
>> +{
>> +       return -EOPNOTSUPP;
>> +}
>> +
>>  #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

Re: [PATCH v6 1/3] PM / Domains: add domain feature flag for next wakeup

[Lina Iyer]

On Tue, Dec 22 2020 at 06:07 -0700, Ulf Hansson wrote:
>On Mon, 30 Nov 2020 at 23:51, Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> PM domains may support entering multiple power down states when the
>> component devices and sub-domains are suspended. Also, they may specify
>> the residency value for an idle state, only after which the idle state
>> may provide power benefits. If the domain does not specify the residency
>> for any of its idle states, the governor's choice is much simplified.
>>
>> Let's make this optional with the use of a PM domain feature flag.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> Changes in v6:
>>         - New patch based on discussions on v5 of the series
>> ---
>>  drivers/base/power/domain.c | 18 ++++++++++++++++++
>>  include/linux/pm_domain.h   | 28 ++++++++++++++++++++++------
>>  2 files changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 1b0c9ccbbe1f..1e6c0bf1c945 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1748,6 +1748,24 @@ int dev_pm_genpd_remove_notifier(struct device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_genpd_remove_notifier);
>>
>> +/**
>> + * genpd_enable_next_wakeup - Enable genpd gov to use next_wakeup
>> + *
>> + * @genpd: The genpd to be updated
>> + * @enable: Enable/disable genpd gov to use next wakeup
>> + */
>> +void genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable)
>> +{
>> +       genpd_lock(genpd);
>> +       if (enable)
>> +               genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;
>> +       else
>> +               genpd->flags &= ~GENPD_FLAG_GOV_NEXT_WAKEUP;
>> +       genpd->next_wakeup = KTIME_MAX;
>> +       genpd_unlock(genpd);
>> +}
>> +EXPORT_SYMBOL_GPL(genpd_enable_next_wakeup);
>> +
>
>Please drop this, as I don't think we need a dedicated function to
>enable this feature.
>
Ah, I responded to the patch #2 regarding this before reading this
email.

>Instead, it seems like a better idea to let the genpd provider set it,
>before it calls pm_genpd_init(), along with its other genpd
>configurations.
>
If we no longer have a need for this feature in the domain, we could
dynamically disable it.

>>  static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>>                                struct generic_pm_domain *subdomain)
>>  {
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 2ca919ae8d36..1f359bd19f77 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -55,13 +55,19 @@
>>   *
>>   * GENPD_FLAG_RPM_ALWAYS_ON:   Instructs genpd to always keep the PM domain
>>   *                             powered on except for system suspend.
>> + *
>> + * GENPD_FLAG_GOV_NEXT_WAKEUP: Enable the genpd governor to consider its
>> + *                             components' next wakeup when  determining the
>> + *                             optimal idle state. This is setup only if the
>> + *                             domain's idle state specifies a residency.
>>   */
>> -#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_GOV_NEXT_WAKEUP (1U << 6)
>
>Nitpick.
>
>To me, the configuration is something that corresponds to the genpd,
>rather than the governor (even if it affects it in this case). That
>said, how about just naming the flag something like
>"GENPD_FLAG_MIN_RESIDENCY", as to indicate that the genpd's power off
>states have minimum residencies values that may deserve to be
>considered, while power off.
>
Hmmm..
>>
>>  enum gpd_status {
>>         GENPD_STATE_ON = 0,     /* PM domain is on */
>> @@ -205,6 +211,11 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
>>         return to_gpd_data(dev->power.subsys_data->domain_data);
>>  }
>>
>> +static inline bool genpd_may_use_next_wakeup(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->flags & GENPD_FLAG_GOV_NEXT_WAKEUP;
>> +}
>
>This can probably be moved into drivers/base/power/domain_governor.c.
>
Okay.
>> +
>>  int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
>>  int pm_genpd_remove_device(struct device *dev);
>>  int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>> @@ -217,6 +228,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 genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable);
>>
>>  extern struct dev_power_governor simple_qos_governor;
>>  extern struct dev_power_governor pm_domain_always_on_gov;
>> @@ -275,6 +287,10 @@ static inline int dev_pm_genpd_remove_notifier(struct device *dev)
>>         return -EOPNOTSUPP;
>>  }
>>
>> +static void genpd_enable_next_wakeup(struct generic_pm_domain *genpd,
>> +                                    bool enable)
>> +{ }
>> +
>>  #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

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

[Ulf Hansson]

On Mon, 4 Jan 2021 at 16:44, Lina Iyer <ilina@codeaurora.org> wrote:
>
> On Tue, Dec 22 2020 at 06:10 -0700, Ulf Hansson wrote:
> >On Mon, 30 Nov 2020 at 23:51, 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>
> >> ---
> >> 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 | 41 +++++++++++++++++++++++++++++++++++++
> >>  include/linux/pm_domain.h   |  8 ++++++++
> >>  2 files changed, 49 insertions(+)
> >>
> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >> index 1e6c0bf1c945..191539a8e06d 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -408,6 +408,46 @@ 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.
> >
> >How about clarifying this as:
> >
> >"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).
> >
> Sure.
>
> >> + * 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.
> >> + */
> >> +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;
> >> +               genpd->flags |= GENPD_FLAG_GOV_NEXT_WAKEUP;
> >
> >I don't think we should set this here, but instead leave this to be
> >set by the genpd provider at initialization.
> >
> But, we don't want it to be enabled by default. It is possible that
> domains may have multiple idle states but none of the devices in the
> domain have the need for next wakeup. We could optimize out the next
> wakeup for those cases.

Yes, I understand, but at this point I don't think it's needed.

My main concern was to allow us to avoid the path for the
"pm_domain_cpu_gov" type for some genpds. Thus I think it's good
enough to set a new flag per genpd at initialization time.

> Or, the domain needs to call genpd_enable_next_wakeup() (patch #1) to
> allow this feature. Is that acceptable?

The problem is that we would need a reference counting mechanism, as
to also understand when to also turn off the flag. In other words, we
would need some kind of locking. I suggest we simply skip this for now
and see how this plays. What do you think?

>
> >> +               ret = 0;
> >> +       }
> >> +       genpd_unlock(genpd);
> >
> >I would suggest to simplify the above into:
> >
> >gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> >gpd_data->next_wakeup = next;
> >
> >Then there is no need for locks because:
> >*) We assume the device remains attached to the genpd.
> >**) The device isn't runtime suspended, hence its corresponding genpd
> >is powered on and thus the genpd governor can't be looking at
> >"gpd_data->next_wakeup" simulantfsfulsy.
> >
> :)
> >Moreover, as we agreed to ignore stale values for "next", there is no
> >reason to validate it against the current ktime, so let's just skip
> >it.
> >
> Okay.
>
> >> +
> >> +       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;
> >> @@ -1450,6 +1490,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;
> >
> >When looking at patch3, I wonder if it perhaps could be easier to use
> >"zero" as the default value? What do you think, just an idea?
> >
> Hmm.. Let me think it over.
>
> Thanks,
> Lina
>
> >>
> >>         spin_lock_irq(&dev->power.lock);
> >>

[...]

Kind regards
Uffe