| Message ID | 20181211100455.25905-1-ulf.hansson@linaro.org |
|---|---|
| State | Accepted |
| Commit | 68de2fe57a8f2746db1064d39c697595cd76bb16 |
| Headers | show |
| Series | [v2] PM / Domains: Make genpd performance states orthogonal to the idlestates | expand |
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; > >
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 :) -- viresh
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.
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 ?
--
viresh
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.
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. -- viresh
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
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 ? -- viresh
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
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;
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(-) -- 2.17.1