[v2] PM / Domains: Make genpd performance states orthogonal to the idlestates
diff mbox series

Message ID 20181211100455.25905-1-ulf.hansson@linaro.org
State Not Applicable
Headers show
Series
  • [v2] PM / Domains: Make genpd performance states orthogonal to the idlestates
Related show

Commit Message

Ulf Hansson Dec. 11, 2018, 10:04 a.m. UTC
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.

Do note, this change means genpd providers needs to restore the performance
state themselves during power on, via the ->power_on() callback. Moreover,
they may also need to check that the PM domain is powered on, from their
->set_performance_state() callback, before deciding to update the state.

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

Changes in v2:
	- Clarified in the changelog, the new constraints this change put on
	genpd providers.

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

Comments

Rajendra Nayak Dec. 11, 2018, 10:11 a.m. UTC | #1
On 12/11/2018 3:34 PM, 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.
> 
> Do note, this change means genpd providers needs to restore the performance
> state themselves during power on, via the ->power_on() callback. Moreover,
> they may also need to check that the PM domain is powered on, from their
> ->set_performance_state() callback, before deciding to update the state.

I tested this with the rpm/rpmh genpd providers and things worked as
expected, given they already did what the changelog above expects, so

Tested-by: Rajendra Nayak <rnayak@codeaurora.org>

Btw, I tested the v1 of this patch, since v2 only has updates in changelog.

> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- Clarified in the changelog, the new constraints this change put on
> 	genpd providers.
> 
> ---
>   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;
>   
>
Viresh Kumar Dec. 11, 2018, 11:23 a.m. UTC | #2
On 11-12-18, 11:04, 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.
> 
> Do note, this change means genpd providers needs to restore the performance
> state themselves during power on, via the ->power_on() callback. Moreover,
> they may also need to check that the PM domain is powered on, from their
> ->set_performance_state() callback, before deciding to update the state.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- Clarified in the changelog, the new constraints this change put on
> 	genpd providers.
> 
> ---
>  drivers/base/power/domain.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

And I will rebase my patches on top of this now :)
Rafael J. Wysocki Dec. 11, 2018, 11:25 a.m. UTC | #3
On Tuesday, December 11, 2018 12:23:51 PM CET Viresh Kumar wrote:
> On 11-12-18, 11:04, 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.
> > 
> > Do note, this change means genpd providers needs to restore the performance
> > state themselves during power on, via the ->power_on() callback. Moreover,
> > they may also need to check that the PM domain is powered on, from their
> > ->set_performance_state() callback, before deciding to update the state.
> > 
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > 
> > Changes in v2:
> > 	- Clarified in the changelog, the new constraints this change put on
> > 	genpd providers.
> > 
> > ---
> >  drivers/base/power/domain.c | 19 ++++---------------
> >  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> And I will rebase my patches on top of this now :)

Please let me know if you need the pm-domains branch to be exposed.
Viresh Kumar Dec. 11, 2018, 11:32 a.m. UTC | #4
On 11-12-18, 12:25, Rafael J. Wysocki wrote:
> Please let me know if you need the pm-domains branch to be exposed.

Yeah, I would need that and it will be a bit messy this time. I would be
required to base my changes over the OPP pull request I sent yesterday (or the
specific relevant patches) and this commit.

Since some of the patches touching domain.c are going via the OPP tree anyway,
will it make more sense for me to rather include this patch in my series ?
Rafael J. Wysocki Dec. 11, 2018, 11:51 a.m. UTC | #5
On Tuesday, December 11, 2018 12:32:57 PM CET Viresh Kumar wrote:
> On 11-12-18, 12:25, Rafael J. Wysocki wrote:
> > Please let me know if you need the pm-domains branch to be exposed.
> 
> Yeah, I would need that and it will be a bit messy this time. I would be
> required to base my changes over the OPP pull request I sent yesterday (or the
> specific relevant patches) and this commit.
> 
> Since some of the patches touching domain.c are going via the OPP tree anyway,
> will it make more sense for me to rather include this patch in my series ?

Yes, you can do that too.
Viresh Kumar Dec. 12, 2018, 6:32 a.m. UTC | #6
On 11-12-18, 16:53, Viresh Kumar wrote:
> On 11-12-18, 11:04, 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.
> > 
> > Do note, this change means genpd providers needs to restore the performance
> > state themselves during power on, via the ->power_on() callback. Moreover,
> > they may also need to check that the PM domain is powered on, from their
> > ->set_performance_state() callback, before deciding to update the state.
> > 
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > 
> > Changes in v2:
> > 	- Clarified in the changelog, the new constraints this change put on
> > 	genpd providers.
> > 
> > ---
> >  drivers/base/power/domain.c | 19 ++++---------------
> >  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

I may need to take my Ack back actually :(

I tried to base my series on this stuff and found an interesting issue. The
issue happens with the propagation stuff.

1. While setting performance state of a sub-domain, we also need to propagate it
   to the master domains. What if the master domains are off ? Well, we will
   still call the set_performance_state() for the master domain and the provider
   driver should take care of it if the master is off. This works fine.

2. While re-evaluating performance state of a master, we now take into account
   its sub-domains as well apart from devices directly managed by master. What
   should we do if the sub-domain is powered-off ? We can't take its vote into
   account for sure as that will result in power being wasted. Then who will
   tell the master to take the vote into account when the sub-domain gets
   powered on ? That is where I am stuck now.
Ulf Hansson Dec. 12, 2018, 7:36 a.m. UTC | #7
On Wed, 12 Dec 2018 at 07:32, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 11-12-18, 16:53, Viresh Kumar wrote:
> > On 11-12-18, 11:04, 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.
> > >
> > > Do note, this change means genpd providers needs to restore the performance
> > > state themselves during power on, via the ->power_on() callback. Moreover,
> > > they may also need to check that the PM domain is powered on, from their
> > > ->set_performance_state() callback, before deciding to update the state.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > Changes in v2:
> > >     - Clarified in the changelog, the new constraints this change put on
> > >     genpd providers.
> > >
> > > ---
> > >  drivers/base/power/domain.c | 19 ++++---------------
> > >  1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> I may need to take my Ack back actually :(
>
> I tried to base my series on this stuff and found an interesting issue. The
> issue happens with the propagation stuff.
>
> 1. While setting performance state of a sub-domain, we also need to propagate it
>    to the master domains. What if the master domains are off ? Well, we will
>    still call the set_performance_state() for the master domain and the provider
>    driver should take care of it if the master is off. This works fine.
>
> 2. While re-evaluating performance state of a master, we now take into account
>    its sub-domains as well apart from devices directly managed by master. What
>    should we do if the sub-domain is powered-off ? We can't take its vote into
>    account for sure as that will result in power being wasted. Then who will
>    tell the master to take the vote into account when the sub-domain gets
>    powered on ? That is where I am stuck now.

This should not be a problem, unless I am missing something.

So, if there is an aggregated performance state != 0 for the
sub-domain, that's because there have been a performance state
requested for some of its attached devices. For the subdomain to be
powered off, all its attached devices must be runtime suspended, which
means some of its devices still have a performance state set for it,
while being runtime suspended. Then there must be a reason for it.

The second problem is already described in the changelog of $subject
patch. At power on, the PM domain may need to restore the performance
state from its ->power_on() callback.

Kind regards
Uffe
Viresh Kumar Dec. 12, 2018, 7:52 a.m. UTC | #8
On 12-12-18, 08:36, Ulf Hansson wrote:
> On Wed, 12 Dec 2018 at 07:32, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > 2. While re-evaluating performance state of a master, we now take into account
> >    its sub-domains as well apart from devices directly managed by master. What
> >    should we do if the sub-domain is powered-off ? We can't take its vote into
> >    account for sure as that will result in power being wasted. Then who will
> >    tell the master to take the vote into account when the sub-domain gets
> >    powered on ? That is where I am stuck now.
> 
> This should not be a problem, unless I am missing something.
> 
> So, if there is an aggregated performance state != 0 for the
> sub-domain, that's because there have been a performance state
> requested for some of its attached devices. For the subdomain to be
> powered off, all its attached devices must be runtime suspended, which
> means some of its devices still have a performance state set for it,
> while being runtime suspended. Then there must be a reason for it.

Right. I gave an example earlier of a device which doesn't do DVFS and so sets
performance state requirement only once from probe() and then keep
enabling/disabling via runtime PM APIs. So that is a valid case and we shouldn't
take its vote into account when the device is disabled.

> The second problem is already described in the changelog of $subject
> patch. At power on, the PM domain may need to restore the performance
> state from its ->power_on() callback.

Yeah, but the problem is who will propagate ? So we are powering ON a sub-domain
here and its power_on() callback will take care of setting its performance-state
and powering ON. But its masters need to be updated as well to take in account
the performance state requirement of the sub-domain, isn't it ?
Ulf Hansson Dec. 12, 2018, 9:47 a.m. UTC | #9
On Wed, 12 Dec 2018 at 08:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 12-12-18, 08:36, Ulf Hansson wrote:
> > On Wed, 12 Dec 2018 at 07:32, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > 2. While re-evaluating performance state of a master, we now take into account
> > >    its sub-domains as well apart from devices directly managed by master. What
> > >    should we do if the sub-domain is powered-off ? We can't take its vote into
> > >    account for sure as that will result in power being wasted. Then who will
> > >    tell the master to take the vote into account when the sub-domain gets
> > >    powered on ? That is where I am stuck now.
> >
> > This should not be a problem, unless I am missing something.
> >
> > So, if there is an aggregated performance state != 0 for the
> > sub-domain, that's because there have been a performance state
> > requested for some of its attached devices. For the subdomain to be
> > powered off, all its attached devices must be runtime suspended, which
> > means some of its devices still have a performance state set for it,
> > while being runtime suspended. Then there must be a reason for it.
>
> Right. I gave an example earlier of a device which doesn't do DVFS and so sets
> performance state requirement only once from probe() and then keep
> enabling/disabling via runtime PM APIs. So that is a valid case and we shouldn't
> take its vote into account when the device is disabled.

What to do in these cases, really deserves a separate discussion (and
code change). As I stated earlier, this boils done to deciding if
genpd should temporary reset the performance state of a device to
zero, at runtime suspend (thus it also needs to restore the
performance state at runtime resume).

As we currently do *not* reset the performance state at runtime
suspend for a device in genpd  - then I don't think there is any
reasons to why we should treat the PM domain power off/on sequence
differently.

>
> > The second problem is already described in the changelog of $subject
> > patch. At power on, the PM domain may need to restore the performance
> > state from its ->power_on() callback.
>
> Yeah, but the problem is who will propagate ? So we are powering ON a sub-domain
> here and its power_on() callback will take care of setting its performance-state
> and powering ON. But its masters need to be updated as well to take in account
> the performance state requirement of the sub-domain, isn't it ?

No.

The propagation should have been done already when the subdomain gets
powered on, as we should do that no matter whether the PM domains are
on or off.

Kind regards
Uffe

Patch
diff mbox series

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;