| Message ID | 20201216175056.19554-1-ilina@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | PM / Domains: allow domain idle states to be disabled | expand |
On Wed, 16 Dec 2020 at 18:51, Lina Iyer <ilina@codeaurora.org> wrote: > > In order to debug critical domain and device power issues, it may be > necessary to disallow certain idle states at runtime. Let the device > disallow a domain idle state before suspending.The domain governor shall > check for the 'disabled' flag while determining the domain idle state. For debug purposes, you might as well just set a dev PM Qos latency request that corresponds to the state you want to disable. This will then prevent the genpd governor from selecting the state. Kind regards Uffe > > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > drivers/base/power/domain.c | 30 ++++++++++++++++++++++++++++ > drivers/base/power/domain_governor.c | 3 +++ > include/linux/pm_domain.h | 7 +++++++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 191539a8e06d..e4e7ab75bdea 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1926,6 +1926,36 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, > } > EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain); > > +/** > + * dev_pm_genpd_disable_idle_state - Disallow a PM domain's idle state > + * > + * @dev: device attached to the PM domain > + * @idx: index of the PM domain's idle state to be disabled > + * @disable: enable/disable idle state > + * > + * Allow a PM domain's idle state to be disabled. Disabled idle states will > + * be ignored by the domain governor when entering idle. Devices would > + * invoke this before calling runtime suspend. > + */ > +int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable) > +{ > + struct generic_pm_domain *genpd; > + > + genpd = dev_to_genpd_safe(dev); > + if (!genpd) > + return -ENODEV; > + > + if (idx >= genpd->state_count) > + return -EINVAL; > + > + genpd_lock(genpd); > + genpd->states[idx].disabled = disable; > + genpd_unlock(genpd); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(dev_pm_genpd_disable_idle_state); > + > static void genpd_free_default_power_state(struct genpd_power_state *states, > unsigned int state_count) > { > diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c > index 2afb7fa90d5d..8decd17c5a6a 100644 > --- a/drivers/base/power/domain_governor.c > +++ b/drivers/base/power/domain_governor.c > @@ -175,6 +175,9 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd, > s64 min_off_time_ns; > s64 off_on_time_ns; > > + if (genpd->states[state].disabled) > + return false; > + > off_on_time_ns = genpd->states[state].power_off_latency_ns + > genpd->states[state].power_on_latency_ns; > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index a41aea9d1c06..3d251b5b461a 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -101,6 +101,7 @@ struct genpd_power_state { > struct fwnode_handle *fwnode; > ktime_t idle_time; > void *data; > + bool disabled; > }; > > struct genpd_lock_ops; > @@ -233,6 +234,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb); > int dev_pm_genpd_remove_notifier(struct device *dev); > void genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable); > int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next); > +int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable); > > extern struct dev_power_governor simple_qos_governor; > extern struct dev_power_governor pm_domain_always_on_gov; > @@ -300,6 +302,11 @@ static inline int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) > return -EOPNOTSUPP; > } > > +static inline int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable) > +{ > + return -EOPNOTSUPP; > +} > + > #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) > #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) > #endif > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Tue, Dec 22 2020 at 03:16 -0700, Ulf Hansson wrote: >On Wed, 16 Dec 2020 at 18:51, Lina Iyer <ilina@codeaurora.org> wrote: >> >> In order to debug critical domain and device power issues, it may be >> necessary to disallow certain idle states at runtime. Let the device >> disallow a domain idle state before suspending.The domain governor shall >> check for the 'disabled' flag while determining the domain idle state. > >For debug purposes, you might as well just set a dev PM Qos latency >request that corresponds to the state you want to disable. This will >then prevent the genpd governor from selecting the state. > True, but it will also disable idle states deeper as well. Would like to avoid that. --Lina >Kind regards >Uffe > >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> drivers/base/power/domain.c | 30 ++++++++++++++++++++++++++++ >> drivers/base/power/domain_governor.c | 3 +++ >> include/linux/pm_domain.h | 7 +++++++ >> 3 files changed, 40 insertions(+) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index 191539a8e06d..e4e7ab75bdea 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -1926,6 +1926,36 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, >> } >> EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain); >> >> +/** >> + * dev_pm_genpd_disable_idle_state - Disallow a PM domain's idle state >> + * >> + * @dev: device attached to the PM domain >> + * @idx: index of the PM domain's idle state to be disabled >> + * @disable: enable/disable idle state >> + * >> + * Allow a PM domain's idle state to be disabled. Disabled idle states will >> + * be ignored by the domain governor when entering idle. Devices would >> + * invoke this before calling runtime suspend. >> + */ >> +int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable) >> +{ >> + struct generic_pm_domain *genpd; >> + >> + genpd = dev_to_genpd_safe(dev); >> + if (!genpd) >> + return -ENODEV; >> + >> + if (idx >= genpd->state_count) >> + return -EINVAL; >> + >> + genpd_lock(genpd); >> + genpd->states[idx].disabled = disable; >> + genpd_unlock(genpd); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(dev_pm_genpd_disable_idle_state); >> + >> static void genpd_free_default_power_state(struct genpd_power_state *states, >> unsigned int state_count) >> { >> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c >> index 2afb7fa90d5d..8decd17c5a6a 100644 >> --- a/drivers/base/power/domain_governor.c >> +++ b/drivers/base/power/domain_governor.c >> @@ -175,6 +175,9 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd, >> s64 min_off_time_ns; >> s64 off_on_time_ns; >> >> + if (genpd->states[state].disabled) >> + return false; >> + >> off_on_time_ns = genpd->states[state].power_off_latency_ns + >> genpd->states[state].power_on_latency_ns; >> >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index a41aea9d1c06..3d251b5b461a 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -101,6 +101,7 @@ struct genpd_power_state { >> struct fwnode_handle *fwnode; >> ktime_t idle_time; >> void *data; >> + bool disabled; >> }; >> >> struct genpd_lock_ops; >> @@ -233,6 +234,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb); >> int dev_pm_genpd_remove_notifier(struct device *dev); >> void genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable); >> int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next); >> +int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable); >> >> extern struct dev_power_governor simple_qos_governor; >> extern struct dev_power_governor pm_domain_always_on_gov; >> @@ -300,6 +302,11 @@ static inline int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) >> return -EOPNOTSUPP; >> } >> >> +static inline int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) >> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) >> #endif >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >>
On Fri, 25 Dec 2020 at 02:31, Lina Iyer <ilina@codeaurora.org> wrote: > > On Tue, Dec 22 2020 at 03:16 -0700, Ulf Hansson wrote: > >On Wed, 16 Dec 2020 at 18:51, Lina Iyer <ilina@codeaurora.org> wrote: > >> > >> In order to debug critical domain and device power issues, it may be > >> necessary to disallow certain idle states at runtime. Let the device > >> disallow a domain idle state before suspending.The domain governor shall > >> check for the 'disabled' flag while determining the domain idle state. > > > >For debug purposes, you might as well just set a dev PM Qos latency > >request that corresponds to the state you want to disable. This will > >then prevent the genpd governor from selecting the state. > > > True, but it will also disable idle states deeper as well. Would like to > avoid that. I see. In any case, I am not so excited about adding an exported genpd interface for this that drivers can call, for example. Then I would rather see a proper debugfs interface, where userspace can both see the available states and choose which one to disable/enable. Would that work? [...] Kind regards Uffe
On Sat, Dec 26 2020 at 05:33 -0700, Ulf Hansson wrote: >On Fri, 25 Dec 2020 at 02:31, Lina Iyer <ilina@codeaurora.org> wrote: >> >> On Tue, Dec 22 2020 at 03:16 -0700, Ulf Hansson wrote: >> >On Wed, 16 Dec 2020 at 18:51, Lina Iyer <ilina@codeaurora.org> wrote: >> >> >> >> In order to debug critical domain and device power issues, it may be >> >> necessary to disallow certain idle states at runtime. Let the device >> >> disallow a domain idle state before suspending.The domain governor shall >> >> check for the 'disabled' flag while determining the domain idle state. >> > >> >For debug purposes, you might as well just set a dev PM Qos latency >> >request that corresponds to the state you want to disable. This will >> >then prevent the genpd governor from selecting the state. >> > >> True, but it will also disable idle states deeper as well. Would like to >> avoid that. > >I see. In any case, I am not so excited about adding an exported genpd >interface for this that drivers can call, for example. > >Then I would rather see a proper debugfs interface, where userspace >can both see the available states and choose which one to >disable/enable. Would that work? > Sure, that would work. Any recommendations for existing debugfs node that we can add this to or something new? --Lina
On Mon, 28 Dec 2020 at 03:30, Lina Iyer <ilina@codeaurora.org> wrote: > > On Sat, Dec 26 2020 at 05:33 -0700, Ulf Hansson wrote: > >On Fri, 25 Dec 2020 at 02:31, Lina Iyer <ilina@codeaurora.org> wrote: > >> > >> On Tue, Dec 22 2020 at 03:16 -0700, Ulf Hansson wrote: > >> >On Wed, 16 Dec 2020 at 18:51, Lina Iyer <ilina@codeaurora.org> wrote: > >> >> > >> >> In order to debug critical domain and device power issues, it may be > >> >> necessary to disallow certain idle states at runtime. Let the device > >> >> disallow a domain idle state before suspending.The domain governor shall > >> >> check for the 'disabled' flag while determining the domain idle state. > >> > > >> >For debug purposes, you might as well just set a dev PM Qos latency > >> >request that corresponds to the state you want to disable. This will > >> >then prevent the genpd governor from selecting the state. > >> > > >> True, but it will also disable idle states deeper as well. Would like to > >> avoid that. > > > >I see. In any case, I am not so excited about adding an exported genpd > >interface for this that drivers can call, for example. > > > >Then I would rather see a proper debugfs interface, where userspace > >can both see the available states and choose which one to > >disable/enable. Would that work? > > > Sure, that would work. Any recommendations for existing debugfs node > that we can add this to or something new? I think there are two options: 1. A specific "enabled-states" node containing an array of values, one value for each available state. Like "on off on", if there are three states available, for example. 2. Convert into having a per idle state subnode/directory and then add a "enabled" node for each state. I guess the second option is more straightforward and easier for user space to deal with, but I have no strong opinions. Perhaps there is a third option as well? Kind regards Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 191539a8e06d..e4e7ab75bdea 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1926,6 +1926,36 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, } EXPORT_SYMBOL_GPL(pm_genpd_remove_subdomain); +/** + * dev_pm_genpd_disable_idle_state - Disallow a PM domain's idle state + * + * @dev: device attached to the PM domain + * @idx: index of the PM domain's idle state to be disabled + * @disable: enable/disable idle state + * + * Allow a PM domain's idle state to be disabled. Disabled idle states will + * be ignored by the domain governor when entering idle. Devices would + * invoke this before calling runtime suspend. + */ +int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable) +{ + struct generic_pm_domain *genpd; + + genpd = dev_to_genpd_safe(dev); + if (!genpd) + return -ENODEV; + + if (idx >= genpd->state_count) + return -EINVAL; + + genpd_lock(genpd); + genpd->states[idx].disabled = disable; + genpd_unlock(genpd); + + return 0; +} +EXPORT_SYMBOL_GPL(dev_pm_genpd_disable_idle_state); + static void genpd_free_default_power_state(struct genpd_power_state *states, unsigned int state_count) { diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c index 2afb7fa90d5d..8decd17c5a6a 100644 --- a/drivers/base/power/domain_governor.c +++ b/drivers/base/power/domain_governor.c @@ -175,6 +175,9 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd, s64 min_off_time_ns; s64 off_on_time_ns; + if (genpd->states[state].disabled) + return false; + off_on_time_ns = genpd->states[state].power_off_latency_ns + genpd->states[state].power_on_latency_ns; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index a41aea9d1c06..3d251b5b461a 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -101,6 +101,7 @@ struct genpd_power_state { struct fwnode_handle *fwnode; ktime_t idle_time; void *data; + bool disabled; }; struct genpd_lock_ops; @@ -233,6 +234,7 @@ int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb); int dev_pm_genpd_remove_notifier(struct device *dev); void genpd_enable_next_wakeup(struct generic_pm_domain *genpd, bool enable); int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next); +int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable); extern struct dev_power_governor simple_qos_governor; extern struct dev_power_governor pm_domain_always_on_gov; @@ -300,6 +302,11 @@ static inline int dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) return -EOPNOTSUPP; } +static inline int dev_pm_genpd_disable_idle_state(struct device *dev, unsigned int idx, bool disable) +{ + return -EOPNOTSUPP; +} + #define simple_qos_governor (*(struct dev_power_governor *)(NULL)) #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL)) #endif
In order to debug critical domain and device power issues, it may be necessary to disallow certain idle states at runtime. Let the device disallow a domain idle state before suspending.The domain governor shall check for the 'disabled' flag while determining the domain idle state. Signed-off-by: Lina Iyer <ilina@codeaurora.org> --- drivers/base/power/domain.c | 30 ++++++++++++++++++++++++++++ drivers/base/power/domain_governor.c | 3 +++ include/linux/pm_domain.h | 7 +++++++ 3 files changed, 40 insertions(+)