| Message ID | 20181206101702.26418-1-ulf.hansson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / Domains: Make genpd performance states orthogonal to the idlestates | expand |
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.
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 ? -- viresh
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
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
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. -- viresh
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.
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. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/base/power/domain.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) -- 2.17.1