[PATCH v2 0/5] PM / Domains / Runtime: Optimize device system PM when using genpd

[PATCH v2 0/5] PM / Domains / Runtime: Optimize device system PM when using genpd

[Ulf Hansson]

The main goal of this series is to avoid unnecessary operations of devices
during the system PM suspend/resume path, especially when using genpd.

For example, we want to avoid to have genpd to unconditionally runtime resume
devices in the system PM prepare phase. That also means that in most cases a
runtime suspended device can remain in such state, during the system PM suspend
sequence.

Moreover, we don't want to unconditionally wake up all devices during the system
PM resume sequence, but instead defer that to be done when devices are actually
going to be used.

Note, this series is based upon another posted series (not queued) for genpd:
[PATCH v3 0/2] PM / Domains: Second step in improving system PM code in genpd
http://www.spinics.net/lists/dri-devel/msg107207.html

Ulf Hansson (5):
  PM / Domains: Remove redundant call to pm_request_idle() in genpd
  PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()
  PM / Domains: Allow runtime PM during system PM phases
  PM / Domains: Stop/start devices during system PM suspend/resume in
    genpd
  PM / Runtime: Defer resuming of the device in
    pm_runtime_force_resume()

 drivers/base/power/domain.c  | 56 ++++++++++++++++++++++++--------------------
 drivers/base/power/runtime.c | 14 +++++++++++
 2 files changed, 44 insertions(+), 26 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] PM / Domains: Remove redundant call to pm_request_idle() in genpd

[Ulf Hansson]

The PM core increases the runtime PM usage count at the system PM prepare
phase. Later when the system resumes, it does a pm_runtime_put() in the
complete phase, which in addition to decrementing the usage count, does
the equivalent of a pm_request_idle().

Therefore the call to pm_request_idle() from within genpd's ->complete()
callback is redundant, so let's remove it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Updated changelog.

---
 drivers/base/power/domain.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 658eb1b..60a9971 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -938,7 +938,6 @@ static void pm_genpd_complete(struct device *dev)
 	pm_generic_complete(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-	pm_request_idle(dev);
 }
 
 /**
-- 
1.9.1

[PATCH v2 2/5] PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()

[Ulf Hansson]

If the runtime PM status of the device isn't RPM_SUSPENDED, prevent the
pm_runtime_force_resume() to call the ->runtime_resume() callback for the
device, as it's not the expected behaviour from the subsystem/driver.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Updated changelog.

---
 drivers/base/power/runtime.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b746904..09e4eb1 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1506,6 +1506,9 @@ int pm_runtime_force_resume(struct device *dev)
 		goto out;
 	}
 
+	if (!pm_runtime_status_suspended(dev))
+		goto out;
+
 	ret = pm_runtime_set_active(dev);
 	if (ret)
 		goto out;
-- 
1.9.1

[PATCH v2 3/5] PM / Domains: Allow runtime PM during system PM phases

[Ulf Hansson]

In cases when the PM domain isn't powered off when genpd's ->prepare()
callback is invoked, genpd runtime resumes and disables runtime PM for the
device. This behaviour was needed when genpd managed intermediate states
during the power off sequence, as to maintain proper low power states of
devices during system PM suspend/resume.

The commit ba2bbfbf6307 ("PM / Domains: Remove intermediate states from
the power off sequence"), enables genpd to improve its behaviour in this
regards.

The PM core disables runtime PM at __device_suspend_late() before it calls
a system PM "late" callback for a device. When resuming a device, after a
corresponding "early" callback has been invoked, the PM core re-enables
runtime PM.

By changing genpd to allow runtime PM according to the same system PM
phases as the PM core, devices can be runtime resumed by its corresponding
subsystem/driver when really needed.

In this way, genpd no longer need to runtime resume the device from its
->prepare() callback. In most cases that avoids unnecessary and energy-
wasting operations of runtime resuming devices that have nothing to do,
only to runtime suspend them shortly after.

Although, because of changing this behaviour in genpd and due to that
genpd powers on the PM domain unconditionally in the system PM resume
"noirq" phase, it could potentially cause a PM domain to stay powered on
even if it's unused after the system has resumed. To avoid this, let's
schedule a power off work when genpd's system PM ->complete() callback has
been invoked for the last device in the PM domain.

Another issue that arises due to this change in genpd, concerns those
platforms/PM domains that makes use of genpd's device ->stop|start()
callbacks. In these scenarios, the corresponding subsystem/driver needs to
invoke pm_runtime_force_suspend() from a system PM suspend callback to
allow genpd's ->runtime_suspend() to be invoked for an active device, else
genpd won't "stop" a device that has been "started".

The subsystem/driver also needs to invoke pm_runtime_force_resume()
in a system PM resume callback, to restore the runtime PM state for the
device and to re-enable runtime PM.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Updated changelog.
	- Split patch into two pieces, to deal with stop/start issues from a
	separate change on top of this one.

---
 drivers/base/power/domain.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 60a9971..4cb57f3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -739,21 +739,6 @@ static int pm_genpd_prepare(struct device *dev)
 
 	mutex_unlock(&genpd->lock);
 
-	/*
-	 * Even if the PM domain is powered off at this point, we can't expect
-	 * it to remain in that state during the entire system PM suspend
-	 * phase. Any subsystem/driver for a device in the PM domain, may still
-	 * need to serve a request which may require the device to be runtime
-	 * resumed and its PM domain to be powered.
-	 *
-	 * As we are disabling runtime PM at this point, we are preventing the
-	 * subsystem/driver to decide themselves. For that reason, we need to
-	 * make sure the device is operational as it may be required in some
-	 * cases.
-	 */
-	pm_runtime_resume(dev);
-	__pm_runtime_disable(dev, false);
-
 	ret = pm_generic_prepare(dev);
 	if (ret) {
 		mutex_lock(&genpd->lock);
@@ -761,7 +746,6 @@ static int pm_genpd_prepare(struct device *dev)
 		genpd->prepared_count--;
 
 		mutex_unlock(&genpd->lock);
-		pm_runtime_enable(dev);
 	}
 
 	return ret;
@@ -787,8 +771,6 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
 		return 0;
 
-	genpd_stop_dev(genpd, dev);
-
 	/*
 	 * Since all of the "noirq" callbacks are executed sequentially, it is
 	 * guaranteed that this function will never run twice in parallel for
@@ -827,7 +809,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	pm_genpd_sync_poweron(genpd, true);
 	genpd->suspended_count--;
 
-	return genpd_start_dev(genpd, dev);
+	return 0;
 }
 
 /**
@@ -849,7 +831,7 @@ static int pm_genpd_freeze_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd_stop_dev(genpd, dev);
+	return 0;
 }
 
 /**
@@ -869,7 +851,7 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd_start_dev(genpd, dev);
+	return 0;
 }
 
 /**
@@ -907,7 +889,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
 
 	pm_genpd_sync_poweron(genpd, true);
 
-	return genpd_start_dev(genpd, dev);
+	return 0;
 }
 
 /**
@@ -929,15 +911,15 @@ static void pm_genpd_complete(struct device *dev)
 	if (IS_ERR(genpd))
 		return;
 
+	pm_generic_complete(dev);
+
 	mutex_lock(&genpd->lock);
 
 	genpd->prepared_count--;
+	if (!genpd->prepared_count)
+		genpd_queue_power_off_work(genpd);
 
 	mutex_unlock(&genpd->lock);
-
-	pm_generic_complete(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
 }
 
 /**
-- 
1.9.1

[PATCH v2 4/5] PM / Domains: Stop/start devices during system PM suspend/resume in genpd

[Ulf Hansson]

Not all subsystems/drivers that manages devices attached to a genpd, makes
use of the pm_runtime_force_suspend|resume() helper functions to deal with
system PM suspend/resume.

In cases like these and when genpd's ->stop|start() callbacks are being
used for the device, invoke the pm_runtime_force_suspend|resume() helper
functions from genpd's "noirq" system PM callbacks. In this way we make
sure to "stop" the device on suspend and to "start" it on resume.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- New patch. Implements the second part of the earlier version of
	[PATCH 3/4] PM / Domains: Allow runtime PM during system PM phases

---
 drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4cb57f3..9193aac 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -761,6 +761,7 @@ static int pm_genpd_prepare(struct device *dev)
 static int pm_genpd_suspend_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -771,6 +772,12 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
 		return 0;
 
+	if (genpd->dev_ops.stop && genpd->dev_ops.start) {
+		ret = pm_runtime_force_suspend(dev);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * Since all of the "noirq" callbacks are executed sequentially, it is
 	 * guaranteed that this function will never run twice in parallel for
@@ -791,6 +798,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 static int pm_genpd_resume_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -809,7 +817,10 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	pm_genpd_sync_poweron(genpd, true);
 	genpd->suspended_count--;
 
-	return 0;
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_resume(dev);
+
+	return ret;
 }
 
 /**
@@ -824,6 +835,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 static int pm_genpd_freeze_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -831,7 +843,10 @@ static int pm_genpd_freeze_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return 0;
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_suspend(dev);
+
+	return ret;
 }
 
 /**
@@ -844,6 +859,7 @@ static int pm_genpd_freeze_noirq(struct device *dev)
 static int pm_genpd_thaw_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -851,7 +867,10 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return 0;
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_resume(dev);
+
+	return ret;
 }
 
 /**
@@ -864,6 +883,7 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 static int pm_genpd_restore_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
+	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -889,7 +909,10 @@ static int pm_genpd_restore_noirq(struct device *dev)
 
 	pm_genpd_sync_poweron(genpd, true);
 
-	return 0;
+	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+		ret = pm_runtime_force_resume(dev);
+
+	return ret;
 }
 
 /**
-- 
1.9.1

[PATCH v2 5/5] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

When the pm_runtime_force_suspend|resume() helpers were invented, we still
had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.

To make sure these helpers worked for all combinations and without
introducing too much of complexity, the device was always resumed in
pm_runtime_force_resume().

More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
unset, we needed to resume the device as the subsystem/driver couldn't
rely on using runtime PM to do it.

As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
removed this combination, of using CONFIG_PM_SLEEP without the earlier
CONFIG_PM_RUNTIME.

For this reason we can now rely on the subsystem/driver to use runtime PM
to resume the device, instead of forcing that to be done in all cases. In
other words, let's defer the runtime resume to a later point when it's
actually needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Updated changelog.
	- Updated comment in the code.

---
 drivers/base/power/runtime.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 09e4eb1..81731a2 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1509,6 +1509,17 @@ int pm_runtime_force_resume(struct device *dev)
 	if (!pm_runtime_status_suspended(dev))
 		goto out;
 
+	/*
+	 * The PM core increases the runtime PM usage count in the system PM
+	 * prepare phase. If the count is greater than 1 at this point, a real
+	 * user (such as a subsystem, driver, userspace, etc.) has also
+	 * increased it, indicating that the device was used when system suspend
+	 * was invoked. In this case, the device is expected to be used on
+	 * system resume as well, so invoke the ->runtime_resume() callback.
+	 */
+	if (atomic_read(&dev->power.usage_count) < 2)
+		goto out;
+
 	ret = pm_runtime_set_active(dev);
 	if (ret)
 		goto out;
-- 
1.9.1

Re: [PATCH v2 1/5] PM / Domains: Remove redundant call to pm_request_idle() in genpd

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> The PM core increases the runtime PM usage count at the system PM prepare
> phase. Later when the system resumes, it does a pm_runtime_put() in the
> complete phase, which in addition to decrementing the usage count, does
> the equivalent of a pm_request_idle().
>
> Therefore the call to pm_request_idle() from within genpd's ->complete()
> callback is redundant, so let's remove it.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Re: [PATCH v2 2/5] PM / Runtime: Prevent re-resuming devices in pm_runtime_force_resume()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> If the runtime PM status of the device isn't RPM_SUSPENDED, prevent the
> pm_runtime_force_resume() to call the ->runtime_resume() callback for the
> device, as it's not the expected behaviour from the subsystem/driver.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Re: [PATCH v2 3/5] PM / Domains: Allow runtime PM during system PM phases

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> In cases when the PM domain isn't powered off when genpd's ->prepare()
> callback is invoked, genpd runtime resumes and disables runtime PM for the
> device. This behaviour was needed when genpd managed intermediate states
> during the power off sequence, as to maintain proper low power states of
> devices during system PM suspend/resume.
>
> The commit ba2bbfbf6307 ("PM / Domains: Remove intermediate states from
> the power off sequence"), enables genpd to improve its behaviour in this
> regards.
>
> The PM core disables runtime PM at __device_suspend_late() before it calls
> a system PM "late" callback for a device. When resuming a device, after a
> corresponding "early" callback has been invoked, the PM core re-enables
> runtime PM.
>
> By changing genpd to allow runtime PM according to the same system PM
> phases as the PM core, devices can be runtime resumed by its corresponding

grammar nit: s/its/their/

> subsystem/driver when really needed.
>
> In this way, genpd no longer need to runtime resume the device from its
> ->prepare() callback. In most cases that avoids unnecessary and energy-
> wasting operations of runtime resuming devices that have nothing to do,
> only to runtime suspend them shortly after.
>
> Although, because of changing this behaviour in genpd and due to that
> genpd powers on the PM domain unconditionally in the system PM resume
> "noirq" phase, it could potentially cause a PM domain to stay powered on
> even if it's unused after the system has resumed. To avoid this, let's
> schedule a power off work when genpd's system PM ->complete() callback has
> been invoked for the last device in the PM domain.

IIUC, this should be the end of the changelog for this patch.  The rest
was moved to a new patch, no?

> Another issue that arises due to this change in genpd, concerns those
> platforms/PM domains that makes use of genpd's device ->stop|start()
> callbacks. In these scenarios, the corresponding subsystem/driver needs to
> invoke pm_runtime_force_suspend() from a system PM suspend callback to
> allow genpd's ->runtime_suspend() to be invoked for an active device, else
> genpd won't "stop" a device that has been "started".
>
> The subsystem/driver also needs to invoke pm_runtime_force_resume()
> in a system PM resume callback, to restore the runtime PM state for the
> device and to re-enable runtime PM.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Other than the changelog nits above, LGTM.

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Kevin

Re: [PATCH v2 4/5] PM / Domains: Stop/start devices during system PM suspend/resume in genpd

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> Not all subsystems/drivers that manages devices attached to a genpd, makes
> use of the pm_runtime_force_suspend|resume() helper functions to deal with
> system PM suspend/resume.
>
> In cases like these and when genpd's ->stop|start() callbacks are being
> used for the device, invoke the pm_runtime_force_suspend|resume() helper
> functions from genpd's "noirq" system PM callbacks. In this way we make
> sure to "stop" the device on suspend and to "start" it on resume.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Re: [PATCH v2 5/5] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> When the pm_runtime_force_suspend|resume() helpers were invented, we still
> had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig options.
>
> To make sure these helpers worked for all combinations and without
> introducing too much of complexity, the device was always resumed in
> pm_runtime_force_resume().
>
> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> unset, we needed to resume the device as the subsystem/driver couldn't
> rely on using runtime PM to do it.
>
> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> removed this combination, of using CONFIG_PM_SLEEP without the earlier
> CONFIG_PM_RUNTIME.
>
> For this reason we can now rely on the subsystem/driver to use runtime PM
> to resume the device, instead of forcing that to be done in all cases. In
> other words, let's defer the runtime resume to a later point when it's
> actually needed.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Re: [PATCH v2 3/5] PM / Domains: Allow runtime PM during system PM phases

[Ulf Hansson]

On 15 June 2016 at 23:38, Kevin Hilman <khilman@baylibre.com> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> In cases when the PM domain isn't powered off when genpd's ->prepare()
>> callback is invoked, genpd runtime resumes and disables runtime PM for the
>> device. This behaviour was needed when genpd managed intermediate states
>> during the power off sequence, as to maintain proper low power states of
>> devices during system PM suspend/resume.
>>
>> The commit ba2bbfbf6307 ("PM / Domains: Remove intermediate states from
>> the power off sequence"), enables genpd to improve its behaviour in this
>> regards.
>>
>> The PM core disables runtime PM at __device_suspend_late() before it calls
>> a system PM "late" callback for a device. When resuming a device, after a
>> corresponding "early" callback has been invoked, the PM core re-enables
>> runtime PM.
>>
>> By changing genpd to allow runtime PM according to the same system PM
>> phases as the PM core, devices can be runtime resumed by its corresponding
>
> grammar nit: s/its/their/
>
>> subsystem/driver when really needed.
>>
>> In this way, genpd no longer need to runtime resume the device from its
>> ->prepare() callback. In most cases that avoids unnecessary and energy-
>> wasting operations of runtime resuming devices that have nothing to do,
>> only to runtime suspend them shortly after.
>>
>> Although, because of changing this behaviour in genpd and due to that
>> genpd powers on the PM domain unconditionally in the system PM resume
>> "noirq" phase, it could potentially cause a PM domain to stay powered on
>> even if it's unused after the system has resumed. To avoid this, let's
>> schedule a power off work when genpd's system PM ->complete() callback has
>> been invoked for the last device in the PM domain.
>
> IIUC, this should be the end of the changelog for this patch.  The rest
> was moved to a new patch, no?
>
>> Another issue that arises due to this change in genpd, concerns those
>> platforms/PM domains that makes use of genpd's device ->stop|start()
>> callbacks. In these scenarios, the corresponding subsystem/driver needs to
>> invoke pm_runtime_force_suspend() from a system PM suspend callback to
>> allow genpd's ->runtime_suspend() to be invoked for an active device, else
>> genpd won't "stop" a device that has been "started".
>>
>> The subsystem/driver also needs to invoke pm_runtime_force_resume()
>> in a system PM resume callback, to restore the runtime PM state for the
>> device and to re-enable runtime PM.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Other than the changelog nits above, LGTM.
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>
> Kevin

Kevin, thanks for the review(s)!

Rafael, I noticed you have queued the two series and amended the
change-log according to the comments from Kevin. Thanks!

Kind regards
Uffe