[PATCH 1/2] PM / Domains: Don't measure ->start|stop() latency in system PM callbacks

[PATCH 1/2] PM / Domains: Don't measure ->start|stop() latency in system PM callbacks

[Ulf Hansson]

Measure latency does by itself contribute to an increased latency, thus we
should avoid it when it isn't needed.

Genpd measures latencies in the system PM phase for the ->start|stop()
callbacks and is thus affecting the system PM suspend/resume time.
Moreover these latencies are validated only at runtime PM suspend/resume.

To this reasoning, let's decide to leave these measurements out of the
system PM phase. There should be plenty of occasions during runtime PM to
perform these measurements anyway.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 6e1bcde..a1c3ec4 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -90,8 +90,12 @@ static struct generic_pm_domain *dev_to_genpd(struct device *dev)
 	return pd_to_genpd(dev->pm_domain);
 }
 
-static int genpd_stop_dev(struct generic_pm_domain *genpd, struct device *dev)
+static int genpd_stop_dev(struct generic_pm_domain *genpd, struct device *dev,
+			bool timed)
 {
+	if (!timed)
+		return GENPD_DEV_CALLBACK(genpd, int, stop, dev);
+
 	return GENPD_DEV_TIMED_CALLBACK(genpd, int, stop, dev,
 					stop_latency_ns, "stop");
 }
@@ -434,7 +438,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = genpd_stop_dev(genpd, dev);
+	ret = genpd_stop_dev(genpd, dev, true);
 	if (ret) {
 		genpd_restore_dev(genpd, dev, true);
 		return ret;
@@ -779,7 +783,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	    || (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
 		return 0;
 
-	genpd_stop_dev(genpd, dev);
+	genpd_stop_dev(genpd, dev, false);
 
 	/*
 	 * Since all of the "noirq" callbacks are executed sequentially, it is
@@ -820,7 +824,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, true);
+	return genpd_start_dev(genpd, dev, false);
 }
 
 /**
@@ -928,7 +932,7 @@ static int pm_genpd_freeze_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : genpd_stop_dev(genpd, dev);
+	return genpd->suspend_power_off ? 0 : genpd_stop_dev(genpd, dev, false);
 }
 
 /**
@@ -948,7 +952,8 @@ static int pm_genpd_thaw_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	return genpd->suspend_power_off ? 0 : genpd_start_dev(genpd, dev, true);
+	return genpd->suspend_power_off ?
+		0 : genpd_start_dev(genpd, dev, false);
 }
 
 /**
@@ -1042,7 +1047,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
 
 	pm_genpd_sync_poweron(genpd, true);
 
-	return genpd_start_dev(genpd, dev, true);
+	return genpd_start_dev(genpd, dev, false);
 }
 
 /**
-- 
1.9.1

Re: [PATCH 1/2] PM / Domains: Don't measure ->start|stop() latency in system PM callbacks

[Lina Iyer]

On Thu, Oct 15 2015 at 09:02 -0600, Ulf Hansson wrote:
>Measure latency does by itself contribute to an increased latency, thus we
>should avoid it when it isn't needed.
>
>Genpd measures latencies in the system PM phase for the ->start|stop()
>callbacks and is thus affecting the system PM suspend/resume time.
>Moreover these latencies are validated only at runtime PM suspend/resume.
>
>To this reasoning, let's decide to leave these measurements out of the
>system PM phase. There should be plenty of occasions during runtime PM to
>perform these measurements anyway.
>
>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Lina Iyer <lina.iyer@linaro.org>

Thanks,
Lina

>---
> drivers/base/power/domain.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>index 6e1bcde..a1c3ec4 100644
>--- a/drivers/base/power/domain.c
>+++ b/drivers/base/power/domain.c
>@@ -90,8 +90,12 @@ static struct generic_pm_domain *dev_to_genpd(struct device *dev)
> 	return pd_to_genpd(dev->pm_domain);
> }
>
>-static int genpd_stop_dev(struct generic_pm_domain *genpd, struct device *dev)
>+static int genpd_stop_dev(struct generic_pm_domain *genpd, struct device *dev,
>+			bool timed)
> {
>+	if (!timed)
>+		return GENPD_DEV_CALLBACK(genpd, int, stop, dev);
>+
> 	return GENPD_DEV_TIMED_CALLBACK(genpd, int, stop, dev,
> 					stop_latency_ns, "stop");
> }
>@@ -434,7 +438,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
> 	if (ret)
> 		return ret;
>
>-	ret = genpd_stop_dev(genpd, dev);
>+	ret = genpd_stop_dev(genpd, dev, true);
> 	if (ret) {
> 		genpd_restore_dev(genpd, dev, true);
> 		return ret;
>@@ -779,7 +783,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
> 	    || (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
> 		return 0;
>
>-	genpd_stop_dev(genpd, dev);
>+	genpd_stop_dev(genpd, dev, false);
>
> 	/*
> 	 * Since all of the "noirq" callbacks are executed sequentially, it is
>@@ -820,7 +824,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, true);
>+	return genpd_start_dev(genpd, dev, false);
> }
>
> /**
>@@ -928,7 +932,7 @@ static int pm_genpd_freeze_noirq(struct device *dev)
> 	if (IS_ERR(genpd))
> 		return -EINVAL;
>
>-	return genpd->suspend_power_off ? 0 : genpd_stop_dev(genpd, dev);
>+	return genpd->suspend_power_off ? 0 : genpd_stop_dev(genpd, dev, false);
> }
>
> /**
>@@ -948,7 +952,8 @@ static int pm_genpd_thaw_noirq(struct device *dev)
> 	if (IS_ERR(genpd))
> 		return -EINVAL;
>
>-	return genpd->suspend_power_off ? 0 : genpd_start_dev(genpd, dev, true);
>+	return genpd->suspend_power_off ?
>+		0 : genpd_start_dev(genpd, dev, false);
> }
>
> /**
>@@ -1042,7 +1047,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
>
> 	pm_genpd_sync_poweron(genpd, true);
>
>-	return genpd_start_dev(genpd, dev, true);
>+	return genpd_start_dev(genpd, dev, false);
> }
>
> /**
>-- 
>1.9.1
>

Re: [PATCH 1/2] PM / Domains: Don't measure ->start|stop() latency in system PM callbacks

[Pavel Machek]

On Thu 2015-10-15 17:02:06, Ulf Hansson wrote:
> Measure latency does by itself contribute to an increased latency, thus we
> should avoid it when it isn't needed.
> 
> Genpd measures latencies in the system PM phase for the ->start|stop()
> callbacks and is thus affecting the system PM suspend/resume time.
> Moreover these latencies are validated only at runtime PM suspend/resume.
> 
> To this reasoning, let's decide to leave these measurements out of the
> system PM phase. There should be plenty of occasions during runtime PM to
> perform these measurements anyway.

How much latency does the latency measure cause? Something like 0.000
msec?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] PM / Domains: Don't measure ->start|stop() latency in system PM callbacks

[Ulf Hansson]

On 21 October 2015 at 12:31, Pavel Machek <pavel@ucw.cz> wrote:
> On Thu 2015-10-15 17:02:06, Ulf Hansson wrote:
>> Measure latency does by itself contribute to an increased latency, thus we
>> should avoid it when it isn't needed.
>>
>> Genpd measures latencies in the system PM phase for the ->start|stop()
>> callbacks and is thus affecting the system PM suspend/resume time.
>> Moreover these latencies are validated only at runtime PM suspend/resume.
>>
>> To this reasoning, let's decide to leave these measurements out of the
>> system PM phase. There should be plenty of occasions during runtime PM to
>> perform these measurements anyway.
>
> How much latency does the latency measure cause? Something like 0.000
> msec?

Me and Lina did some measurement by hand a couple of weeks ago. If I
remember correctly the results where in the range of a couple of us. I
will check again and update you with some refreshed data.

For system PM the time saved might be negligible, depending on the
number of devices that are attached to a genpd and on what level you
try to optimize things for.

Perhaps this change then becomes more of a policy decision, rather
than an a noticeable improvement. I certainly think dealing with
device PM QoS constraints should belong to runtime PM and I think it's
wrong/useless to do deal with this during system PM.

Kind regards
Uffe

Re: [PATCH 1/2] PM / Domains: Don't measure ->start|stop() latency in system PM callbacks

[Rafael J. Wysocki]

On Wednesday, October 21, 2015 02:47:12 PM Ulf Hansson wrote:
> On 21 October 2015 at 12:31, Pavel Machek <pavel@ucw.cz> wrote:
> > On Thu 2015-10-15 17:02:06, Ulf Hansson wrote:
> >> Measure latency does by itself contribute to an increased latency, thus we
> >> should avoid it when it isn't needed.
> >>
> >> Genpd measures latencies in the system PM phase for the ->start|stop()
> >> callbacks and is thus affecting the system PM suspend/resume time.
> >> Moreover these latencies are validated only at runtime PM suspend/resume.
> >>
> >> To this reasoning, let's decide to leave these measurements out of the
> >> system PM phase. There should be plenty of occasions during runtime PM to
> >> perform these measurements anyway.
> >
> > How much latency does the latency measure cause? Something like 0.000
> > msec?
> 
> Me and Lina did some measurement by hand a couple of weeks ago. If I
> remember correctly the results where in the range of a couple of us. I
> will check again and update you with some refreshed data.
> 
> For system PM the time saved might be negligible, depending on the
> number of devices that are attached to a genpd and on what level you
> try to optimize things for.
> 
> Perhaps this change then becomes more of a policy decision, rather
> than an a noticeable improvement. I certainly think dealing with
> device PM QoS constraints should belong to runtime PM and I think it's
> wrong/useless to do deal with this during system PM.

Right.  It should be used for runtime PM only.

Thanks,
Rafael

Re: [PATCH 1/2] PM / Domains: Don't measure ->start|stop() latency in system PM callbacks

[Kevin Hilman]

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

> Measure latency does by itself contribute to an increased latency, thus we
> should avoid it when it isn't needed.
>
> Genpd measures latencies in the system PM phase for the ->start|stop()
> callbacks and is thus affecting the system PM suspend/resume time.
> Moreover these latencies are validated only at runtime PM suspend/resume.
>
> To this reasoning, let's decide to leave these measurements out of the
> system PM phase. There should be plenty of occasions during runtime PM to
> perform these measurements anyway.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Acked-by: Kevin Hilman <khilman@linaro.org>