Re: [PATCH v2 4/4] PM: domains: Drop/restore performance state votes for devices at system PM

[Ulf Hansson]

+ Mark Brown, Dmitry Baryshkov

On Thu, 3 Jun 2021 at 11:34, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> Recent changes in genpd drops and restore performance state votes for
> devices during runtime PM.
>
> For the similar reasons, but to avoid the same kind of boilerplate code in
> device PM callbacks for system sleep in subsystems/drivers, let's drop and
> restore performance states votes in genpd for the attached devices during
> system sleep.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

After a second thought, it looks like we maybe should defer to apply
this final patch of the series. At least until we figured out how to
address the below issue:

So, I noticed that we have things like "regulator-fixed-domain", that
uses "required-opps" to enable/disable a regulator through the
dev_pm_set_performance_state() interface. We likely don't want to drop
the performance state internally in genpd when genpd_suspend_noirq()
gets called, for the corresponding struct device for the regulator.

I guess if genpd should drop performance states like $subject patch
suggest, we need some kind of additional coordination, that allows a
subsystem/driver to inform genpd when it should avoid it. Or something
along those lines.

Kind regards
Uffe

> ---
>
> Changes in v2:
>         - Rebased.
>         - A few cosmetic changes.
>
> ---
>  drivers/base/power/domain.c | 9 +++++++++
>  include/linux/pm_domain.h   | 1 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e5d97174c254..a33e5b341f3f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1171,6 +1171,7 @@ static int genpd_prepare(struct device *dev)
>   */
>  static int genpd_finish_suspend(struct device *dev, bool poweroff)
>  {
> +       struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
>         struct generic_pm_domain *genpd;
>         int ret = 0;
>
> @@ -1201,6 +1202,7 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff)
>         }
>
>         genpd_lock(genpd);
> +       gpd_data->pm_pstate = genpd_drop_performance_state(dev);
>         genpd->suspended_count++;
>         genpd_sync_power_off(genpd, true, 0);
>         genpd_unlock(genpd);
> @@ -1245,6 +1247,7 @@ static int genpd_resume_noirq(struct device *dev)
>         genpd_lock(genpd);
>         genpd_sync_power_on(genpd, true, 0);
>         genpd->suspended_count--;
> +       genpd_restore_performance_state(dev, dev_gpd_data(dev)->pm_pstate);
>         genpd_unlock(genpd);
>
>         if (genpd->dev_ops.stop && genpd->dev_ops.start &&
> @@ -1364,6 +1367,7 @@ static int genpd_restore_noirq(struct device *dev)
>         }
>
>         genpd_sync_power_on(genpd, true, 0);
> +       genpd_restore_performance_state(dev, dev_gpd_data(dev)->pm_pstate);
>         genpd_unlock(genpd);
>
>         if (genpd->dev_ops.stop && genpd->dev_ops.start &&
> @@ -1409,23 +1413,28 @@ static void genpd_complete(struct device *dev)
>  static void genpd_switch_state(struct device *dev, bool suspend)
>  {
>         struct generic_pm_domain *genpd;
> +       struct generic_pm_domain_data *gpd_data;
>         bool use_lock;
>
>         genpd = dev_to_genpd_safe(dev);
>         if (!genpd)
>                 return;
>
> +       gpd_data = dev_gpd_data(dev);
> +
>         use_lock = genpd_is_irq_safe(genpd);
>
>         if (use_lock)
>                 genpd_lock(genpd);
>
>         if (suspend) {
> +               gpd_data->pm_pstate = genpd_drop_performance_state(dev);
>                 genpd->suspended_count++;
>                 genpd_sync_power_off(genpd, use_lock, 0);
>         } else {
>                 genpd_sync_power_on(genpd, use_lock, 0);
>                 genpd->suspended_count--;
> +               genpd_restore_performance_state(dev, gpd_data->pm_pstate);
>         }
>
>         if (use_lock)
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 21a0577305ef..f6e9dc28621c 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -199,6 +199,7 @@ struct generic_pm_domain_data {
>         int cpu;
>         unsigned int performance_state;
>         unsigned int rpm_pstate;
> +       unsigned int pm_pstate;
>         ktime_t next_wakeup;
>         void *data;
>  };
> --
> 2.25.1
>