Re: [PATCH 2/3] PM: domains: Drop/restore performance state votes for devices at runtime PM

[Viresh Kumar]

On 03-06-21, 08:04, Viresh Kumar wrote:
> On 02-06-21, 12:12, 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@xxxxxxxxxx>
> > ---
> >  drivers/base/power/domain.c | 21 +++++++++++++++++++--
> >  include/linux/pm_domain.h   |  1 +
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index a3b6e751f366..81b9d4652b90 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -397,6 +397,18 @@ static int genpd_set_performance_state(struct device *dev, unsigned int state)
> >  	return ret;
> >  }
> >  
> > +static int genpd_drop_performance_state(struct device *dev)
> > +{
> > +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> > +	unsigned int prev_state;
> > +
> > +	prev_state = gpd_data->performance_state;
> > +	if (prev_state && !genpd_set_performance_state(dev, 0))
> 
> What about adding this prev_state check in
> genpd_set_performance_state() itself ? We already have one for the
> genpd in _genpd_set_performance_state(), why not one for the device ?
> 
> > +		return prev_state;
> > +
> > +	return 0;
> 
> Hmm, we will return 0 in case genpd_set_performance_state() fails,
> which will make us set the state to 0 again on resume. Maybe add a
> comment for this somewhere ?

No, we won't as you check for rpm_saved_pstate there, so the device
will stay disabled.

Again adding the check into genpd_set_performance_state() may help
reducing similar checks elsewhere.

> > +}
> > +
> >  /**
> >   * dev_pm_genpd_set_performance_state- Set performance state of device's power
> >   * domain.
> > @@ -839,7 +851,8 @@ static int genpd_runtime_suspend(struct device *dev)
> >  {
> >  	struct generic_pm_domain *genpd;
> >  	bool (*suspend_ok)(struct device *__dev);
> > -	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> > +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> > +	struct gpd_timing_data *td = &gpd_data->td;
> >  	bool runtime_pm = pm_runtime_enabled(dev);
> >  	ktime_t time_start;
> >  	s64 elapsed_ns;
> > @@ -896,6 +909,7 @@ static int genpd_runtime_suspend(struct device *dev)
> >  		return 0;
> >  
> >  	genpd_lock(genpd);
> > +	gpd_data->rpm_saved_pstate = genpd_drop_performance_state(dev);
> >  	genpd_power_off(genpd, true, 0);
> >  	genpd_unlock(genpd);
> >  
> > @@ -913,7 +927,8 @@ static int genpd_runtime_suspend(struct device *dev)
> >  static int genpd_runtime_resume(struct device *dev)
> >  {
> >  	struct generic_pm_domain *genpd;
> > -	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> > +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> > +	struct gpd_timing_data *td = &gpd_data->td;
> >  	bool runtime_pm = pm_runtime_enabled(dev);
> >  	ktime_t time_start;
> >  	s64 elapsed_ns;
> > @@ -937,6 +952,8 @@ static int genpd_runtime_resume(struct device *dev)
> >  
> >  	genpd_lock(genpd);
> >  	ret = genpd_power_on(genpd, 0);
> > +	if (!ret && gpd_data->rpm_saved_pstate)
> > +		genpd_set_performance_state(dev, gpd_data->rpm_saved_pstate);
> >  	genpd_unlock(genpd);

-- 
viresh