Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/2] Better domain idle from device wakeup patterns
@ 2020-10-15 19:38 Lina Iyer
  2020-10-15 19:38 ` [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup Lina Iyer
  2020-10-15 19:38 ` [PATCH v3 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer
  0 siblings, 2 replies; 7+ messages in thread
From: Lina Iyer @ 2020-10-15 19:38 UTC (permalink / raw)
  To: rjw, ulf.hansson, linux-pm; +Cc: linux-arm-msm, Lina Iyer

Changes since v2:
- Fix unwanted change

Changes since v1 [2]:
- Update documentation and commit text
- Remove check for runtime PM when setting next_event
- Fix kernel-test robot reported issue

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/
[2]. https://lore.kernel.org/linux-pm/20201012223400.23609-3-ilina@codeaurora.org/T/#u

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

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

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup
  2020-10-15 19:38 [PATCH v3 0/2] Better domain idle from device wakeup patterns Lina Iyer
@ 2020-10-15 19:38 ` Lina Iyer
  2020-10-16 16:55   ` Rafael J. Wysocki
  2020-10-15 19:38 ` [PATCH v3 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer
  1 sibling, 1 reply; 7+ messages in thread
From: Lina Iyer @ 2020-10-15 19:38 UTC (permalink / raw)
  To: rjw, ulf.hansson, linux-pm; +Cc: linux-arm-msm, 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 runtime PM of their next
event, the parent PM domain's idle duration can be determined.

So let's add the pm_runtime_set_next_wake() API for the device to notify
runtime PM of the impending wakeup and document it's usage.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v2:
	- Update documentation
	- Remove runtime PM enabled check
	- Update commit text
---
 Documentation/power/runtime_pm.rst | 17 +++++++++++++++++
 drivers/base/power/runtime.c       | 24 ++++++++++++++++++++++++
 include/linux/pm.h                 |  2 ++
 include/linux/pm_runtime.h         |  1 +
 4 files changed, 44 insertions(+)

diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
index 0553008b6279..f6aaef15a511 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -515,6 +515,12 @@ 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);`
+    - inform 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 domain governor would
+      use this information to calculate it's sleep length.
+
 It is safe to execute the following helper functions from interrupt context:
 
 - pm_request_idle()
@@ -545,6 +551,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 +646,16 @@ 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 and if device has a predictable interrupt pattern, we can even do a
+better job at determining the parent's idle state. For example, a display
+device gets a VSYNC interrupt every 16 ms when running at 60 Hz. When it's PM
+domain is powering down and happens to be at the boundary of the VSYNC
+interrupt, it may not be efficient to power off the domain. Knowing the next
+wake up (when available) for devices in the domain we can determine the idle
+duration of the domain. By comparing idle duration with the residencies of the
+domain idle states, we can be efficient in both power and performance.
+
 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..5d2ebacfd35e 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -122,6 +122,27 @@ 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;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+	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.
@@ -1415,6 +1436,9 @@ void pm_runtime_enable(struct device *dev)
 	     "Enabling runtime PM for inactive device (%s) with active children\n",
 	     dev_name(dev));
 
+	/* Reset the next wakeup for the device */
+	dev->power.next_event = KTIME_MAX;
+
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 }
 EXPORT_SYMBOL_GPL(pm_runtime_enable);
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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 2/2] PM / Domains: use device's next wakeup to determine domain idle state
  2020-10-15 19:38 [PATCH v3 0/2] Better domain idle from device wakeup patterns Lina Iyer
  2020-10-15 19:38 ` [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup Lina Iyer
@ 2020-10-15 19:38 ` Lina Iyer
  1 sibling, 0 replies; 7+ messages in thread
From: Lina Iyer @ 2020-10-15 19:38 UTC (permalink / raw)
  To: rjw, ulf.hansson, linux-pm; +Cc: linux-arm-msm, 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 v2:
	- Fix state_idx type to hold negative value.
	- Update commit text.
---
 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..2383421290f2 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;
+	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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup
  2020-10-15 19:38 ` [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup Lina Iyer
@ 2020-10-16 16:55   ` Rafael J. Wysocki
  2020-10-19 10:00     ` Ulf Hansson
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2020-10-16 16:55 UTC (permalink / raw)
  To: Lina Iyer; +Cc: Rafael J. Wysocki, Ulf Hansson, Linux PM, linux-arm-msm

On Thu, Oct 15, 2020 at 9:38 PM 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 runtime PM of their next
> event, the parent PM domain's idle duration can be determined.
>
> So let's add the pm_runtime_set_next_wake() API for the device to notify
> runtime PM of the impending wakeup and document it's usage.
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v2:
>         - Update documentation
>         - Remove runtime PM enabled check
>         - Update commit text
> ---
>  Documentation/power/runtime_pm.rst | 17 +++++++++++++++++
>  drivers/base/power/runtime.c       | 24 ++++++++++++++++++++++++
>  include/linux/pm.h                 |  2 ++
>  include/linux/pm_runtime.h         |  1 +
>  4 files changed, 44 insertions(+)
>
> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> index 0553008b6279..f6aaef15a511 100644
> --- a/Documentation/power/runtime_pm.rst
> +++ b/Documentation/power/runtime_pm.rst
> @@ -515,6 +515,12 @@ 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);`
> +    - inform 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 domain governor would
> +      use this information to calculate it's sleep length.
> +
>  It is safe to execute the following helper functions from interrupt context:
>
>  - pm_request_idle()
> @@ -545,6 +551,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 +646,16 @@ 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 and if device has a predictable interrupt pattern, we can even do a
> +better job at determining the parent's idle state. For example, a display
> +device gets a VSYNC interrupt every 16 ms when running at 60 Hz. When it's PM
> +domain is powering down and happens to be at the boundary of the VSYNC
> +interrupt, it may not be efficient to power off the domain. Knowing the next
> +wake up (when available) for devices in the domain we can determine the idle
> +duration of the domain. By comparing idle duration with the residencies of the
> +domain idle states, we can be efficient in both power and performance.
> +
>  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..5d2ebacfd35e 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -122,6 +122,27 @@ 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;
> +
> +       spin_lock_irqsave(&dev->power.lock, flags);
> +       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.
> @@ -1415,6 +1436,9 @@ void pm_runtime_enable(struct device *dev)
>              "Enabling runtime PM for inactive device (%s) with active children\n",
>              dev_name(dev));
>
> +       /* Reset the next wakeup for the device */
> +       dev->power.next_event = KTIME_MAX;
> +
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>  }
>  EXPORT_SYMBOL_GPL(pm_runtime_enable);
> 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;

While there are some cosmetic changes to be made, this particular bit
is fundamentally questionable IMV, because next_event (which BTW would
better be called next_wakeup IMO) is not used by PM-runtime.

The only user of it will be genpd AFAICS, so I don't quite see a
reason to inflict this extra memory cost on everybody, even if they
don't care about genpd and may not even compile it in.

>  #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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup
  2020-10-16 16:55   ` Rafael J. Wysocki
@ 2020-10-19 10:00     ` Ulf Hansson
  2020-10-19 10:21       ` Rafael J. Wysocki
  0 siblings, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2020-10-19 10:00 UTC (permalink / raw)
  To: Rafael J. Wysocki, Lina Iyer; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm

On Fri, 16 Oct 2020 at 18:55, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Oct 15, 2020 at 9:38 PM 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 runtime PM of their next
> > event, the parent PM domain's idle duration can be determined.
> >
> > So let's add the pm_runtime_set_next_wake() API for the device to notify
> > runtime PM of the impending wakeup and document it's usage.
> >
> > Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> > ---
> > Changes in v2:
> >         - Update documentation
> >         - Remove runtime PM enabled check
> >         - Update commit text
> > ---
> >  Documentation/power/runtime_pm.rst | 17 +++++++++++++++++
> >  drivers/base/power/runtime.c       | 24 ++++++++++++++++++++++++
> >  include/linux/pm.h                 |  2 ++
> >  include/linux/pm_runtime.h         |  1 +
> >  4 files changed, 44 insertions(+)
> >
> > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> > index 0553008b6279..f6aaef15a511 100644
> > --- a/Documentation/power/runtime_pm.rst
> > +++ b/Documentation/power/runtime_pm.rst
> > @@ -515,6 +515,12 @@ 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);`
> > +    - inform 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 domain governor would
> > +      use this information to calculate it's sleep length.
> > +
> >  It is safe to execute the following helper functions from interrupt context:
> >
> >  - pm_request_idle()
> > @@ -545,6 +551,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 +646,16 @@ 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 and if device has a predictable interrupt pattern, we can even do a
> > +better job at determining the parent's idle state. For example, a display
> > +device gets a VSYNC interrupt every 16 ms when running at 60 Hz. When it's PM
> > +domain is powering down and happens to be at the boundary of the VSYNC
> > +interrupt, it may not be efficient to power off the domain. Knowing the next
> > +wake up (when available) for devices in the domain we can determine the idle
> > +duration of the domain. By comparing idle duration with the residencies of the
> > +domain idle states, we can be efficient in both power and performance.
> > +
> >  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..5d2ebacfd35e 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -122,6 +122,27 @@ 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;
> > +
> > +       spin_lock_irqsave(&dev->power.lock, flags);
> > +       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.
> > @@ -1415,6 +1436,9 @@ void pm_runtime_enable(struct device *dev)
> >              "Enabling runtime PM for inactive device (%s) with active children\n",
> >              dev_name(dev));
> >
> > +       /* Reset the next wakeup for the device */
> > +       dev->power.next_event = KTIME_MAX;
> > +
> >         spin_unlock_irqrestore(&dev->power.lock, flags);
> >  }
> >  EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > 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;
>
> While there are some cosmetic changes to be made, this particular bit
> is fundamentally questionable IMV, because next_event (which BTW would
> better be called next_wakeup IMO) is not used by PM-runtime.
>
> The only user of it will be genpd AFAICS, so I don't quite see a
> reason to inflict this extra memory cost on everybody, even if they
> don't care about genpd and may not even compile it in.

That's a good point!

May I suggest that the new data is put into the "struct
generic_pm_domain_data" instead, which means it will be allocated when
a device is attached to a genpd.

Moreover, we should probably rename the API (and move the
implementation of it accordingly) from pm_runtime_set_next_event() to
dev_pm_genpd_set_next_wakeup(). Unless we believe the interface could
be useful for other PM domain types (ACPI ?), then we could consider
adding a ->set_next_wakeup() callback to the struct dev_pm_domain and
implement the interface through a common
dev_pm_domain_set_next_wakeup() API.

>
> >  #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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup
  2020-10-19 10:00     ` Ulf Hansson
@ 2020-10-19 10:21       ` Rafael J. Wysocki
  2020-10-19 17:02         ` Lina Iyer
  0 siblings, 1 reply; 7+ messages in thread
From: Rafael J. Wysocki @ 2020-10-19 10:21 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rafael J. Wysocki, Lina Iyer, Rafael J. Wysocki, Linux PM, linux-arm-msm

On Mon, Oct 19, 2020 at 12:01 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 16 Oct 2020 at 18:55, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Oct 15, 2020 at 9:38 PM 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 runtime PM of their next
> > > event, the parent PM domain's idle duration can be determined.
> > >
> > > So let's add the pm_runtime_set_next_wake() API for the device to notify
> > > runtime PM of the impending wakeup and document it's usage.
> > >
> > > Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> > > ---
> > > Changes in v2:
> > >         - Update documentation
> > >         - Remove runtime PM enabled check
> > >         - Update commit text
> > > ---
> > >  Documentation/power/runtime_pm.rst | 17 +++++++++++++++++
> > >  drivers/base/power/runtime.c       | 24 ++++++++++++++++++++++++
> > >  include/linux/pm.h                 |  2 ++
> > >  include/linux/pm_runtime.h         |  1 +
> > >  4 files changed, 44 insertions(+)
> > >
> > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> > > index 0553008b6279..f6aaef15a511 100644
> > > --- a/Documentation/power/runtime_pm.rst
> > > +++ b/Documentation/power/runtime_pm.rst
> > > @@ -515,6 +515,12 @@ 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);`
> > > +    - inform 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 domain governor would
> > > +      use this information to calculate it's sleep length.
> > > +
> > >  It is safe to execute the following helper functions from interrupt context:
> > >
> > >  - pm_request_idle()
> > > @@ -545,6 +551,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 +646,16 @@ 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 and if device has a predictable interrupt pattern, we can even do a
> > > +better job at determining the parent's idle state. For example, a display
> > > +device gets a VSYNC interrupt every 16 ms when running at 60 Hz. When it's PM
> > > +domain is powering down and happens to be at the boundary of the VSYNC
> > > +interrupt, it may not be efficient to power off the domain. Knowing the next
> > > +wake up (when available) for devices in the domain we can determine the idle
> > > +duration of the domain. By comparing idle duration with the residencies of the
> > > +domain idle states, we can be efficient in both power and performance.
> > > +
> > >  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..5d2ebacfd35e 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -122,6 +122,27 @@ 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;
> > > +
> > > +       spin_lock_irqsave(&dev->power.lock, flags);
> > > +       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.
> > > @@ -1415,6 +1436,9 @@ void pm_runtime_enable(struct device *dev)
> > >              "Enabling runtime PM for inactive device (%s) with active children\n",
> > >              dev_name(dev));
> > >
> > > +       /* Reset the next wakeup for the device */
> > > +       dev->power.next_event = KTIME_MAX;
> > > +
> > >         spin_unlock_irqrestore(&dev->power.lock, flags);
> > >  }
> > >  EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > 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;
> >
> > While there are some cosmetic changes to be made, this particular bit
> > is fundamentally questionable IMV, because next_event (which BTW would
> > better be called next_wakeup IMO) is not used by PM-runtime.
> >
> > The only user of it will be genpd AFAICS, so I don't quite see a
> > reason to inflict this extra memory cost on everybody, even if they
> > don't care about genpd and may not even compile it in.
>
> That's a good point!
>
> May I suggest that the new data is put into the "struct
> generic_pm_domain_data" instead, which means it will be allocated when
> a device is attached to a genpd.

Yes, something like that.

> Moreover, we should probably rename the API (and move the
> implementation of it accordingly) from pm_runtime_set_next_event() to
> dev_pm_genpd_set_next_wakeup().

Right.

> Unless we believe the interface could
> be useful for other PM domain types (ACPI ?), then we could consider
> adding a ->set_next_wakeup() callback to the struct dev_pm_domain and
> implement the interface through a common
> dev_pm_domain_set_next_wakeup() API.

Maybe.

That would depend on who the other user would be and I wouldn't worry
about that upfront.

Cheers!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup
  2020-10-19 10:21       ` Rafael J. Wysocki
@ 2020-10-19 17:02         ` Lina Iyer
  0 siblings, 0 replies; 7+ messages in thread
From: Lina Iyer @ 2020-10-19 17:02 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Ulf Hansson, Rafael J. Wysocki, Linux PM, linux-arm-msm

On Mon, Oct 19 2020 at 04:21 -0600, Rafael J. Wysocki wrote:
>On Mon, Oct 19, 2020 at 12:01 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On Fri, 16 Oct 2020 at 18:55, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >
>> > On Thu, Oct 15, 2020 at 9:38 PM 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 runtime PM of their next
>> > > event, the parent PM domain's idle duration can be determined.
>> > >
>> > > So let's add the pm_runtime_set_next_wake() API for the device to notify
>> > > runtime PM of the impending wakeup and document it's usage.
>> > >
>> > > Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> > > ---
>> > > Changes in v2:
>> > >         - Update documentation
>> > >         - Remove runtime PM enabled check
>> > >         - Update commit text
>> > > ---
>> > >  Documentation/power/runtime_pm.rst | 17 +++++++++++++++++
>> > >  drivers/base/power/runtime.c       | 24 ++++++++++++++++++++++++
>> > >  include/linux/pm.h                 |  2 ++
>> > >  include/linux/pm_runtime.h         |  1 +
>> > >  4 files changed, 44 insertions(+)
>> > >
>> > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
>> > > index 0553008b6279..f6aaef15a511 100644
>> > > --- a/Documentation/power/runtime_pm.rst
>> > > +++ b/Documentation/power/runtime_pm.rst
>> > > @@ -515,6 +515,12 @@ 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);`
>> > > +    - inform 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 domain governor would
>> > > +      use this information to calculate it's sleep length.
>> > > +
>> > >  It is safe to execute the following helper functions from interrupt context:
>> > >
>> > >  - pm_request_idle()
>> > > @@ -545,6 +551,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 +646,16 @@ 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 and if device has a predictable interrupt pattern, we can even do a
>> > > +better job at determining the parent's idle state. For example, a display
>> > > +device gets a VSYNC interrupt every 16 ms when running at 60 Hz. When it's PM
>> > > +domain is powering down and happens to be at the boundary of the VSYNC
>> > > +interrupt, it may not be efficient to power off the domain. Knowing the next
>> > > +wake up (when available) for devices in the domain we can determine the idle
>> > > +duration of the domain. By comparing idle duration with the residencies of the
>> > > +domain idle states, we can be efficient in both power and performance.
>> > > +
>> > >  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..5d2ebacfd35e 100644
>> > > --- a/drivers/base/power/runtime.c
>> > > +++ b/drivers/base/power/runtime.c
>> > > @@ -122,6 +122,27 @@ 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;
>> > > +
>> > > +       spin_lock_irqsave(&dev->power.lock, flags);
>> > > +       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.
>> > > @@ -1415,6 +1436,9 @@ void pm_runtime_enable(struct device *dev)
>> > >              "Enabling runtime PM for inactive device (%s) with active children\n",
>> > >              dev_name(dev));
>> > >
>> > > +       /* Reset the next wakeup for the device */
>> > > +       dev->power.next_event = KTIME_MAX;
>> > > +
>> > >         spin_unlock_irqrestore(&dev->power.lock, flags);
>> > >  }
>> > >  EXPORT_SYMBOL_GPL(pm_runtime_enable);
>> > > 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;
>> >
>> > While there are some cosmetic changes to be made, this particular bit
>> > is fundamentally questionable IMV, because next_event (which BTW would
>> > better be called next_wakeup IMO) is not used by PM-runtime.
>> >
>> > The only user of it will be genpd AFAICS, so I don't quite see a
>> > reason to inflict this extra memory cost on everybody, even if they
>> > don't care about genpd and may not even compile it in.
>>
>> That's a good point!
>>
>> May I suggest that the new data is put into the "struct
>> generic_pm_domain_data" instead, which means it will be allocated when
>> a device is attached to a genpd.
>
>Yes, something like that.
>
>> Moreover, we should probably rename the API (and move the
>> implementation of it accordingly) from pm_runtime_set_next_event() to
>> dev_pm_genpd_set_next_wakeup().
>
>Right.
>
Thanks, both of you for the suggestions. I will send an update soon.

--Lina

>> Unless we believe the interface could
>> be useful for other PM domain types (ACPI ?), then we could consider
>> adding a ->set_next_wakeup() callback to the struct dev_pm_domain and
>> implement the interface through a common
>> dev_pm_domain_set_next_wakeup() API.
>
>Maybe.
>
>That would depend on who the other user would be and I wouldn't worry
>about that upfront.
>
>Cheers!

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 19:38 [PATCH v3 0/2] Better domain idle from device wakeup patterns Lina Iyer
2020-10-15 19:38 ` [PATCH v3 1/2] PM / runtime: inform runtime PM of a device's next wakeup Lina Iyer
2020-10-16 16:55   ` Rafael J. Wysocki
2020-10-19 10:00     ` Ulf Hansson
2020-10-19 10:21       ` Rafael J. Wysocki
2020-10-19 17:02         ` Lina Iyer
2020-10-15 19:38 ` [PATCH v3 2/2] PM / Domains: use device's next wakeup to determine domain idle state Lina Iyer

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git