* [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits
@ 2020-01-16 23:12 ` Matthias Kaehlcke
2020-01-17 5:22 ` Chanwoo Choi
0 siblings, 1 reply; 6+ messages in thread
From: Matthias Kaehlcke @ 2020-01-16 23:12 UTC (permalink / raw)
To: MyungJoo Ham, Kyungmin Park, Chanwoo Choi, Zhang Rui,
Daniel Lezcano, Amit Kucheria
Cc: Leonard Crestez, linux-kernel, linux-pm, Matthias Kaehlcke,
Eduardo Valentin
Now that devfreq supports limiting the frequency range of a device
through PM QoS make use of it instead of disabling OPPs that should
not be used.
The switch from disabling OPPs to PM QoS introduces a subtle behavioral
change in case of conflicting requests (min > max): PM QoS gives
precedence to the MIN_FREQUENCY request, while higher OPPs disabled
with dev_pm_opp_disable() would override MIN_FREQUENCY.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- added documentation for 'req_max_freq'
- fixed jumps in of_devfreq_cooling_register_power() unwind
- added comment about behavioral change to the commit message
drivers/thermal/devfreq_cooling.c | 70 ++++++++++---------------------
1 file changed, 23 insertions(+), 47 deletions(-)
diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
index ef59256887ff63..cbbaf5bc425d1a 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -24,11 +24,13 @@
#include <linux/idr.h>
#include <linux/slab.h>
#include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
#include <linux/thermal.h>
#include <trace/events/thermal.h>
-#define SCALE_ERROR_MITIGATION 100
+#define HZ_PER_KHZ 1000
+#define SCALE_ERROR_MITIGATION 100
static DEFINE_IDA(devfreq_ida);
@@ -53,6 +55,8 @@ static DEFINE_IDA(devfreq_ida);
* 'utilization' (which is 'busy_time / 'total_time').
* The 'res_util' range is from 100 to (power_table[state] * 100)
* for the corresponding 'state'.
+ * @req_max_freq: PM QoS request for limiting the maximum frequency
+ * of the devfreq device.
*/
struct devfreq_cooling_device {
int id;
@@ -65,49 +69,9 @@ struct devfreq_cooling_device {
struct devfreq_cooling_power *power_ops;
u32 res_util;
int capped_state;
+ struct dev_pm_qos_request req_max_freq;
};
-/**
- * partition_enable_opps() - disable all opps above a given state
- * @dfc: Pointer to devfreq we are operating on
- * @cdev_state: cooling device state we're setting
- *
- * Go through the OPPs of the device, enabling all OPPs until
- * @cdev_state and disabling those frequencies above it.
- */
-static int partition_enable_opps(struct devfreq_cooling_device *dfc,
- unsigned long cdev_state)
-{
- int i;
- struct device *dev = dfc->devfreq->dev.parent;
-
- for (i = 0; i < dfc->freq_table_size; i++) {
- struct dev_pm_opp *opp;
- int ret = 0;
- unsigned int freq = dfc->freq_table[i];
- bool want_enable = i >= cdev_state ? true : false;
-
- opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
-
- if (PTR_ERR(opp) == -ERANGE)
- continue;
- else if (IS_ERR(opp))
- return PTR_ERR(opp);
-
- dev_pm_opp_put(opp);
-
- if (want_enable)
- ret = dev_pm_opp_enable(dev, freq);
- else
- ret = dev_pm_opp_disable(dev, freq);
-
- if (ret)
- return ret;
- }
-
- return 0;
-}
-
static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
unsigned long *state)
{
@@ -134,7 +98,7 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
struct devfreq_cooling_device *dfc = cdev->devdata;
struct devfreq *df = dfc->devfreq;
struct device *dev = df->dev.parent;
- int ret;
+ unsigned long freq;
if (state == dfc->cooling_state)
return 0;
@@ -144,9 +108,10 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
if (state >= dfc->freq_table_size)
return -EINVAL;
- ret = partition_enable_opps(dfc, state);
- if (ret)
- return ret;
+ freq = dfc->freq_table[state];
+
+ dev_pm_qos_update_request(&dfc->req_max_freq,
+ DIV_ROUND_UP(freq, HZ_PER_KHZ));
dfc->cooling_state = state;
@@ -529,9 +494,15 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
if (err)
goto free_dfc;
- err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
+ err = dev_pm_qos_add_request(df->dev.parent, &dfc->req_max_freq,
+ DEV_PM_QOS_MAX_FREQUENCY,
+ PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
if (err < 0)
goto free_tables;
+
+ err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
+ if (err < 0)
+ goto remove_qos_req;
dfc->id = err;
snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
@@ -552,6 +523,10 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
release_ida:
ida_simple_remove(&devfreq_ida, dfc->id);
+
+remove_qos_req:
+ dev_pm_qos_remove_request(&dfc->req_max_freq);
+
free_tables:
kfree(dfc->power_table);
kfree(dfc->freq_table);
@@ -600,6 +575,7 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
thermal_cooling_device_unregister(dfc->cdev);
ida_simple_remove(&devfreq_ida, dfc->id);
+ dev_pm_qos_remove_request(&dfc->req_max_freq);
kfree(dfc->power_table);
kfree(dfc->freq_table);
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits
2020-01-16 23:12 ` [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits Matthias Kaehlcke
@ 2020-01-17 5:22 ` Chanwoo Choi
2020-03-12 0:35 ` Matthias Kaehlcke
0 siblings, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2020-01-17 5:22 UTC (permalink / raw)
To: Matthias Kaehlcke, MyungJoo Ham, Kyungmin Park, Zhang Rui,
Daniel Lezcano, Amit Kucheria
Cc: Leonard Crestez, linux-kernel, linux-pm, Eduardo Valentin
On 1/17/20 8:12 AM, Matthias Kaehlcke wrote:
> Now that devfreq supports limiting the frequency range of a device
> through PM QoS make use of it instead of disabling OPPs that should
> not be used.
>
> The switch from disabling OPPs to PM QoS introduces a subtle behavioral
> change in case of conflicting requests (min > max): PM QoS gives
> precedence to the MIN_FREQUENCY request, while higher OPPs disabled
> with dev_pm_opp_disable() would override MIN_FREQUENCY.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>
> Changes in v2:
> - added documentation for 'req_max_freq'
> - fixed jumps in of_devfreq_cooling_register_power() unwind
> - added comment about behavioral change to the commit message
>
> drivers/thermal/devfreq_cooling.c | 70 ++++++++++---------------------
> 1 file changed, 23 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> index ef59256887ff63..cbbaf5bc425d1a 100644
> --- a/drivers/thermal/devfreq_cooling.c
> +++ b/drivers/thermal/devfreq_cooling.c
> @@ -24,11 +24,13 @@
> #include <linux/idr.h>
> #include <linux/slab.h>
> #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
> #include <linux/thermal.h>
>
> #include <trace/events/thermal.h>
>
> -#define SCALE_ERROR_MITIGATION 100
> +#define HZ_PER_KHZ 1000
> +#define SCALE_ERROR_MITIGATION 100
>
> static DEFINE_IDA(devfreq_ida);
>
> @@ -53,6 +55,8 @@ static DEFINE_IDA(devfreq_ida);
> * 'utilization' (which is 'busy_time / 'total_time').
> * The 'res_util' range is from 100 to (power_table[state] * 100)
> * for the corresponding 'state'.
> + * @req_max_freq: PM QoS request for limiting the maximum frequency
> + * of the devfreq device.
> */
> struct devfreq_cooling_device {
> int id;
> @@ -65,49 +69,9 @@ struct devfreq_cooling_device {
> struct devfreq_cooling_power *power_ops;
> u32 res_util;
> int capped_state;
> + struct dev_pm_qos_request req_max_freq;
> };
>
> -/**
> - * partition_enable_opps() - disable all opps above a given state
> - * @dfc: Pointer to devfreq we are operating on
> - * @cdev_state: cooling device state we're setting
> - *
> - * Go through the OPPs of the device, enabling all OPPs until
> - * @cdev_state and disabling those frequencies above it.
> - */
> -static int partition_enable_opps(struct devfreq_cooling_device *dfc,
> - unsigned long cdev_state)
> -{
> - int i;
> - struct device *dev = dfc->devfreq->dev.parent;
> -
> - for (i = 0; i < dfc->freq_table_size; i++) {
> - struct dev_pm_opp *opp;
> - int ret = 0;
> - unsigned int freq = dfc->freq_table[i];
> - bool want_enable = i >= cdev_state ? true : false;
> -
> - opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
> -
> - if (PTR_ERR(opp) == -ERANGE)
> - continue;
> - else if (IS_ERR(opp))
> - return PTR_ERR(opp);
> -
> - dev_pm_opp_put(opp);
> -
> - if (want_enable)
> - ret = dev_pm_opp_enable(dev, freq);
> - else
> - ret = dev_pm_opp_disable(dev, freq);
> -
> - if (ret)
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
> unsigned long *state)
> {
> @@ -134,7 +98,7 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> struct devfreq_cooling_device *dfc = cdev->devdata;
> struct devfreq *df = dfc->devfreq;
> struct device *dev = df->dev.parent;
> - int ret;
> + unsigned long freq;
>
> if (state == dfc->cooling_state)
> return 0;
> @@ -144,9 +108,10 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> if (state >= dfc->freq_table_size)
> return -EINVAL;
>
> - ret = partition_enable_opps(dfc, state);
> - if (ret)
> - return ret;
> + freq = dfc->freq_table[state];
> +
> + dev_pm_qos_update_request(&dfc->req_max_freq,
> + DIV_ROUND_UP(freq, HZ_PER_KHZ));
>
> dfc->cooling_state = state;
>
> @@ -529,9 +494,15 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> if (err)
> goto free_dfc;
>
> - err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> + err = dev_pm_qos_add_request(df->dev.parent, &dfc->req_max_freq,
> + DEV_PM_QOS_MAX_FREQUENCY,
> + PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> if (err < 0)
> goto free_tables;
> +
> + err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> + if (err < 0)
> + goto remove_qos_req;
> dfc->id = err;
>
> snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
> @@ -552,6 +523,10 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>
> release_ida:
> ida_simple_remove(&devfreq_ida, dfc->id);
> +
> +remove_qos_req:
> + dev_pm_qos_remove_request(&dfc->req_max_freq);
> +
> free_tables:
> kfree(dfc->power_table);
> kfree(dfc->freq_table);
> @@ -600,6 +575,7 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
>
> thermal_cooling_device_unregister(dfc->cdev);
> ida_simple_remove(&devfreq_ida, dfc->id);
> + dev_pm_qos_remove_request(&dfc->req_max_freq);
> kfree(dfc->power_table);
> kfree(dfc->freq_table);
>
>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits
2020-01-17 5:22 ` Chanwoo Choi
@ 2020-03-12 0:35 ` Matthias Kaehlcke
2020-03-12 11:39 ` Lukasz Luba
0 siblings, 1 reply; 6+ messages in thread
From: Matthias Kaehlcke @ 2020-03-12 0:35 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Zhang Rui,
Amit Kucheria, Leonard Crestez, linux-kernel, linux-pm,
Eduardo Valentin
Is any further action needed from my side or can this land?
Thanks
Matthias
On Fri, Jan 17, 2020 at 02:22:02PM +0900, Chanwoo Choi wrote:
> On 1/17/20 8:12 AM, Matthias Kaehlcke wrote:
> > Now that devfreq supports limiting the frequency range of a device
> > through PM QoS make use of it instead of disabling OPPs that should
> > not be used.
> >
> > The switch from disabling OPPs to PM QoS introduces a subtle behavioral
> > change in case of conflicting requests (min > max): PM QoS gives
> > precedence to the MIN_FREQUENCY request, while higher OPPs disabled
> > with dev_pm_opp_disable() would override MIN_FREQUENCY.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >
> > Changes in v2:
> > - added documentation for 'req_max_freq'
> > - fixed jumps in of_devfreq_cooling_register_power() unwind
> > - added comment about behavioral change to the commit message
> >
> > drivers/thermal/devfreq_cooling.c | 70 ++++++++++---------------------
> > 1 file changed, 23 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> > index ef59256887ff63..cbbaf5bc425d1a 100644
> > --- a/drivers/thermal/devfreq_cooling.c
> > +++ b/drivers/thermal/devfreq_cooling.c
> > @@ -24,11 +24,13 @@
> > #include <linux/idr.h>
> > #include <linux/slab.h>
> > #include <linux/pm_opp.h>
> > +#include <linux/pm_qos.h>
> > #include <linux/thermal.h>
> >
> > #include <trace/events/thermal.h>
> >
> > -#define SCALE_ERROR_MITIGATION 100
> > +#define HZ_PER_KHZ 1000
> > +#define SCALE_ERROR_MITIGATION 100
> >
> > static DEFINE_IDA(devfreq_ida);
> >
> > @@ -53,6 +55,8 @@ static DEFINE_IDA(devfreq_ida);
> > * 'utilization' (which is 'busy_time / 'total_time').
> > * The 'res_util' range is from 100 to (power_table[state] * 100)
> > * for the corresponding 'state'.
> > + * @req_max_freq: PM QoS request for limiting the maximum frequency
> > + * of the devfreq device.
> > */
> > struct devfreq_cooling_device {
> > int id;
> > @@ -65,49 +69,9 @@ struct devfreq_cooling_device {
> > struct devfreq_cooling_power *power_ops;
> > u32 res_util;
> > int capped_state;
> > + struct dev_pm_qos_request req_max_freq;
> > };
> >
> > -/**
> > - * partition_enable_opps() - disable all opps above a given state
> > - * @dfc: Pointer to devfreq we are operating on
> > - * @cdev_state: cooling device state we're setting
> > - *
> > - * Go through the OPPs of the device, enabling all OPPs until
> > - * @cdev_state and disabling those frequencies above it.
> > - */
> > -static int partition_enable_opps(struct devfreq_cooling_device *dfc,
> > - unsigned long cdev_state)
> > -{
> > - int i;
> > - struct device *dev = dfc->devfreq->dev.parent;
> > -
> > - for (i = 0; i < dfc->freq_table_size; i++) {
> > - struct dev_pm_opp *opp;
> > - int ret = 0;
> > - unsigned int freq = dfc->freq_table[i];
> > - bool want_enable = i >= cdev_state ? true : false;
> > -
> > - opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
> > -
> > - if (PTR_ERR(opp) == -ERANGE)
> > - continue;
> > - else if (IS_ERR(opp))
> > - return PTR_ERR(opp);
> > -
> > - dev_pm_opp_put(opp);
> > -
> > - if (want_enable)
> > - ret = dev_pm_opp_enable(dev, freq);
> > - else
> > - ret = dev_pm_opp_disable(dev, freq);
> > -
> > - if (ret)
> > - return ret;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
> > unsigned long *state)
> > {
> > @@ -134,7 +98,7 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> > struct devfreq_cooling_device *dfc = cdev->devdata;
> > struct devfreq *df = dfc->devfreq;
> > struct device *dev = df->dev.parent;
> > - int ret;
> > + unsigned long freq;
> >
> > if (state == dfc->cooling_state)
> > return 0;
> > @@ -144,9 +108,10 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> > if (state >= dfc->freq_table_size)
> > return -EINVAL;
> >
> > - ret = partition_enable_opps(dfc, state);
> > - if (ret)
> > - return ret;
> > + freq = dfc->freq_table[state];
> > +
> > + dev_pm_qos_update_request(&dfc->req_max_freq,
> > + DIV_ROUND_UP(freq, HZ_PER_KHZ));
> >
> > dfc->cooling_state = state;
> >
> > @@ -529,9 +494,15 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> > if (err)
> > goto free_dfc;
> >
> > - err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> > + err = dev_pm_qos_add_request(df->dev.parent, &dfc->req_max_freq,
> > + DEV_PM_QOS_MAX_FREQUENCY,
> > + PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> > if (err < 0)
> > goto free_tables;
> > +
> > + err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> > + if (err < 0)
> > + goto remove_qos_req;
> > dfc->id = err;
> >
> > snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
> > @@ -552,6 +523,10 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> >
> > release_ida:
> > ida_simple_remove(&devfreq_ida, dfc->id);
> > +
> > +remove_qos_req:
> > + dev_pm_qos_remove_request(&dfc->req_max_freq);
> > +
> > free_tables:
> > kfree(dfc->power_table);
> > kfree(dfc->freq_table);
> > @@ -600,6 +575,7 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
> >
> > thermal_cooling_device_unregister(dfc->cdev);
> > ida_simple_remove(&devfreq_ida, dfc->id);
> > + dev_pm_qos_remove_request(&dfc->req_max_freq);
> > kfree(dfc->power_table);
> > kfree(dfc->freq_table);
> >
> >
>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits
2020-03-12 0:35 ` Matthias Kaehlcke
@ 2020-03-12 11:39 ` Lukasz Luba
2020-03-12 17:57 ` Matthias Kaehlcke
0 siblings, 1 reply; 6+ messages in thread
From: Lukasz Luba @ 2020-03-12 11:39 UTC (permalink / raw)
To: Matthias Kaehlcke, Daniel Lezcano
Cc: Chanwoo Choi, MyungJoo Ham, Kyungmin Park, Zhang Rui,
Amit Kucheria, Leonard Crestez, linux-kernel, linux-pm,
Eduardo Valentin
Hi Matthias,
I just saw this email below the patch. I wasn't aware that you
are working on this. I will have to update my changes...
It looks good to me.
Unfortunately, it does not apply on top of Amit's commit
1b5cb9570670a6277cc0 thermal: devfreq_cooling: Appease the kernel-doc deity
Could you check this?
Other then that
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Regards,
Lukasz
On 3/12/20 12:35 AM, Matthias Kaehlcke wrote:
> Is any further action needed from my side or can this land?
>
> Thanks
>
> Matthias
>
> On Fri, Jan 17, 2020 at 02:22:02PM +0900, Chanwoo Choi wrote:
>> On 1/17/20 8:12 AM, Matthias Kaehlcke wrote:
>>> Now that devfreq supports limiting the frequency range of a device
>>> through PM QoS make use of it instead of disabling OPPs that should
>>> not be used.
>>>
>>> The switch from disabling OPPs to PM QoS introduces a subtle behavioral
>>> change in case of conflicting requests (min > max): PM QoS gives
>>> precedence to the MIN_FREQUENCY request, while higher OPPs disabled
>>> with dev_pm_opp_disable() would override MIN_FREQUENCY.
>>>
>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>> - added documentation for 'req_max_freq'
>>> - fixed jumps in of_devfreq_cooling_register_power() unwind
>>> - added comment about behavioral change to the commit message
>>>
>>> drivers/thermal/devfreq_cooling.c | 70 ++++++++++---------------------
>>> 1 file changed, 23 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
>>> index ef59256887ff63..cbbaf5bc425d1a 100644
>>> --- a/drivers/thermal/devfreq_cooling.c
>>> +++ b/drivers/thermal/devfreq_cooling.c
>>> @@ -24,11 +24,13 @@
>>> #include <linux/idr.h>
>>> #include <linux/slab.h>
>>> #include <linux/pm_opp.h>
>>> +#include <linux/pm_qos.h>
>>> #include <linux/thermal.h>
>>>
>>> #include <trace/events/thermal.h>
>>>
>>> -#define SCALE_ERROR_MITIGATION 100
>>> +#define HZ_PER_KHZ 1000
>>> +#define SCALE_ERROR_MITIGATION 100
>>>
>>> static DEFINE_IDA(devfreq_ida);
>>>
>>> @@ -53,6 +55,8 @@ static DEFINE_IDA(devfreq_ida);
>>> * 'utilization' (which is 'busy_time / 'total_time').
>>> * The 'res_util' range is from 100 to (power_table[state] * 100)
>>> * for the corresponding 'state'.
>>> + * @req_max_freq: PM QoS request for limiting the maximum frequency
>>> + * of the devfreq device.
>>> */
>>> struct devfreq_cooling_device {
>>> int id;
>>> @@ -65,49 +69,9 @@ struct devfreq_cooling_device {
>>> struct devfreq_cooling_power *power_ops;
>>> u32 res_util;
>>> int capped_state;
>>> + struct dev_pm_qos_request req_max_freq;
>>> };
>>>
>>> -/**
>>> - * partition_enable_opps() - disable all opps above a given state
>>> - * @dfc: Pointer to devfreq we are operating on
>>> - * @cdev_state: cooling device state we're setting
>>> - *
>>> - * Go through the OPPs of the device, enabling all OPPs until
>>> - * @cdev_state and disabling those frequencies above it.
>>> - */
>>> -static int partition_enable_opps(struct devfreq_cooling_device *dfc,
>>> - unsigned long cdev_state)
>>> -{
>>> - int i;
>>> - struct device *dev = dfc->devfreq->dev.parent;
>>> -
>>> - for (i = 0; i < dfc->freq_table_size; i++) {
>>> - struct dev_pm_opp *opp;
>>> - int ret = 0;
>>> - unsigned int freq = dfc->freq_table[i];
>>> - bool want_enable = i >= cdev_state ? true : false;
>>> -
>>> - opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
>>> -
>>> - if (PTR_ERR(opp) == -ERANGE)
>>> - continue;
>>> - else if (IS_ERR(opp))
>>> - return PTR_ERR(opp);
>>> -
>>> - dev_pm_opp_put(opp);
>>> -
>>> - if (want_enable)
>>> - ret = dev_pm_opp_enable(dev, freq);
>>> - else
>>> - ret = dev_pm_opp_disable(dev, freq);
>>> -
>>> - if (ret)
>>> - return ret;
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
>>> unsigned long *state)
>>> {
>>> @@ -134,7 +98,7 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
>>> struct devfreq_cooling_device *dfc = cdev->devdata;
>>> struct devfreq *df = dfc->devfreq;
>>> struct device *dev = df->dev.parent;
>>> - int ret;
>>> + unsigned long freq;
>>>
>>> if (state == dfc->cooling_state)
>>> return 0;
>>> @@ -144,9 +108,10 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
>>> if (state >= dfc->freq_table_size)
>>> return -EINVAL;
>>>
>>> - ret = partition_enable_opps(dfc, state);
>>> - if (ret)
>>> - return ret;
>>> + freq = dfc->freq_table[state];
>>> +
>>> + dev_pm_qos_update_request(&dfc->req_max_freq,
>>> + DIV_ROUND_UP(freq, HZ_PER_KHZ));
>>>
>>> dfc->cooling_state = state;
>>>
>>> @@ -529,9 +494,15 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>>> if (err)
>>> goto free_dfc;
>>>
>>> - err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
>>> + err = dev_pm_qos_add_request(df->dev.parent, &dfc->req_max_freq,
>>> + DEV_PM_QOS_MAX_FREQUENCY,
>>> + PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
>>> if (err < 0)
>>> goto free_tables;
>>> +
>>> + err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
>>> + if (err < 0)
>>> + goto remove_qos_req;
>>> dfc->id = err;
>>>
>>> snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
>>> @@ -552,6 +523,10 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
>>>
>>> release_ida:
>>> ida_simple_remove(&devfreq_ida, dfc->id);
>>> +
>>> +remove_qos_req:
>>> + dev_pm_qos_remove_request(&dfc->req_max_freq);
>>> +
>>> free_tables:
>>> kfree(dfc->power_table);
>>> kfree(dfc->freq_table);
>>> @@ -600,6 +575,7 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
>>>
>>> thermal_cooling_device_unregister(dfc->cdev);
>>> ida_simple_remove(&devfreq_ida, dfc->id);
>>> + dev_pm_qos_remove_request(&dfc->req_max_freq);
>>> kfree(dfc->power_table);
>>> kfree(dfc->freq_table);
>>>
>>>
>>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> --
>> Best Regards,
>> Chanwoo Choi
>> Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits
2020-03-12 11:39 ` Lukasz Luba
@ 2020-03-12 17:57 ` Matthias Kaehlcke
2020-03-12 20:26 ` Lukasz Luba
0 siblings, 1 reply; 6+ messages in thread
From: Matthias Kaehlcke @ 2020-03-12 17:57 UTC (permalink / raw)
To: Lukasz Luba
Cc: Daniel Lezcano, Chanwoo Choi, MyungJoo Ham, Kyungmin Park,
Zhang Rui, Amit Kucheria, Leonard Crestez, linux-kernel,
linux-pm, Eduardo Valentin
Hi Lukasz,
thanks for the review!
I'll rebase and send v3. Hopefully it doesn't cause too much extra
work for your changes.
Thanks
Matthias
On Thu, Mar 12, 2020 at 11:39:56AM +0000, Lukasz Luba wrote:
> Hi Matthias,
>
> I just saw this email below the patch. I wasn't aware that you
> are working on this. I will have to update my changes...
>
> It looks good to me.
> Unfortunately, it does not apply on top of Amit's commit
> 1b5cb9570670a6277cc0 thermal: devfreq_cooling: Appease the kernel-doc deity
>
> Could you check this?
>
> Other then that
>
> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
>
> Regards,
> Lukasz
>
> On 3/12/20 12:35 AM, Matthias Kaehlcke wrote:
> > Is any further action needed from my side or can this land?
> >
> > Thanks
> >
> > Matthias
> >
> > On Fri, Jan 17, 2020 at 02:22:02PM +0900, Chanwoo Choi wrote:
> > > On 1/17/20 8:12 AM, Matthias Kaehlcke wrote:
> > > > Now that devfreq supports limiting the frequency range of a device
> > > > through PM QoS make use of it instead of disabling OPPs that should
> > > > not be used.
> > > >
> > > > The switch from disabling OPPs to PM QoS introduces a subtle behavioral
> > > > change in case of conflicting requests (min > max): PM QoS gives
> > > > precedence to the MIN_FREQUENCY request, while higher OPPs disabled
> > > > with dev_pm_opp_disable() would override MIN_FREQUENCY.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - added documentation for 'req_max_freq'
> > > > - fixed jumps in of_devfreq_cooling_register_power() unwind
> > > > - added comment about behavioral change to the commit message
> > > >
> > > > drivers/thermal/devfreq_cooling.c | 70 ++++++++++---------------------
> > > > 1 file changed, 23 insertions(+), 47 deletions(-)
> > > >
> > > > diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
> > > > index ef59256887ff63..cbbaf5bc425d1a 100644
> > > > --- a/drivers/thermal/devfreq_cooling.c
> > > > +++ b/drivers/thermal/devfreq_cooling.c
> > > > @@ -24,11 +24,13 @@
> > > > #include <linux/idr.h>
> > > > #include <linux/slab.h>
> > > > #include <linux/pm_opp.h>
> > > > +#include <linux/pm_qos.h>
> > > > #include <linux/thermal.h>
> > > > #include <trace/events/thermal.h>
> > > > -#define SCALE_ERROR_MITIGATION 100
> > > > +#define HZ_PER_KHZ 1000
> > > > +#define SCALE_ERROR_MITIGATION 100
> > > > static DEFINE_IDA(devfreq_ida);
> > > > @@ -53,6 +55,8 @@ static DEFINE_IDA(devfreq_ida);
> > > > * 'utilization' (which is 'busy_time / 'total_time').
> > > > * The 'res_util' range is from 100 to (power_table[state] * 100)
> > > > * for the corresponding 'state'.
> > > > + * @req_max_freq: PM QoS request for limiting the maximum frequency
> > > > + * of the devfreq device.
> > > > */
> > > > struct devfreq_cooling_device {
> > > > int id;
> > > > @@ -65,49 +69,9 @@ struct devfreq_cooling_device {
> > > > struct devfreq_cooling_power *power_ops;
> > > > u32 res_util;
> > > > int capped_state;
> > > > + struct dev_pm_qos_request req_max_freq;
> > > > };
> > > > -/**
> > > > - * partition_enable_opps() - disable all opps above a given state
> > > > - * @dfc: Pointer to devfreq we are operating on
> > > > - * @cdev_state: cooling device state we're setting
> > > > - *
> > > > - * Go through the OPPs of the device, enabling all OPPs until
> > > > - * @cdev_state and disabling those frequencies above it.
> > > > - */
> > > > -static int partition_enable_opps(struct devfreq_cooling_device *dfc,
> > > > - unsigned long cdev_state)
> > > > -{
> > > > - int i;
> > > > - struct device *dev = dfc->devfreq->dev.parent;
> > > > -
> > > > - for (i = 0; i < dfc->freq_table_size; i++) {
> > > > - struct dev_pm_opp *opp;
> > > > - int ret = 0;
> > > > - unsigned int freq = dfc->freq_table[i];
> > > > - bool want_enable = i >= cdev_state ? true : false;
> > > > -
> > > > - opp = dev_pm_opp_find_freq_exact(dev, freq, !want_enable);
> > > > -
> > > > - if (PTR_ERR(opp) == -ERANGE)
> > > > - continue;
> > > > - else if (IS_ERR(opp))
> > > > - return PTR_ERR(opp);
> > > > -
> > > > - dev_pm_opp_put(opp);
> > > > -
> > > > - if (want_enable)
> > > > - ret = dev_pm_opp_enable(dev, freq);
> > > > - else
> > > > - ret = dev_pm_opp_disable(dev, freq);
> > > > -
> > > > - if (ret)
> > > > - return ret;
> > > > - }
> > > > -
> > > > - return 0;
> > > > -}
> > > > -
> > > > static int devfreq_cooling_get_max_state(struct thermal_cooling_device *cdev,
> > > > unsigned long *state)
> > > > {
> > > > @@ -134,7 +98,7 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> > > > struct devfreq_cooling_device *dfc = cdev->devdata;
> > > > struct devfreq *df = dfc->devfreq;
> > > > struct device *dev = df->dev.parent;
> > > > - int ret;
> > > > + unsigned long freq;
> > > > if (state == dfc->cooling_state)
> > > > return 0;
> > > > @@ -144,9 +108,10 @@ static int devfreq_cooling_set_cur_state(struct thermal_cooling_device *cdev,
> > > > if (state >= dfc->freq_table_size)
> > > > return -EINVAL;
> > > > - ret = partition_enable_opps(dfc, state);
> > > > - if (ret)
> > > > - return ret;
> > > > + freq = dfc->freq_table[state];
> > > > +
> > > > + dev_pm_qos_update_request(&dfc->req_max_freq,
> > > > + DIV_ROUND_UP(freq, HZ_PER_KHZ));
> > > > dfc->cooling_state = state;
> > > > @@ -529,9 +494,15 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> > > > if (err)
> > > > goto free_dfc;
> > > > - err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> > > > + err = dev_pm_qos_add_request(df->dev.parent, &dfc->req_max_freq,
> > > > + DEV_PM_QOS_MAX_FREQUENCY,
> > > > + PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> > > > if (err < 0)
> > > > goto free_tables;
> > > > +
> > > > + err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
> > > > + if (err < 0)
> > > > + goto remove_qos_req;
> > > > dfc->id = err;
> > > > snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
> > > > @@ -552,6 +523,10 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
> > > > release_ida:
> > > > ida_simple_remove(&devfreq_ida, dfc->id);
> > > > +
> > > > +remove_qos_req:
> > > > + dev_pm_qos_remove_request(&dfc->req_max_freq);
> > > > +
> > > > free_tables:
> > > > kfree(dfc->power_table);
> > > > kfree(dfc->freq_table);
> > > > @@ -600,6 +575,7 @@ void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
> > > > thermal_cooling_device_unregister(dfc->cdev);
> > > > ida_simple_remove(&devfreq_ida, dfc->id);
> > > > + dev_pm_qos_remove_request(&dfc->req_max_freq);
> > > > kfree(dfc->power_table);
> > > > kfree(dfc->freq_table);
> > > >
> > >
> > > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> > >
> > > --
> > > Best Regards,
> > > Chanwoo Choi
> > > Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits
2020-03-12 17:57 ` Matthias Kaehlcke
@ 2020-03-12 20:26 ` Lukasz Luba
0 siblings, 0 replies; 6+ messages in thread
From: Lukasz Luba @ 2020-03-12 20:26 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Daniel Lezcano, Chanwoo Choi, MyungJoo Ham, Kyungmin Park,
Zhang Rui, Amit Kucheria, Leonard Crestez, linux-kernel,
linux-pm, Eduardo Valentin
On 3/12/20 5:57 PM, Matthias Kaehlcke wrote:
> Hi Lukasz,
>
> thanks for the review!
>
> I'll rebase and send v3. Hopefully it doesn't cause too much extra
> work for your changes.
No worries, your change is really needed. It will simply the code
that I am working on (can be found here [1]). I don't have
deal with the race with devfreq governor now.
Thank you for working on it.
Lukasz
[1] https://lkml.org/lkml/2020/3/9/475
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-12 20:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200116231233epcas1p363ab7e3ad2966d0ae7bac11e33aa6b83@epcas1p3.samsung.com>
2020-01-16 23:12 ` [PATCH v2] thermal: devfreq_cooling: Use PM QoS to set frequency limits Matthias Kaehlcke
2020-01-17 5:22 ` Chanwoo Choi
2020-03-12 0:35 ` Matthias Kaehlcke
2020-03-12 11:39 ` Lukasz Luba
2020-03-12 17:57 ` Matthias Kaehlcke
2020-03-12 20:26 ` Lukasz Luba
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).