Messages in this thread | | | From | Ulf Hansson <> | Date | Fri, 4 Jun 2021 09:45:35 +0200 | Subject | Re: [PATCH v2 3/4] PM: domains: Drop/restore performance state votes for devices at runtime PM |
| |
On Fri, 4 Jun 2021 at 05:53, Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 03-06-21, 13:17, Ulf Hansson wrote: > > On Thu, 3 Jun 2021 at 12:31, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > +static int genpd_drop_performance_state(struct device *dev) > > > > > +{ > > > > > + 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. > > What stops the core to call genpd_drop_performance_state() in the > first place here, if the driver was doing its own thing ? If that gets > called, then restore should be without any checks IMO. The state > should already be 0 at this point of time, I don't know why this will > get called again with state 0, but it will have no effect. > > Can you give some sort of flow sequence where I can see the problem a > bit more clearly ?
Starting calls from the subsystem/driver:
------ dev_pm_genpd_set_performance_state(dev, 100); "run a use case with device runtime resumed" ... "use case ends" dev_pm_genpd_set_performance_state(dev, 0); pm_runtime_put() ->genpd_runtime_suspend() gpd_data->performance_state == 0, -> gpd_data->rpm_pstate = 0; ... "new use case start" dev_pm_genpd_set_performance_state(dev, 100); pm_runtime_get_sync() ->genpd_runtime_resume() gpd_data->performance_state == 100, -> gpd_data->rpm_pstate = 0; (This is where we need to check for "zero" to not override the value) ..... ------
I wouldn't say that the above is the way how I see the calls to dev_pm_genpd_set_performance_state (or actually dev_pm_opp_set_rate|opp()) being deployed. The calls should rather be done from the subsystem/driver's ->runtime_suspend|resume() callback, then the path above would work in the way you suggest.
Although, as we currently treat performance states and power states in genpd orthogonally, I wanted to make sure we could cope with both situations.
Did this help? :-)
Kind regards Uffe
| |