* [PATCH] PM / Domains: allow domain idle states to be disabled
@ 2020-12-16 17:50 Lina Iyer
2020-12-22 10:15 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Lina Iyer @ 2020-12-16 17:50 UTC (permalink / raw)
To: ulf.hansson, rjw; +Cc: linux-pm, linux-arm-msm, Lina Iyer
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(+)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] PM / Domains: allow domain idle states to be disabled
2020-12-16 17:50 [PATCH] PM / Domains: allow domain idle states to be disabled Lina Iyer
@ 2020-12-22 10:15 ` Ulf Hansson
2020-12-25 1:31 ` Lina Iyer
0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2020-12-22 10:15 UTC (permalink / raw)
To: Lina Iyer; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] PM / Domains: allow domain idle states to be disabled
2020-12-22 10:15 ` Ulf Hansson
@ 2020-12-25 1:31 ` Lina Iyer
2020-12-26 12:33 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Lina Iyer @ 2020-12-25 1:31 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] PM / Domains: allow domain idle states to be disabled
2020-12-25 1:31 ` Lina Iyer
@ 2020-12-26 12:33 ` Ulf Hansson
2020-12-28 2:30 ` Lina Iyer
0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2020-12-26 12:33 UTC (permalink / raw)
To: Lina Iyer; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PM / Domains: allow domain idle states to be disabled
2020-12-26 12:33 ` Ulf Hansson
@ 2020-12-28 2:30 ` Lina Iyer
2021-01-08 10:22 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Lina Iyer @ 2020-12-28 2:30 UTC (permalink / raw)
To: Ulf Hansson; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PM / Domains: allow domain idle states to be disabled
2020-12-28 2:30 ` Lina Iyer
@ 2021-01-08 10:22 ` Ulf Hansson
0 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2021-01-08 10:22 UTC (permalink / raw)
To: Lina Iyer; +Cc: Rafael J. Wysocki, Linux PM, linux-arm-msm
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-08 10:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-16 17:50 [PATCH] PM / Domains: allow domain idle states to be disabled Lina Iyer
2020-12-22 10:15 ` Ulf Hansson
2020-12-25 1:31 ` Lina Iyer
2020-12-26 12:33 ` Ulf Hansson
2020-12-28 2:30 ` Lina Iyer
2021-01-08 10:22 ` Ulf Hansson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).