Patchwork PM / Domains: Make genpd performance states orthogonal to the idlestates

login
register
mail settings
Submitter Ulf Hansson
Date Dec. 6, 2018, 10:17 a.m.
Message ID <20181206101702.26418-1-ulf.hansson@linaro.org>
Download mbox | patch
Permalink /patch/673913/
State New
Headers show

Comments

Ulf Hansson - Dec. 6, 2018, 10:17 a.m.
It's quite questionable whether genpd internally should care about if the
corresponding PM domain for a device is powered on, as to allow setting a
new performance state for it. The assumptions creates an unnecessary
limitation at this point, for both consumers and providers, but more
importantly it also makes the code more complicated.

Therefore, let's simplify the code to allow setting a performance state, by
invoking the ->set_performance_state() callback, no matter whether the PM
domain is powered on or off.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)
Rafael J. Wysocki - Dec. 7, 2018, 11:58 a.m.
On Thursday, December 6, 2018 11:17:02 AM CET Ulf Hansson wrote:
> It's quite questionable whether genpd internally should care about if the
> corresponding PM domain for a device is powered on, as to allow setting a
> new performance state for it. The assumptions creates an unnecessary
> limitation at this point, for both consumers and providers, but more
> importantly it also makes the code more complicated.
> 
> Therefore, let's simplify the code to allow setting a performance state, by
> invoking the ->set_performance_state() callback, no matter whether the PM
> domain is powered on or off.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7f38a92b444a..4c39ea1b2cf6 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -311,12 +311,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>  	 */
>  
>  update_state:
> -	if (genpd_status_on(genpd)) {
> -		ret = genpd->set_performance_state(genpd, state);
> -		if (ret) {
> -			gpd_data->performance_state = prev;
> -			goto unlock;
> -		}
> +	ret = genpd->set_performance_state(genpd, state);
> +	if (ret) {
> +		gpd_data->performance_state = prev;
> +		goto unlock;
>  	}
>  
>  	genpd->performance_state = state;
> @@ -347,15 +345,6 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  		return ret;
>  
>  	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> -
> -	if (unlikely(genpd->set_performance_state)) {
> -		ret = genpd->set_performance_state(genpd, genpd->performance_state);
> -		if (ret) {
> -			pr_warn("%s: Failed to set performance state %d (%d)\n",
> -				genpd->name, genpd->performance_state, ret);
> -		}
> -	}
> -
>  	if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
>  		return ret;

I agree with the patch in general, but I'd like Viresh to comment.
Viresh Kumar - Dec. 10, 2018, 6:52 a.m.
On 06-12-18, 11:17, Ulf Hansson wrote:
> It's quite questionable whether genpd internally should care about if the
> corresponding PM domain for a device is powered on, as to allow setting a
> new performance state for it. The assumptions creates an unnecessary
> limitation at this point, for both consumers and providers, but more
> importantly it also makes the code more complicated.
> 
> Therefore, let's simplify the code to allow setting a performance state, by
> invoking the ->set_performance_state() callback, no matter whether the PM
> domain is powered on or off.

It may be better to write the side-effects of this in terms of power, if any, in
the commit log..

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7f38a92b444a..4c39ea1b2cf6 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -311,12 +311,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
>  	 */
>  
>  update_state:
> -	if (genpd_status_on(genpd)) {
> -		ret = genpd->set_performance_state(genpd, state);
> -		if (ret) {
> -			gpd_data->performance_state = prev;
> -			goto unlock;
> -		}
> +	ret = genpd->set_performance_state(genpd, state);
> +	if (ret) {
> +		gpd_data->performance_state = prev;
> +		goto unlock;
>  	}
>  
>  	genpd->performance_state = state;
> @@ -347,15 +345,6 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  		return ret;
>  
>  	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> -
> -	if (unlikely(genpd->set_performance_state)) {
> -		ret = genpd->set_performance_state(genpd, genpd->performance_state);
> -		if (ret) {
> -			pr_warn("%s: Failed to set performance state %d (%d)\n",
> -				genpd->name, genpd->performance_state, ret);
> -		}
> -	}
> -
>  	if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
>  		return ret;

I am trying to understand the side-effects of such a change. When do we expect
dev_pm_genpd_set_performance_state() to get called with the genpd powered off ?

There is one case I can think of. A simple driver which doesn't do DVFS sets its
performance state requirements from its probe() routine only once and then keeps
doing pm_runtime_get() and pm_runtime_put(). With this change the performance
state requirement of that device will always be there with the firmware, even
when the device is powered off.

If I remember correctly, you avoided calling set_performance_state(genpd, 0)
when the genpd gets powered-off as you thought that the firmware should take
care of it by itself as the genpd is getting powered off. If we still stick to
that, how will the firmware restore the performance state now at resume or power
ON of the genpd ?
Ulf Hansson - Dec. 10, 2018, 9:44 a.m.
On Mon, 10 Dec 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 06-12-18, 11:17, Ulf Hansson wrote:
> > It's quite questionable whether genpd internally should care about if the
> > corresponding PM domain for a device is powered on, as to allow setting a
> > new performance state for it. The assumptions creates an unnecessary
> > limitation at this point, for both consumers and providers, but more
> > importantly it also makes the code more complicated.
> >
> > Therefore, let's simplify the code to allow setting a performance state, by
> > invoking the ->set_performance_state() callback, no matter whether the PM
> > domain is powered on or off.
>
> It may be better to write the side-effects of this in terms of power, if any, in
> the commit log..

There should not be a difference in regards to power (energy).

However, perhaps I should clarify that a genpd provider may, due to
this change, now use the ->power_on() callback to also restore the
performance state?

>
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/base/power/domain.c | 19 ++++---------------
> >  1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 7f38a92b444a..4c39ea1b2cf6 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -311,12 +311,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> >        */
> >
> >  update_state:
> > -     if (genpd_status_on(genpd)) {
> > -             ret = genpd->set_performance_state(genpd, state);
> > -             if (ret) {
> > -                     gpd_data->performance_state = prev;
> > -                     goto unlock;
> > -             }
> > +     ret = genpd->set_performance_state(genpd, state);
> > +     if (ret) {
> > +             gpd_data->performance_state = prev;
> > +             goto unlock;
> >       }
> >
> >       genpd->performance_state = state;
> > @@ -347,15 +345,6 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> >               return ret;
> >
> >       elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> > -
> > -     if (unlikely(genpd->set_performance_state)) {
> > -             ret = genpd->set_performance_state(genpd, genpd->performance_state);
> > -             if (ret) {
> > -                     pr_warn("%s: Failed to set performance state %d (%d)\n",
> > -                             genpd->name, genpd->performance_state, ret);
> > -             }
> > -     }
> > -
> >       if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
> >               return ret;
>
> I am trying to understand the side-effects of such a change. When do we expect
> dev_pm_genpd_set_performance_state() to get called with the genpd powered off ?

Maybe for drivers that don't deploy runtime PM support (never calls
pm_runtime_enable()). Perhaps the corresponding devices are attached
to always on PM domains.

>
> There is one case I can think of. A simple driver which doesn't do DVFS sets its
> performance state requirements from its probe() routine only once and then keeps
> doing pm_runtime_get() and pm_runtime_put(). With this change the performance
> state requirement of that device will always be there with the firmware, even
> when the device is powered off.

Genpd don't reset the performance state to zero for the PM domain at
power off. Instead that's up the genpd provider to deal with, if
needed.

$subject patch doesn't change this.

>
> If I remember correctly, you avoided calling set_performance_state(genpd, 0)
> when the genpd gets powered-off as you thought that the firmware should take
> care of it by itself as the genpd is getting powered off. If we still stick to
> that, how will the firmware restore the performance state now at resume or power
> ON of the genpd ?

The genpd->performance_state reflects the current aggregated state of
the PM domain.

I expect the genpd provider to look at it from the ->power_on()
callback and restore the performance state of the PM domain at that
point.

Does it make sense?

Kind regards
Uffe
Ulf Hansson - Dec. 10, 2018, 9:52 a.m.
On Mon, 10 Dec 2018 at 10:44, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 10 Dec 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 06-12-18, 11:17, Ulf Hansson wrote:
> > > It's quite questionable whether genpd internally should care about if the
> > > corresponding PM domain for a device is powered on, as to allow setting a
> > > new performance state for it. The assumptions creates an unnecessary
> > > limitation at this point, for both consumers and providers, but more
> > > importantly it also makes the code more complicated.
> > >
> > > Therefore, let's simplify the code to allow setting a performance state, by
> > > invoking the ->set_performance_state() callback, no matter whether the PM
> > > domain is powered on or off.
> >
> > It may be better to write the side-effects of this in terms of power, if any, in
> > the commit log..
>
> There should not be a difference in regards to power (energy).
>
> However, perhaps I should clarify that a genpd provider may, due to
> this change, now use the ->power_on() callback to also restore the
> performance state?
>
> >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/base/power/domain.c | 19 ++++---------------
> > >  1 file changed, 4 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > > index 7f38a92b444a..4c39ea1b2cf6 100644
> > > --- a/drivers/base/power/domain.c
> > > +++ b/drivers/base/power/domain.c
> > > @@ -311,12 +311,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> > >        */
> > >
> > >  update_state:
> > > -     if (genpd_status_on(genpd)) {
> > > -             ret = genpd->set_performance_state(genpd, state);
> > > -             if (ret) {
> > > -                     gpd_data->performance_state = prev;
> > > -                     goto unlock;
> > > -             }
> > > +     ret = genpd->set_performance_state(genpd, state);
> > > +     if (ret) {
> > > +             gpd_data->performance_state = prev;
> > > +             goto unlock;
> > >       }
> > >
> > >       genpd->performance_state = state;
> > > @@ -347,15 +345,6 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> > >               return ret;
> > >
> > >       elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> > > -
> > > -     if (unlikely(genpd->set_performance_state)) {
> > > -             ret = genpd->set_performance_state(genpd, genpd->performance_state);
> > > -             if (ret) {
> > > -                     pr_warn("%s: Failed to set performance state %d (%d)\n",
> > > -                             genpd->name, genpd->performance_state, ret);
> > > -             }
> > > -     }
> > > -
> > >       if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
> > >               return ret;
> >
> > I am trying to understand the side-effects of such a change. When do we expect
> > dev_pm_genpd_set_performance_state() to get called with the genpd powered off ?
>
> Maybe for drivers that don't deploy runtime PM support (never calls
> pm_runtime_enable()). Perhaps the corresponding devices are attached
> to always on PM domains.

Huh. That was not answering you question, as it was not about powered
on but powered *off*.

In regards to powered off, I can't think of a case right now.

>
> >
> > There is one case I can think of. A simple driver which doesn't do DVFS sets its
> > performance state requirements from its probe() routine only once and then keeps
> > doing pm_runtime_get() and pm_runtime_put(). With this change the performance
> > state requirement of that device will always be there with the firmware, even
> > when the device is powered off.
>
> Genpd don't reset the performance state to zero for the PM domain at
> power off. Instead that's up the genpd provider to deal with, if
> needed.
>
> $subject patch doesn't change this.
>
> >
> > If I remember correctly, you avoided calling set_performance_state(genpd, 0)
> > when the genpd gets powered-off as you thought that the firmware should take
> > care of it by itself as the genpd is getting powered off. If we still stick to
> > that, how will the firmware restore the performance state now at resume or power
> > ON of the genpd ?
>
> The genpd->performance_state reflects the current aggregated state of
> the PM domain.
>
> I expect the genpd provider to look at it from the ->power_on()
> callback and restore the performance state of the PM domain at that
> point.
>
> Does it make sense?
>
> Kind regards
> Uffe
Viresh Kumar - Dec. 10, 2018, 10:12 a.m.
On 10-12-18, 10:44, Ulf Hansson wrote:
> On Mon, 10 Dec 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> There should not be a difference in regards to power (energy).
> 
> However, perhaps I should clarify that a genpd provider may, due to
> this change, now use the ->power_on() callback to also restore the
> performance state?

Yeah. And not just that, it will also need to check if the genpd is powered on or
not in its ->set_performance_state() callback as we may end up calling that on a
genpd which is powered off.
Rajendra Nayak - Dec. 10, 2018, 10:58 a.m.
On 12/10/2018 3:42 PM, Viresh Kumar wrote:
> On 10-12-18, 10:44, Ulf Hansson wrote:
>> On Mon, 10 Dec 2018 at 07:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
>> There should not be a difference in regards to power (energy).
>>
>> However, perhaps I should clarify that a genpd provider may, due to
>> this change, now use the ->power_on() callback to also restore the
>> performance state?
> 
> Yeah. And not just that, it will also need to check if the genpd is powered on or
> not in its ->set_performance_state() callback as we may end up calling that on a
> genpd which is powered off.

btw, we already do all of that as part of the rpm/rpmh genpd providers.

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7f38a92b444a..4c39ea1b2cf6 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -311,12 +311,10 @@  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 	 */
 
 update_state:
-	if (genpd_status_on(genpd)) {
-		ret = genpd->set_performance_state(genpd, state);
-		if (ret) {
-			gpd_data->performance_state = prev;
-			goto unlock;
-		}
+	ret = genpd->set_performance_state(genpd, state);
+	if (ret) {
+		gpd_data->performance_state = prev;
+		goto unlock;
 	}
 
 	genpd->performance_state = state;
@@ -347,15 +345,6 @@  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 		return ret;
 
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-
-	if (unlikely(genpd->set_performance_state)) {
-		ret = genpd->set_performance_state(genpd, genpd->performance_state);
-		if (ret) {
-			pr_warn("%s: Failed to set performance state %d (%d)\n",
-				genpd->name, genpd->performance_state, ret);
-		}
-	}
-
 	if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
 		return ret;