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

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

[Lina Iyer]

Changes since RFC [1]:
- Organized the code to make it cleaner
- Fixed some issues with idle state determination
- Add documentation and update commit text

Hello,

I was looking for an option to do better power management for some
domains where the devices enter runtime PM in a predictable fashion. For
example a display device that sends a vsync interrupt every 16 ms for a
60 Hz panel. These interrupts are not timer interrupts but tend to
interrupt periodically to service the workflow and the devices and
domains may go back to idle soon after. Two domains are affected by this
- the device's PM domain and the CPU PM domain.

As a first step, I am looking to solve for the device's PM domain idle
state (and hopefully solve for the CPU PM domains subsequently). The PM
domain could have multiple idle states and/or the enter/exit latencies
could be high. In either case, it may not always be beneficial to power
off the domain, only to turn it back on before satisfying the idle state
residency. When the wakeup is known for the device, we could use that to
determine the worthiness of entering a domain idle state. Only the
device can tell us when the future event would be and that could change
as the usecase changes. Like, when the panel refresh rate increases to
120 Hz. If this information was made available to runtime PM, we could
use that in the domain governor to determine a suitable idle state. This
is the idea behind these patches.

Would appreciate your thoughts on this.

Thanks,
Lina

[1]. https://lore.kernel.org/linux-pm/010101746eccb270-05beb27f-e1e4-40eb-92da-ad1bb48feb41-000000@us-west-2.amazonses.com/T/

Lina Iyer (2):
  PM / runtime: register device's next wakeup
  PM / Domains: use device's next wakeup to determine domain idle state

 Documentation/power/runtime_pm.rst   | 21 +++++++
 drivers/base/power/domain_governor.c | 83 ++++++++++++++++++++++++++--
 drivers/base/power/runtime.c         | 31 +++++++++++
 include/linux/pm.h                   |  2 +
 include/linux/pm_domain.h            |  1 +
 include/linux/pm_runtime.h           |  1 +
 6 files changed, 134 insertions(+), 5 deletions(-)

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

[PATCH 1/2] PM / runtime: register device's next wakeup

[Lina Iyer]

Some devices have a predictable interrupt pattern while executing a
particular usecase. An example would be the VSYNC interrupt on devices
associated with displays. A 60 Hz display could cause a periodic
interrupt every 16 ms. A PM domain that holds such a device could power
off and on at similar intervals.

Entering a domain idle state saves power, only if the domain remains in
the idle state for the amount of time greater than or equal to the
residency of the idle state. Knowing the next wakeup event of the device
will help the PM domain governor make better idle state decisions.

Let's add the pm_runtime_set_next_wake() API for the device and document
the usage of the API.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 Documentation/power/runtime_pm.rst | 21 ++++++++++++++++++++
 drivers/base/power/runtime.c       | 31 ++++++++++++++++++++++++++++++
 include/linux/pm.h                 |  2 ++
 include/linux/pm_runtime.h         |  1 +
 4 files changed, 55 insertions(+)

diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
index 0553008b6279..90a5ac481ad4 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -515,6 +515,14 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
       power.use_autosuspend isn't set, otherwise returns the expiration time
       in jiffies
 
+  `int pm_runtime_set_next_event(struct device *dev, ktime_t next);`
+    - notify runtime PM of the next event on the device. Devices that are
+      sensitive to their domain idle enter/exit latencies may provide this
+      information for use by the PM domain governor. The governor may determine
+      if it worthwhile to enter a domain idle state. If the residency of the
+      domain idle state is not met, the domain would waste more power entering
+      and exiting the said idle state.
+
 It is safe to execute the following helper functions from interrupt context:
 
 - pm_request_idle()
@@ -545,6 +553,7 @@ functions may also be used in interrupt context:
 - pm_runtime_put_sync()
 - pm_runtime_put_sync_suspend()
 - pm_runtime_put_sync_autosuspend()
+- pm_runtime_set_next_event()
 
 5. Runtime PM Initialization, Device Probing and Removal
 ========================================================
@@ -639,6 +648,18 @@ suspend routine).  It may be necessary to resume the device and suspend it again
 in order to do so.  The same is true if the driver uses different power levels
 or other settings for runtime suspend and system sleep.
 
+When a device enters idle at runtime, it may trigger the runtime PM up the
+hierarchy. Devices that have an predictable interrupt pattern, may help
+influence a better idle state determination of its parent. For example, a
+display device could get a VSYNC interrupt every 16ms. A PM domain containing
+the device, could also be entering and exiting idle due to runtime PM
+coordination. If the domain were also entering runtime idle, we would know when
+the domain would be waken up as a result of the display device waking up. Using
+the device's next_event, the PM domain governor can make a better choice of the
+idle state for the domain, knowing it would be be woken up by the device in the
+near future. This is specially useful when the device is sensitive to its PM
+domain's idle state enter and exit latencies.
+
 During system resume, the simplest approach is to bring all devices back to full
 power, even if they had been suspended before the system suspend began.  There
 are several reasons for this, including:
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 8143210a5c54..53c2b3d962bc 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -122,6 +122,33 @@ u64 pm_runtime_suspended_time(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
 
+/**
+ * pm_runtime_set_next_wakeup_event - Notify PM framework of an impending event.
+ * @dev: Device to handle
+ * @next: impending interrupt/wakeup for the device
+ */
+int pm_runtime_set_next_event(struct device *dev, ktime_t next)
+{
+	unsigned long flags;
+	int ret = -EINVAL;
+
+	/*
+	 * Note the next pending wakeup of a device,
+	 * if the device does not have runtime PM enabled.
+	 */
+	spin_lock_irqsave(&dev->power.lock, flags);
+	if (!dev->power.disable_depth) {
+		if (ktime_before(ktime_get(), next)) {
+			dev->power.next_event = next;
+			ret = 0;
+		}
+	}
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_set_next_event);
+
 /**
  * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
  * @dev: Device to handle.
@@ -1380,6 +1407,9 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
 	/* Update time accounting before disabling PM-runtime. */
 	update_pm_runtime_accounting(dev);
 
+	/* Reset the next wakeup for the device */
+	dev->power.next_event = KTIME_MAX;
+
 	if (!dev->power.disable_depth++)
 		__pm_runtime_barrier(dev);
 
@@ -1609,6 +1639,7 @@ void pm_runtime_init(struct device *dev)
 	dev->power.deferred_resume = false;
 	INIT_WORK(&dev->power.work, pm_runtime_work);
 
+	dev->power.next_event = KTIME_MAX;
 	dev->power.timer_expires = 0;
 	hrtimer_init(&dev->power.suspend_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
 	dev->power.suspend_timer.function = pm_suspend_timer_fn;
diff --git a/include/linux/pm.h b/include/linux/pm.h
index a30a4b54df52..9051658674a4 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -8,6 +8,7 @@
 #ifndef _LINUX_PM_H
 #define _LINUX_PM_H
 
+#include <linux/ktime.h>
 #include <linux/list.h>
 #include <linux/workqueue.h>
 #include <linux/spinlock.h>
@@ -616,6 +617,7 @@ struct dev_pm_info {
 	u64			active_time;
 	u64			suspended_time;
 	u64			accounting_timestamp;
+	ktime_t			next_event;
 #endif
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	void (*set_latency_tolerance)(struct device *, s32);
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 6245caa18034..af6d35178335 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -59,6 +59,7 @@ extern void pm_runtime_get_suppliers(struct device *dev);
 extern void pm_runtime_put_suppliers(struct device *dev);
 extern void pm_runtime_new_link(struct device *dev);
 extern void pm_runtime_drop_link(struct device *dev);
+extern int pm_runtime_set_next_event(struct device *dev, ktime_t next);
 
 /**
  * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Lina Iyer]

If the device's next event is known, determine if it is worthwhile
entering a domain idle state. To find the next wakeup, traverse a domain
for all child devices and find out the earliest wakeup value. A parent
domain's next wakeup is the earliest of all its child devices and
domains. The next wakeup is specified by devices in the domains before
they devices are suspended.

Update the domain governor logic to determine if it is worthwhile to
enter an idle state based on the next wakeup for the domain along with
other existing constraints.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/base/power/domain_governor.c | 83 ++++++++++++++++++++++++++--
 include/linux/pm_domain.h            |  1 +
 2 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 490ed7deb99a..77b4928aa389 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -117,6 +117,49 @@ 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;
+
+	/* Find the earliest wakeup for all devices in the domain */
+	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+		next_wakeup = READ_ONCE(pdd->dev->power.next_event);
+		if (next_wakeup != KTIME_MAX && !ktime_before(next_wakeup, now))
+			if (ktime_before(next_wakeup, domain_wakeup))
+				domain_wakeup = next_wakeup;
+	}
+
+	/* Then find the earliest wakeup of from all the child domains */
+	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)
+{
+	s64 idle_time_ns, min_sleep_ns;
+	ktime_t domain_wakeup = genpd->next_wakeup;
+
+	min_sleep_ns = genpd->states[state].power_off_latency_ns +
+		       genpd->states[state].power_on_latency_ns +
+		       genpd->states[state].residency_ns;
+
+	idle_time_ns = ktime_to_ns(ktime_sub(domain_wakeup, now));
+	if (idle_time_ns < min_sleep_ns)
+		return false;
+
+	return true;
+}
+
 static bool __default_power_down_ok(struct dev_pm_domain *pd,
 				     unsigned int state)
 {
@@ -210,6 +253,33 @@ static bool default_power_down_ok(struct dev_pm_domain *pd)
 {
 	struct generic_pm_domain *genpd = pd_to_genpd(pd);
 	struct gpd_link *link;
+	unsigned int state_idx;
+	ktime_t now = ktime_get();
+
+	/*
+	 * 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);
+	state_idx = genpd->state_count - 1;
+	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;
@@ -228,17 +298,20 @@ 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;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index ee11502a575b..9ea6f666967b 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -119,6 +119,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;
 	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 2/2] PM / Domains: use device's next wakeup to determine domain idle state

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]

Hi Lina,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on linus/master v5.9 next-20201013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lina-Iyer/Better-domain-idle-from-device-wakeup-patterns/20201013-063543
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-randconfig-m021-20201013 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
drivers/base/power/domain_governor.c:269 default_power_down_ok() warn: always true condition '(state_idx >= 0) => (0-u32max >= 0)'
drivers/base/power/domain_governor.c:269 default_power_down_ok() warn: always true condition '(state_idx >= 0) => (0-u32max >= 0)'
drivers/base/power/domain_governor.c:277 default_power_down_ok() warn: unsigned 'state_idx' is never less than zero.

vim +269 drivers/base/power/domain_governor.c

   245	
   246	/**
   247	 * default_power_down_ok - Default generic PM domain power off governor routine.
   248	 * @pd: PM domain to check.
   249	 *
   250	 * This routine must be executed under the PM domain's lock.
   251	 */
   252	static bool default_power_down_ok(struct dev_pm_domain *pd)
   253	{
   254		struct generic_pm_domain *genpd = pd_to_genpd(pd);
   255		struct gpd_link *link;
   256		unsigned int state_idx;
   257		ktime_t now = ktime_get();
   258	
   259		/*
   260		 * Find the next wakeup from devices that can determine their own wakeup
   261		 * to find when the domain would wakeup and do it for every device down
   262		 * the hierarchy. It is not worth while to sleep if the state's residency
   263		 * cannot be met.
   264		 */
   265		update_domain_next_wakeup(genpd, now);
   266		state_idx = genpd->state_count - 1;
   267		if (genpd->next_wakeup != KTIME_MAX) {
   268			/* Let's find out the deepest domain idle state, the devices prefer */
 > 269			while (state_idx >= 0) {
   270				if (next_wakeup_allows_state(genpd, state_idx, now)) {
   271					genpd->max_off_time_changed = true;
   272					break;
   273				}
   274				state_idx--;
   275			}
   276	
 > 277			if (state_idx < 0) {
   278				state_idx = 0;
   279				genpd->cached_power_down_ok = false;
   280				goto done;
   281			}
   282		}
   283	
   284		if (!genpd->max_off_time_changed) {
   285			genpd->state_idx = genpd->cached_power_down_state_idx;
   286			return genpd->cached_power_down_ok;
   287		}
   288	
   289		/*
   290		 * We have to invalidate the cached results for the parents, so
   291		 * use the observation that default_power_down_ok() is not
   292		 * going to be called for any parent until this instance
   293		 * returns.
   294		 */
   295		list_for_each_entry(link, &genpd->child_links, child_node)
   296			link->parent->max_off_time_changed = true;
   297	
   298		genpd->max_off_time_ns = -1;
   299		genpd->max_off_time_changed = false;
   300		genpd->cached_power_down_ok = true;
   301	
   302		/* Find a state to power down to, starting from the state
   303		 * determined by the next wakeup.
   304		 */
   305		while (!__default_power_down_ok(pd, state_idx)) {
   306			if (state_idx == 0) {
   307				genpd->cached_power_down_ok = false;
   308				break;
   309			}
   310			state_idx--;
   311		}
   312	
   313	done:
   314		genpd->state_idx = state_idx;
   315		genpd->cached_power_down_state_idx = genpd->state_idx;
   316		return genpd->cached_power_down_ok;
   317	}
   318	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40850 bytes --]

Re: [PATCH 1/2] PM / runtime: register device's next wakeup

[Ulf Hansson]

On Tue, 13 Oct 2020 at 00:34, Lina Iyer <ilina@codeaurora.org> wrote:
>
> Some devices have a predictable interrupt pattern while executing a
> particular usecase. An example would be the VSYNC interrupt on devices
> associated with displays. A 60 Hz display could cause a periodic
> interrupt every 16 ms. A PM domain that holds such a device could power
> off and on at similar intervals.
>
> Entering a domain idle state saves power, only if the domain remains in
> the idle state for the amount of time greater than or equal to the
> residency of the idle state. Knowing the next wakeup event of the device
> will help the PM domain governor make better idle state decisions.
>
> Let's add the pm_runtime_set_next_wake() API for the device and document
> the usage of the API.
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  Documentation/power/runtime_pm.rst | 21 ++++++++++++++++++++
>  drivers/base/power/runtime.c       | 31 ++++++++++++++++++++++++++++++
>  include/linux/pm.h                 |  2 ++
>  include/linux/pm_runtime.h         |  1 +
>  4 files changed, 55 insertions(+)
>
> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> index 0553008b6279..90a5ac481ad4 100644
> --- a/Documentation/power/runtime_pm.rst
> +++ b/Documentation/power/runtime_pm.rst
> @@ -515,6 +515,14 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
>        power.use_autosuspend isn't set, otherwise returns the expiration time
>        in jiffies
>
> +  `int pm_runtime_set_next_event(struct device *dev, ktime_t next);`

Rather than specifying the next event, could it make sense to specify
the delta instead? I guess it depends on the behaviour of the
driver/client that calls this API...

> +    - notify runtime PM of the next event on the device. Devices that are

I would prefer to change from "notify" to "inform", just to make it
clear that this isn't a notification mechanism we are talking about.

> +      sensitive to their domain idle enter/exit latencies may provide this
> +      information for use by the PM domain governor. The governor may determine
> +      if it worthwhile to enter a domain idle state. If the residency of the
> +      domain idle state is not met, the domain would waste more power entering
> +      and exiting the said idle state.
> +
>  It is safe to execute the following helper functions from interrupt context:
>
>  - pm_request_idle()
> @@ -545,6 +553,7 @@ functions may also be used in interrupt context:
>  - pm_runtime_put_sync()
>  - pm_runtime_put_sync_suspend()
>  - pm_runtime_put_sync_autosuspend()
> +- pm_runtime_set_next_event()
>
>  5. Runtime PM Initialization, Device Probing and Removal
>  ========================================================
> @@ -639,6 +648,18 @@ suspend routine).  It may be necessary to resume the device and suspend it again
>  in order to do so.  The same is true if the driver uses different power levels
>  or other settings for runtime suspend and system sleep.
>
> +When a device enters idle at runtime, it may trigger the runtime PM up the
> +hierarchy. Devices that have an predictable interrupt pattern, may help
> +influence a better idle state determination of its parent. For example, a
> +display device could get a VSYNC interrupt every 16ms. A PM domain containing
> +the device, could also be entering and exiting idle due to runtime PM

/containing the device/that has the device attached to it

> +coordination. If the domain were also entering runtime idle, we would know when
> +the domain would be waken up as a result of the display device waking up. Using
> +the device's next_event, the PM domain governor can make a better choice of the
> +idle state for the domain, knowing it would be be woken up by the device in the
> +near future. This is specially useful when the device is sensitive to its PM
> +domain's idle state enter and exit latencies.

The above sounds a little hand wavy, can you try to be a little more exact?

Perhaps, rather than just saying "sensitive to it's PM domain's idle
state..", how about explaining that by using the "next event" the
governor is able to select a more optimal domain idle state, thus we
should avoid wasting energy and better conform to QoS latency
constraints.

> +
>  During system resume, the simplest approach is to bring all devices back to full
>  power, even if they had been suspended before the system suspend began.  There
>  are several reasons for this, including:
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 8143210a5c54..53c2b3d962bc 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -122,6 +122,33 @@ u64 pm_runtime_suspended_time(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
>
> +/**
> + * pm_runtime_set_next_wakeup_event - Notify PM framework of an impending event.
> + * @dev: Device to handle
> + * @next: impending interrupt/wakeup for the device

At what typical points do you expect this function to be called?

> + */
> +int pm_runtime_set_next_event(struct device *dev, ktime_t next)
> +{
> +       unsigned long flags;
> +       int ret = -EINVAL;
> +
> +       /*
> +        * Note the next pending wakeup of a device,
> +        * if the device does not have runtime PM enabled.
> +        */

/s/Note/Store

Do you really need to check if runtime PM is enabled? Does it matter?

> +       spin_lock_irqsave(&dev->power.lock, flags);
> +       if (!dev->power.disable_depth) {
> +               if (ktime_before(ktime_get(), next)) {
> +                       dev->power.next_event = next;
> +                       ret = 0;
> +               }
> +       }
> +       spin_unlock_irqrestore(&dev->power.lock, flags);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_set_next_event);
> +
>  /**
>   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
>   * @dev: Device to handle.
> @@ -1380,6 +1407,9 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
>         /* Update time accounting before disabling PM-runtime. */
>         update_pm_runtime_accounting(dev);
>
> +       /* Reset the next wakeup for the device */
> +       dev->power.next_event = KTIME_MAX;
> +

I am not sure I get the purpose of this, can you elaborate?

I am thinking that the genpd governor doesn't allow to power off of
the PM domain, unless all devices that are attached to it are runtime
PM enabled and runtime PM suspended (see pm_runtime_suspended). That
said, it looks like the above isn't needed? No?

Perhaps it's better to use pm_runtime_enable() as the point of
resetting the dev->power.next_event?

>         if (!dev->power.disable_depth++)
>                 __pm_runtime_barrier(dev);
>
> @@ -1609,6 +1639,7 @@ void pm_runtime_init(struct device *dev)
>         dev->power.deferred_resume = false;
>         INIT_WORK(&dev->power.work, pm_runtime_work);
>
> +       dev->power.next_event = KTIME_MAX;
>         dev->power.timer_expires = 0;
>         hrtimer_init(&dev->power.suspend_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>         dev->power.suspend_timer.function = pm_suspend_timer_fn;
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index a30a4b54df52..9051658674a4 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -8,6 +8,7 @@
>  #ifndef _LINUX_PM_H
>  #define _LINUX_PM_H
>
> +#include <linux/ktime.h>
>  #include <linux/list.h>
>  #include <linux/workqueue.h>
>  #include <linux/spinlock.h>
> @@ -616,6 +617,7 @@ struct dev_pm_info {
>         u64                     active_time;
>         u64                     suspended_time;
>         u64                     accounting_timestamp;
> +       ktime_t                 next_event;
>  #endif
>         struct pm_subsys_data   *subsys_data;  /* Owned by the subsystem. */
>         void (*set_latency_tolerance)(struct device *, s32);
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 6245caa18034..af6d35178335 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -59,6 +59,7 @@ extern void pm_runtime_get_suppliers(struct device *dev);
>  extern void pm_runtime_put_suppliers(struct device *dev);
>  extern void pm_runtime_new_link(struct device *dev);
>  extern void pm_runtime_drop_link(struct device *dev);
> +extern int pm_runtime_set_next_event(struct device *dev, ktime_t next);
>
>  /**
>   * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

Kind regards
Uffe

Re: [PATCH 1/2] PM / runtime: register device's next wakeup

[Lina Iyer]

Hi Ulf,

Thanks for taking time to review this.

On Wed, Oct 14 2020 at 04:29 -0600, Ulf Hansson wrote:
>On Tue, 13 Oct 2020 at 00:34, Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> Some devices have a predictable interrupt pattern while executing a
>> particular usecase. An example would be the VSYNC interrupt on devices
>> associated with displays. A 60 Hz display could cause a periodic
>> interrupt every 16 ms. A PM domain that holds such a device could power
>> off and on at similar intervals.
>>
>> Entering a domain idle state saves power, only if the domain remains in
>> the idle state for the amount of time greater than or equal to the
>> residency of the idle state. Knowing the next wakeup event of the device
>> will help the PM domain governor make better idle state decisions.
>>
>> Let's add the pm_runtime_set_next_wake() API for the device and document
>> the usage of the API.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  Documentation/power/runtime_pm.rst | 21 ++++++++++++++++++++
>>  drivers/base/power/runtime.c       | 31 ++++++++++++++++++++++++++++++
>>  include/linux/pm.h                 |  2 ++
>>  include/linux/pm_runtime.h         |  1 +
>>  4 files changed, 55 insertions(+)
>>
>> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
>> index 0553008b6279..90a5ac481ad4 100644
>> --- a/Documentation/power/runtime_pm.rst
>> +++ b/Documentation/power/runtime_pm.rst
>> @@ -515,6 +515,14 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
>>        power.use_autosuspend isn't set, otherwise returns the expiration time
>>        in jiffies
>>
>> +  `int pm_runtime_set_next_event(struct device *dev, ktime_t next);`
>
>Rather than specifying the next event, could it make sense to specify
>the delta instead? I guess it depends on the behaviour of the
>driver/client that calls this API...
>
I guess, drivers would calculate the next event from an interval, but
the usage of this feature needs to account for the time from when this
call was received. I am open to taking in interval as the input and
saving the next actual time (adding the current ktime) to it.

>> +    - notify runtime PM of the next event on the device. Devices that are
>
>I would prefer to change from "notify" to "inform", just to make it
>clear that this isn't a notification mechanism we are talking about.
>
Okay.

>>  5. Runtime PM Initialization, Device Probing and Removal
>>  ========================================================
>> @@ -639,6 +648,18 @@ suspend routine).  It may be necessary to resume the device and suspend it again
>>  in order to do so.  The same is true if the driver uses different power levels
>>  or other settings for runtime suspend and system sleep.
>>
>> +When a device enters idle at runtime, it may trigger the runtime PM up the
>> +hierarchy. Devices that have an predictable interrupt pattern, may help
>> +influence a better idle state determination of its parent. For example, a
>> +display device could get a VSYNC interrupt every 16ms. A PM domain containing
>> +the device, could also be entering and exiting idle due to runtime PM
>
>/containing the device/that has the device attached to it
>
>> +coordination. If the domain were also entering runtime idle, we would know when
>> +the domain would be waken up as a result of the display device waking up. Using
>> +the device's next_event, the PM domain governor can make a better choice of the
>> +idle state for the domain, knowing it would be be woken up by the device in the
>> +near future. This is specially useful when the device is sensitive to its PM
>> +domain's idle state enter and exit latencies.
>
>The above sounds a little hand wavy, can you try to be a little more exact?
>
I can try and rephrase this. But what I think I should be saying is that
if the domain has multiple devices and if some devices are sensitive to
the exit latency of the domain idle, then knowing the next wakeup would
help the governor make better domain idle state decision.

>Perhaps, rather than just saying "sensitive to it's PM domain's idle
>state..", how about explaining that by using the "next event" the
>governor is able to select a more optimal domain idle state, thus we
>should avoid wasting energy and better conform to QoS latency
>constraints.
>
QoS is not what we are trying to conform to. We are trying to provide
residency information to the domain to help it make better choice. Just
like we use the CPU's next wakeup in the cluster domain governor.
>> +
>>  During system resume, the simplest approach is to bring all devices back to full
>>  power, even if they had been suspended before the system suspend began.  There
>>  are several reasons for this, including:
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index 8143210a5c54..53c2b3d962bc 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -122,6 +122,33 @@ u64 pm_runtime_suspended_time(struct device *dev)
>>  }
>>  EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
>>
>> +/**
>> + * pm_runtime_set_next_wakeup_event - Notify PM framework of an impending event.
>> + * @dev: Device to handle
>> + * @next: impending interrupt/wakeup for the device
>
>At what typical points do you expect this function to be called?
>
Most likely from at the start of the usecase and periodically when the
interrupt/work is being handled. I would think this change to a
different periodicity when the usecase parameters changes.

>> + */
>> +int pm_runtime_set_next_event(struct device *dev, ktime_t next)
>> +{
>> +       unsigned long flags;
>> +       int ret = -EINVAL;
>> +
>> +       /*
>> +        * Note the next pending wakeup of a device,
>> +        * if the device does not have runtime PM enabled.
>> +        */
>
>/s/Note/Store
>
>Do you really need to check if runtime PM is enabled? Does it matter?
>
Hmm.. This has no meaning without runtime PM. Any reason why we don't
need the check? I am okay to removing the check.

>> +       spin_lock_irqsave(&dev->power.lock, flags);
>> +       if (!dev->power.disable_depth) {
>> +               if (ktime_before(ktime_get(), next)) {
>> +                       dev->power.next_event = next;
>> +                       ret = 0;
>> +               }
>> +       }
>> +       spin_unlock_irqrestore(&dev->power.lock, flags);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(pm_runtime_set_next_event);
>> +
>>  /**
>>   * pm_runtime_deactivate_timer - Deactivate given device's suspend timer.
>>   * @dev: Device to handle.
>> @@ -1380,6 +1407,9 @@ void __pm_runtime_disable(struct device *dev, bool check_resume)
>>         /* Update time accounting before disabling PM-runtime. */
>>         update_pm_runtime_accounting(dev);
>>
>> +       /* Reset the next wakeup for the device */
>> +       dev->power.next_event = KTIME_MAX;
>> +
>
>I am not sure I get the purpose of this, can you elaborate?
>
I was trying to make sure that we clean up any next_events when we
disable runtime PM. But your following point negates the need.

>I am thinking that the genpd governor doesn't allow to power off of
>the PM domain, unless all devices that are attached to it are runtime
>PM enabled and runtime PM suspended (see pm_runtime_suspended). That
>said, it looks like the above isn't needed? No?
>
Makes sense.

>Perhaps it's better to use pm_runtime_enable() as the point of
>resetting the dev->power.next_event?
>
Okay.

Thanks,
Lina

Re: [PATCH 1/2] PM / runtime: register device's next wakeup

[Ulf Hansson]

Hi Lina,

[...]

> >>  5. Runtime PM Initialization, Device Probing and Removal
> >>  ========================================================
> >> @@ -639,6 +648,18 @@ suspend routine).  It may be necessary to resume the device and suspend it again
> >>  in order to do so.  The same is true if the driver uses different power levels
> >>  or other settings for runtime suspend and system sleep.
> >>
> >> +When a device enters idle at runtime, it may trigger the runtime PM up the
> >> +hierarchy. Devices that have an predictable interrupt pattern, may help
> >> +influence a better idle state determination of its parent. For example, a
> >> +display device could get a VSYNC interrupt every 16ms. A PM domain containing
> >> +the device, could also be entering and exiting idle due to runtime PM
> >
> >/containing the device/that has the device attached to it
> >
> >> +coordination. If the domain were also entering runtime idle, we would know when
> >> +the domain would be waken up as a result of the display device waking up. Using
> >> +the device's next_event, the PM domain governor can make a better choice of the
> >> +idle state for the domain, knowing it would be be woken up by the device in the
> >> +near future. This is specially useful when the device is sensitive to its PM
> >> +domain's idle state enter and exit latencies.
> >
> >The above sounds a little hand wavy, can you try to be a little more exact?
> >
> I can try and rephrase this. But what I think I should be saying is that
> if the domain has multiple devices and if some devices are sensitive to
> the exit latency of the domain idle, then knowing the next wakeup would
> help the governor make better domain idle state decision.
>
> >Perhaps, rather than just saying "sensitive to it's PM domain's idle
> >state..", how about explaining that by using the "next event" the
> >governor is able to select a more optimal domain idle state, thus we
> >should avoid wasting energy and better conform to QoS latency
> >constraints.
> >
> QoS is not what we are trying to conform to. We are trying to provide
> residency information to the domain to help it make better choice. Just
> like we use the CPU's next wakeup in the cluster domain governor.

Yep, that makes perfect sense to me as well. Then, please try to
clarify this in the above text.

> >> +
> >>  During system resume, the simplest approach is to bring all devices back to full
> >>  power, even if they had been suspended before the system suspend began.  There
> >>  are several reasons for this, including:
> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> >> index 8143210a5c54..53c2b3d962bc 100644
> >> --- a/drivers/base/power/runtime.c
> >> +++ b/drivers/base/power/runtime.c
> >> @@ -122,6 +122,33 @@ u64 pm_runtime_suspended_time(struct device *dev)
> >>  }
> >>  EXPORT_SYMBOL_GPL(pm_runtime_suspended_time);
> >>
> >> +/**
> >> + * pm_runtime_set_next_wakeup_event - Notify PM framework of an impending event.
> >> + * @dev: Device to handle
> >> + * @next: impending interrupt/wakeup for the device
> >
> >At what typical points do you expect this function to be called?
> >
> Most likely from at the start of the usecase and periodically when the
> interrupt/work is being handled. I would think this change to a
> different periodicity when the usecase parameters changes.

Alright, thanks for explaining.

>
> >> + */
> >> +int pm_runtime_set_next_event(struct device *dev, ktime_t next)
> >> +{
> >> +       unsigned long flags;
> >> +       int ret = -EINVAL;
> >> +
> >> +       /*
> >> +        * Note the next pending wakeup of a device,
> >> +        * if the device does not have runtime PM enabled.
> >> +        */
> >
> >/s/Note/Store
> >
> >Do you really need to check if runtime PM is enabled? Does it matter?
> >
> Hmm.. This has no meaning without runtime PM. Any reason why we don't
> need the check? I am okay to removing the check.

In principle, I want to avoid unnecessary code, thus I am in favor of
dropping the check. Not a big deal though.

>
> >> +       spin_lock_irqsave(&dev->power.lock, flags);
> >> +       if (!dev->power.disable_depth) {
> >> +               if (ktime_before(ktime_get(), next)) {
> >> +                       dev->power.next_event = next;
> >> +                       ret = 0;
> >> +               }
> >> +       }
> >> +       spin_unlock_irqrestore(&dev->power.lock, flags);
> >> +
> >> +       return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(pm_runtime_set_next_event);

[...]

Kind regards
Uffe