Messages in this thread | | | From | Ulf Hansson <> | Date | Thu, 3 Jun 2021 13:17:18 +0200 | Subject | Re: [PATCH v2 3/4] PM: domains: Drop/restore performance state votes for devices at runtime PM |
| |
On Thu, 3 Jun 2021 at 12:31, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 3 Jun 2021 at 11:55, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 03-06-21, 11:34, Ulf Hansson wrote: > > > A subsystem/driver that need to manage OPPs for its device, should > > > typically drop its vote for the OPP when the device becomes runtime > > > suspended. In this way, the corresponding aggregation of the performance > > > state votes that is managed in genpd for the attached PM domain, may find > > > that the aggregated vote can be decreased. Hence, it may allow genpd to set > > > the lower performance state for the PM domain, thus avoiding to waste > > > energy. > > > > > > To accomplish this, typically a subsystem/driver would need to call > > > dev_pm_opp_set_rate|opp() for its device from its ->runtime_suspend() > > > callback, to drop the vote for the OPP. Accordingly, it needs another call > > > to dev_pm_opp_set_rate|opp() to restore the vote for the OPP from its > > > ->runtime_resume() callback. > > > > > > To avoid boilerplate code in subsystems/driver to deal with these things, > > > let's instead manage this internally in genpd. > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > > --- > > > > > > Changes in v2: > > > - Rebased. > > > - A few minor cosmetic changes. > > > - Deal with the error path in genpd_runtime_resume(). > > > > > > --- > > > drivers/base/power/domain.c | 27 +++++++++++++++++++++++++-- > > > include/linux/pm_domain.h | 1 + > > > 2 files changed, 26 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > > > index ef25a5b18587..e5d97174c254 100644 > > > --- a/drivers/base/power/domain.c > > > +++ b/drivers/base/power/domain.c > > > @@ -400,6 +400,23 @@ static int genpd_set_performance_state(struct device *dev, unsigned int state) > > > return ret; > > > } > > > > > > +static int genpd_drop_performance_state(struct device *dev) > > > > What about passing the state pointer here? that will simplify the > > callers to just a call. > > Not sure I get that. Can you elaborate a bit more? > > > > > > +{ > > > + unsigned int prev_state = dev_gpd_data(dev)->performance_state; > > > + > > > + if (!genpd_set_performance_state(dev, 0)) > > > + return prev_state; > > > + > > > + return 0; > > > +} > > > + > > > +static void genpd_restore_performance_state(struct device *dev, > > > + unsigned int state) > > > +{ > > > + if (state) > > > > I will skip this check, as we are checking it in > > genpd_set_performance_state() anyway ? > > I don't want us to override OPP votes made by the subsystem/driver > level runtime PM callbacks. For example, if the drivers manage this > thing themselves, that should be preserved. > > That said, by the check above I want to avoid setting the state to > zero internally by genpd, if the driver level ->runtime_resume() > callback has already restored the state.
Ehh, forget about what I said about the ->runtime_resume() callback.
I am mostly trying to avoid restoring a state that is zero, just to be sure nobody else on some different level outside gendp, have decided to set a new OPP in-between our calls to genpd_drop|restore_performance state.
[...]
Kind regards Uffe
| |