Set devfreq device min and max frequency limits when device is added to devfreq, provided frequency table is supplied. This helps governors to suggest target frequency with in limits.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org --- drivers/devfreq/devfreq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a8f0173..5782c9b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -69,6 +69,29 @@ static struct devfreq *find_device_devfreq(struct device *dev) }
/** + * devfreq_set_freq_limits() - Set min and max frequency from freq_table + * @devfreq: the devfreq instance + */ +static void devfreq_set_freq_limits(struct devfreq *devfreq) +{ + int idx; + unsigned long min = ~0, max = 0; + + if (!devfreq->profile->freq_table) + return; + + for (idx = 0; idx < devfreq->profile->max_state; idx++) { + if (min > devfreq->profile->freq_table[idx]) + min = devfreq->profile->freq_table[idx]; + if (max < devfreq->profile->freq_table[idx]) + max = devfreq->profile->freq_table[idx]; + } + + devfreq->min_freq = min; + devfreq->max_freq = max; +} + +/** * devfreq_get_freq_level() - Lookup freq_table for the frequency * @devfreq: the devfreq instance * @freq: the target frequency @@ -476,6 +499,7 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->profile->max_state, GFP_KERNEL); devfreq->last_stat_updated = jiffies; + devfreq_set_freq_limits(devfreq);
dev_set_name(&devfreq->dev, dev_name(dev)); err = device_register(&devfreq->dev);
Mark the stats start time stamp when actual load monitoring is started for accuracy.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org --- drivers/devfreq/devfreq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 5782c9b..2843a22 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -254,9 +254,12 @@ static void devfreq_monitor(struct work_struct *work) void devfreq_monitor_start(struct devfreq *devfreq) { INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); - if (devfreq->profile->polling_ms) + if (devfreq->profile->polling_ms) { queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); + + devfreq->last_stat_updated = jiffies; + } } EXPORT_SYMBOL(devfreq_monitor_start);
@@ -498,7 +501,6 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->time_in_state = devm_kzalloc(dev, sizeof(unsigned int) * devfreq->profile->max_state, GFP_KERNEL); - devfreq->last_stat_updated = jiffies; devfreq_set_freq_limits(devfreq);
dev_set_name(&devfreq->dev, dev_name(dev));
devfreq stats is not taking device suspend and resume into account. Fix it.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org --- drivers/devfreq/devfreq.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 2843a22..4c50235 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -297,6 +297,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) return; }
+ devfreq_update_status(devfreq, devfreq->previous_freq); devfreq->stop_polling = true; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work); @@ -313,6 +314,8 @@ EXPORT_SYMBOL(devfreq_monitor_suspend); */ void devfreq_monitor_resume(struct devfreq *devfreq) { + unsigned long freq; + mutex_lock(&devfreq->lock); if (!devfreq->stop_polling) goto out; @@ -321,8 +324,14 @@ void devfreq_monitor_resume(struct devfreq *devfreq) devfreq->profile->polling_ms) queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); + + devfreq->last_stat_updated = jiffies; devfreq->stop_polling = false;
+ if (devfreq->profile->get_cur_freq && + !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) + devfreq->previous_freq = freq; + out: mutex_unlock(&devfreq->lock); } @@ -931,11 +940,11 @@ static ssize_t show_trans_table(struct device *dev, struct device_attribute *att { struct devfreq *devfreq = to_devfreq(dev); ssize_t len; - int i, j, err; + int i, j; unsigned int max_state = devfreq->profile->max_state;
- err = devfreq_update_status(devfreq, devfreq->previous_freq); - if (err) + if (!devfreq->stop_polling && + devfreq_update_status(devfreq, devfreq->previous_freq)) return 0;
len = sprintf(buf, " From : To\n");
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
Set devfreq device min and max frequency limits when device is added to devfreq, provided frequency table is supplied. This helps governors to suggest target frequency with in limits.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
Could you please elaborate the benefit of the patch?
The devfreq device drivers are required to choose proper frequencies anyway regardless which values the governors may give (hopefully by choosing the closest value that can support the required performance).
Besides, the min/max values are to be set by userspace. Users may enter 0 in order to express that they do not want to limit the behaviors of governors.
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a8f0173..5782c9b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -69,6 +69,29 @@ static struct devfreq *find_device_devfreq(struct device *dev) }
/**
- devfreq_set_freq_limits() - Set min and max frequency from freq_table
- @devfreq: the devfreq instance
- */
+static void devfreq_set_freq_limits(struct devfreq *devfreq) +{
int idx;unsigned long min = ~0, max = 0;if (!devfreq->profile->freq_table)return;for (idx = 0; idx < devfreq->profile->max_state; idx++) {if (min > devfreq->profile->freq_table[idx])min = devfreq->profile->freq_table[idx];if (max < devfreq->profile->freq_table[idx])max = devfreq->profile->freq_table[idx];}devfreq->min_freq = min;devfreq->max_freq = max;+}
+/**
- devfreq_get_freq_level() - Lookup freq_table for the frequency
- @devfreq: the devfreq instance
- @freq: the target frequency
@@ -476,6 +499,7 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->profile->max_state, GFP_KERNEL); devfreq->last_stat_updated = jiffies;
devfreq_set_freq_limits(devfreq); dev_set_name(&devfreq->dev, dev_name(dev)); err = device_register(&devfreq->dev);-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
Mark the stats start time stamp when actual load monitoring is started for accuracy.
It appears that you are changing the semantics of the information here.
For example, in the case: - devfreq started at 0 w/ 100MHz - at 10, devfreq monitor starts w/ 100MHz - at 20, devfreq updates the freq to 200MHz - at 30, devfreq is requested to show the stat
The conventional: 20 @ 100MHz / 10 @ 200MHz
You proposed: 10 @ 100MHz / 10 @ 200MHz
Could you please give some reasons why the latter is "more accurate"? To me, the former seems appropriate.
Cheers, MyungJoo
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
drivers/devfreq/devfreq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 5782c9b..2843a22 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -254,9 +254,12 @@ static void devfreq_monitor(struct work_struct *work) void devfreq_monitor_start(struct devfreq *devfreq) { INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
if (devfreq->profile->polling_ms)
if (devfreq->profile->polling_ms) { queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms));devfreq->last_stat_updated = jiffies;}} EXPORT_SYMBOL(devfreq_monitor_start);
@@ -498,7 +501,6 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->time_in_state = devm_kzalloc(dev, sizeof(unsigned int) * devfreq->profile->max_state, GFP_KERNEL);
devfreq->last_stat_updated = jiffies; devfreq_set_freq_limits(devfreq); dev_set_name(&devfreq->dev, dev_name(dev));-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
devfreq stats is not taking device suspend and resume into account. Fix it.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
With monitor_suspend(), we are suspending the DVFS mechanism of a device, not the device itself.
Thus, the device may keep its frequency running and we may assume that the frequency is constant as the DVFS mechanism is in suspend.
Why do you want to stop the statistics as well?
Again, as in the other patch, this is about the semantics of the "devfreq statistics".
It does not seem to be a problem of which is correct and which is not, but it seems to be a problem of which is more convenient.
Could you please give me some cases where your semantics is more helpful?
I've been using the stat feature like this:
# cat stat; run benchmark; cat stat and look at the differences with any ops done during "run benchmark".
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 2843a22..4c50235 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -297,6 +297,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) return; }
devfreq_update_status(devfreq, devfreq->previous_freq); devfreq->stop_polling = true; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work);@@ -313,6 +314,8 @@ EXPORT_SYMBOL(devfreq_monitor_suspend); */ void devfreq_monitor_resume(struct devfreq *devfreq) {
unsigned long freq;mutex_lock(&devfreq->lock); if (!devfreq->stop_polling) goto out;@@ -321,8 +324,14 @@ void devfreq_monitor_resume(struct devfreq *devfreq) devfreq->profile->polling_ms) queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms));
devfreq->last_stat_updated = jiffies; devfreq->stop_polling = false;if (devfreq->profile->get_cur_freq &&!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))devfreq->previous_freq = freq;out: mutex_unlock(&devfreq->lock); } @@ -931,11 +940,11 @@ static ssize_t show_trans_table(struct device *dev, struct device_attribute *att { struct devfreq *devfreq = to_devfreq(dev); ssize_t len;
int i, j, err;
int i, j; unsigned int max_state = devfreq->profile->max_state;
err = devfreq_update_status(devfreq, devfreq->previous_freq);if (err)
if (!devfreq->stop_polling &&devfreq_update_status(devfreq, devfreq->previous_freq)) return 0; len = sprintf(buf, " From : To\n");-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 January 2013 20:06, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
Set devfreq device min and max frequency limits when device is added to devfreq, provided frequency table is supplied. This helps governors to suggest target frequency with in limits.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
Could you please elaborate the benefit of the patch?
When freq table is supplied, it's unreasonable to suggest the target frequency which is - target_freq < min_freq and target_freq > max_freq. It avoids unnecessary checks at devfreq drivers.
The devfreq device drivers are required to choose proper frequencies anyway regardless which values the governors may give (hopefully by choosing the closest value that can support the required performance).
Yes. but then each driver needs to have conditional checks for choosing closet value even though freq table is provided.
Besides, the min/max values are to be set by userspace. Users may enter 0 in order to express that they do not want to limit the behaviors of governors.
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a8f0173..5782c9b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -69,6 +69,29 @@ static struct devfreq *find_device_devfreq(struct device *dev) }
/**
- devfreq_set_freq_limits() - Set min and max frequency from freq_table
- @devfreq: the devfreq instance
- */
+static void devfreq_set_freq_limits(struct devfreq *devfreq) +{
int idx;unsigned long min = ~0, max = 0;if (!devfreq->profile->freq_table)return;for (idx = 0; idx < devfreq->profile->max_state; idx++) {if (min > devfreq->profile->freq_table[idx])min = devfreq->profile->freq_table[idx];if (max < devfreq->profile->freq_table[idx])max = devfreq->profile->freq_table[idx];}devfreq->min_freq = min;devfreq->max_freq = max;+}
+/**
- devfreq_get_freq_level() - Lookup freq_table for the frequency
- @devfreq: the devfreq instance
- @freq: the target frequency
@@ -476,6 +499,7 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->profile->max_state, GFP_KERNEL); devfreq->last_stat_updated = jiffies;
devfreq_set_freq_limits(devfreq); dev_set_name(&devfreq->dev, dev_name(dev)); err = device_register(&devfreq->dev);-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
On 14 January 2013 20:12, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
Mark the stats start time stamp when actual load monitoring is started for accuracy.
It appears that you are changing the semantics of the information here.
For example, in the case:
- devfreq started at 0 w/ 100MHz
- at 10, devfreq monitor starts w/ 100MHz
- at 20, devfreq updates the freq to 200MHz
- at 30, devfreq is requested to show the stat
The conventional: 20 @ 100MHz / 10 @ 200MHz
You proposed: 10 @ 100MHz / 10 @ 200MHz
Could you please give some reasons why the latter is "more accurate"? To me, the former seems appropriate.
Yes. I agree. Please ignore this change.
Cheers, MyungJoo
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
drivers/devfreq/devfreq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 5782c9b..2843a22 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -254,9 +254,12 @@ static void devfreq_monitor(struct work_struct *work) void devfreq_monitor_start(struct devfreq *devfreq) { INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
if (devfreq->profile->polling_ms)
if (devfreq->profile->polling_ms) { queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms));devfreq->last_stat_updated = jiffies;}} EXPORT_SYMBOL(devfreq_monitor_start);
@@ -498,7 +501,6 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->time_in_state = devm_kzalloc(dev, sizeof(unsigned int) * devfreq->profile->max_state, GFP_KERNEL);
devfreq->last_stat_updated = jiffies; devfreq_set_freq_limits(devfreq); dev_set_name(&devfreq->dev, dev_name(dev));-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
On 14 January 2013 20:18, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
devfreq stats is not taking device suspend and resume into account. Fix it.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
With monitor_suspend(), we are suspending the DVFS mechanism of a device, not the device itself.
Thus, the device may keep its frequency running and we may assume that the frequency is constant as the DVFS mechanism is in suspend.
Why do you want to stop the statistics as well?
There seems to be misunderstanding of devfreq_monitor_suspend() and devfreq_monitor_resume() apis usage.
Typically devfreq_monitor_suspend() would be called to suspend the device devfreq when device is entering idle state by powering off(clocks and voltage). When device itself is off, it's incorrect to allow stats to continue.
Again, as in the other patch, this is about the semantics of the "devfreq statistics".
It does not seem to be a problem of which is correct and which is not, but it seems to be a problem of which is more convenient.
Could you please give me some cases where your semantics is more helpful?
When gpu is powered off, gpu devfreq should be suspended and hence the stats.
I've been using the stat feature like this:
# cat stat; run benchmark; cat stat and look at the differences with any ops done during "run benchmark".
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 2843a22..4c50235 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -297,6 +297,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) return; }
devfreq_update_status(devfreq, devfreq->previous_freq); devfreq->stop_polling = true; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work);@@ -313,6 +314,8 @@ EXPORT_SYMBOL(devfreq_monitor_suspend); */ void devfreq_monitor_resume(struct devfreq *devfreq) {
unsigned long freq;mutex_lock(&devfreq->lock); if (!devfreq->stop_polling) goto out;@@ -321,8 +324,14 @@ void devfreq_monitor_resume(struct devfreq *devfreq) devfreq->profile->polling_ms) queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms));
devfreq->last_stat_updated = jiffies; devfreq->stop_polling = false;if (devfreq->profile->get_cur_freq &&!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))devfreq->previous_freq = freq;out: mutex_unlock(&devfreq->lock); } @@ -931,11 +940,11 @@ static ssize_t show_trans_table(struct device *dev, struct device_attribute *att { struct devfreq *devfreq = to_devfreq(dev); ssize_t len;
int i, j, err;
int i, j; unsigned int max_state = devfreq->profile->max_state;
err = devfreq_update_status(devfreq, devfreq->previous_freq);if (err)
if (!devfreq->stop_polling &&devfreq_update_status(devfreq, devfreq->previous_freq)) return 0; len = sprintf(buf, " From : To\n");-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
MyungJoo, Ping.
On 15 January 2013 16:51, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
On 14 January 2013 20:06, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
Set devfreq device min and max frequency limits when device is added to devfreq, provided frequency table is supplied. This helps governors to suggest target frequency with in limits.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
Could you please elaborate the benefit of the patch?
When freq table is supplied, it's unreasonable to suggest the target frequency which is - target_freq < min_freq and target_freq > max_freq. It avoids unnecessary checks at devfreq drivers.
The devfreq device drivers are required to choose proper frequencies anyway regardless which values the governors may give (hopefully by choosing the closest value that can support the required performance).
Yes. but then each driver needs to have conditional checks for choosing closet value even though freq table is provided.
Besides, the min/max values are to be set by userspace. Users may enter 0 in order to express that they do not want to limit the behaviors of governors.
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a8f0173..5782c9b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -69,6 +69,29 @@ static struct devfreq *find_device_devfreq(struct device *dev) }
/**
- devfreq_set_freq_limits() - Set min and max frequency from freq_table
- @devfreq: the devfreq instance
- */
+static void devfreq_set_freq_limits(struct devfreq *devfreq) +{
int idx;unsigned long min = ~0, max = 0;if (!devfreq->profile->freq_table)return;for (idx = 0; idx < devfreq->profile->max_state; idx++) {if (min > devfreq->profile->freq_table[idx])min = devfreq->profile->freq_table[idx];if (max < devfreq->profile->freq_table[idx])max = devfreq->profile->freq_table[idx];}devfreq->min_freq = min;devfreq->max_freq = max;+}
+/**
- devfreq_get_freq_level() - Lookup freq_table for the frequency
- @devfreq: the devfreq instance
- @freq: the target frequency
@@ -476,6 +499,7 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->profile->max_state, GFP_KERNEL); devfreq->last_stat_updated = jiffies;
devfreq_set_freq_limits(devfreq); dev_set_name(&devfreq->dev, dev_name(dev)); err = device_register(&devfreq->dev);-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
-- Regards, Rajagopal
MyungJoo, Ping.
On 15 January 2013 17:16, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
On 14 January 2013 20:18, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
devfreq stats is not taking device suspend and resume into account. Fix it.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
With monitor_suspend(), we are suspending the DVFS mechanism of a device, not the device itself.
Thus, the device may keep its frequency running and we may assume that the frequency is constant as the DVFS mechanism is in suspend.
Why do you want to stop the statistics as well?
There seems to be misunderstanding of devfreq_monitor_suspend() and devfreq_monitor_resume() apis usage.
Typically devfreq_monitor_suspend() would be called to suspend the device devfreq when device is entering idle state by powering off(clocks and voltage). When device itself is off, it's incorrect to allow stats to continue.
Again, as in the other patch, this is about the semantics of the "devfreq statistics".
It does not seem to be a problem of which is correct and which is not, but it seems to be a problem of which is more convenient.
Could you please give me some cases where your semantics is more helpful?
When gpu is powered off, gpu devfreq should be suspended and hence the stats.
I've been using the stat feature like this:
# cat stat; run benchmark; cat stat and look at the differences with any ops done during "run benchmark".
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 2843a22..4c50235 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -297,6 +297,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) return; }
devfreq_update_status(devfreq, devfreq->previous_freq); devfreq->stop_polling = true; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work);@@ -313,6 +314,8 @@ EXPORT_SYMBOL(devfreq_monitor_suspend); */ void devfreq_monitor_resume(struct devfreq *devfreq) {
unsigned long freq;mutex_lock(&devfreq->lock); if (!devfreq->stop_polling) goto out;@@ -321,8 +324,14 @@ void devfreq_monitor_resume(struct devfreq *devfreq) devfreq->profile->polling_ms) queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms));
devfreq->last_stat_updated = jiffies; devfreq->stop_polling = false;if (devfreq->profile->get_cur_freq &&!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))devfreq->previous_freq = freq;out: mutex_unlock(&devfreq->lock); } @@ -931,11 +940,11 @@ static ssize_t show_trans_table(struct device *dev, struct device_attribute *att { struct devfreq *devfreq = to_devfreq(dev); ssize_t len;
int i, j, err;
int i, j; unsigned int max_state = devfreq->profile->max_state;
err = devfreq_update_status(devfreq, devfreq->previous_freq);if (err)
if (!devfreq->stop_polling &&devfreq_update_status(devfreq, devfreq->previous_freq)) return 0; len = sprintf(buf, " From : To\n");-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
-- Regards, Rajagopal
On Tue, Jan 15, 2013 at 8:46 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
On 14 January 2013 20:18, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
devfreq stats is not taking device suspend and resume into account. Fix it.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
With monitor_suspend(), we are suspending the DVFS mechanism of a device, not the device itself.
Thus, the device may keep its frequency running and we may assume that the frequency is constant as the DVFS mechanism is in suspend.
Why do you want to stop the statistics as well?
There seems to be misunderstanding of devfreq_monitor_suspend() and devfreq_monitor_resume() apis usage.
Typically devfreq_monitor_suspend() would be called to suspend the device devfreq when device is entering idle state by powering off(clocks and voltage). When device itself is off, it's incorrect to allow stats to continue.
Ah.. yes... you're right. It is meant to be called by suspend of pm-sleep or pm-suspend. This will be applied soon.
Acked-by: MyungJoo Ham myungjoo.ham@samsung.com
Again, as in the other patch, this is about the semantics of the "devfreq statistics".
It does not seem to be a problem of which is correct and which is not, but it seems to be a problem of which is more convenient.
Could you please give me some cases where your semantics is more helpful?
When gpu is powered off, gpu devfreq should be suspended and hence the stats.
I've been using the stat feature like this:
# cat stat; run benchmark; cat stat and look at the differences with any ops done during "run benchmark".
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 2843a22..4c50235 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -297,6 +297,7 @@ void devfreq_monitor_suspend(struct devfreq *devfreq) return; }
devfreq_update_status(devfreq, devfreq->previous_freq); devfreq->stop_polling = true; mutex_unlock(&devfreq->lock); cancel_delayed_work_sync(&devfreq->work);@@ -313,6 +314,8 @@ EXPORT_SYMBOL(devfreq_monitor_suspend); */ void devfreq_monitor_resume(struct devfreq *devfreq) {
unsigned long freq;mutex_lock(&devfreq->lock); if (!devfreq->stop_polling) goto out;@@ -321,8 +324,14 @@ void devfreq_monitor_resume(struct devfreq *devfreq) devfreq->profile->polling_ms) queue_delayed_work(devfreq_wq, &devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms));
devfreq->last_stat_updated = jiffies; devfreq->stop_polling = false;if (devfreq->profile->get_cur_freq &&!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))devfreq->previous_freq = freq;out: mutex_unlock(&devfreq->lock); } @@ -931,11 +940,11 @@ static ssize_t show_trans_table(struct device *dev, struct device_attribute *att { struct devfreq *devfreq = to_devfreq(dev); ssize_t len;
int i, j, err;
int i, j; unsigned int max_state = devfreq->profile->max_state;
err = devfreq_update_status(devfreq, devfreq->previous_freq);if (err)
if (!devfreq->stop_polling &&devfreq_update_status(devfreq, devfreq->previous_freq)) return 0; len = sprintf(buf, " From : To\n");-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
-- Regards, Rajagopal
On Tue, Jan 15, 2013 at 8:21 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
On 14 January 2013 20:06, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
Set devfreq device min and max frequency limits when device is added to devfreq, provided frequency table is supplied. This helps governors to suggest target frequency with in limits.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
Could you please elaborate the benefit of the patch?
When freq table is supplied, it's unreasonable to suggest the target frequency which is - target_freq < min_freq and target_freq > max_freq. It avoids unnecessary checks at devfreq drivers.
Is it a safety barrier against userspace processes or devfreq governors? Users are allowed to enter min_freq and max_freq via sysfs interfaces anyway. Also, as mentioned earlier, users are allowed to enter 0 in order to "disengage" the min/max_freq. If we need "hardware-specific" min/max value, we'd need another sysfs interfaces and values (i.e., scaling_min/max_freq vs cpuinfo_min/max_freq).
Besides, if you are able to provide "freq_table", it means that you are able to provide OPP for the device. Then, you can use OPP APIs to avoid unnecessary checks at devfreq driver (even you can use the devfreq helper functions)
The devfreq device drivers are required to choose proper frequencies anyway regardless which values the governors may give (hopefully by choosing the closest value that can support the required performance).
Yes. but then each driver needs to have conditional checks for choosing closet value even though freq table is provided.
Even with this min/max-freq, when governors suggests some values between min/max, each driver needs to have checks unless it uses OPP.
Besides, the min/max values are to be set by userspace. Users may enter 0 in order to express that they do not want to limit the behaviors of governors.
Because of this, this patch will create discrepency from/to userspace interfaces.
Cheers, MyungJoo.
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a8f0173..5782c9b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -69,6 +69,29 @@ static struct devfreq *find_device_devfreq(struct device *dev) }
/**
- devfreq_set_freq_limits() - Set min and max frequency from freq_table
- @devfreq: the devfreq instance
- */
+static void devfreq_set_freq_limits(struct devfreq *devfreq) +{
int idx;unsigned long min = ~0, max = 0;if (!devfreq->profile->freq_table)return;for (idx = 0; idx < devfreq->profile->max_state; idx++) {if (min > devfreq->profile->freq_table[idx])min = devfreq->profile->freq_table[idx];if (max < devfreq->profile->freq_table[idx])max = devfreq->profile->freq_table[idx];}devfreq->min_freq = min;devfreq->max_freq = max;+}
+/**
- devfreq_get_freq_level() - Lookup freq_table for the frequency
- @devfreq: the devfreq instance
- @freq: the target frequency
@@ -476,6 +499,7 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->profile->max_state, GFP_KERNEL); devfreq->last_stat_updated = jiffies;
devfreq_set_freq_limits(devfreq); dev_set_name(&devfreq->dev, dev_name(dev)); err = device_register(&devfreq->dev);-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
-- Regards, Rajagopal
On 5 February 2013 12:21, MyungJoo Ham myungjoo.ham@gmail.com wrote:
On Tue, Jan 15, 2013 at 8:21 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
On 14 January 2013 20:06, MyungJoo Ham myungjoo.ham@samsung.com wrote:
On Tue, Jan 8, 2013 at 2:50 PM, Rajagopal Venkat rajagopal.venkat@linaro.org wrote:
Set devfreq device min and max frequency limits when device is added to devfreq, provided frequency table is supplied. This helps governors to suggest target frequency with in limits.
Signed-off-by: Rajagopal Venkat rajagopal.venkat@linaro.org
Could you please elaborate the benefit of the patch?
When freq table is supplied, it's unreasonable to suggest the target frequency which is - target_freq < min_freq and target_freq > max_freq. It avoids unnecessary checks at devfreq drivers.
Is it a safety barrier against userspace processes or devfreq governors? Users are allowed to enter min_freq and max_freq via sysfs interfaces anyway. Also, as mentioned earlier, users are allowed to enter 0 in order to "disengage" the min/max_freq.
It doesn't interfere with userspace updates to min_freq/max_freq. The min/max limits from freq table are assigned only during the device devfreq init, which can be overwritten by userspace.
If we need "hardware-specific" min/max value, we'd need another sysfs interfaces and values (i.e., scaling_min/max_freq vs cpuinfo_min/max_freq).
In my opinion, min_freq and max_freq itself represents the devfreq device freq limits. Additional sysfs interfaces may not be required.
Besides, if you are able to provide "freq_table", it means that you are able to provide OPP for the device. Then, you can use OPP APIs to avoid unnecessary checks at devfreq driver (even you can use the devfreq helper functions)
That's my intention as well. If driver is able to provide freq_table, based on the load, let devfreq core suggest only the supported opps (not just the limits, but for all intermediate opps). In which case, devfreq core must mandate drivers to add OPP table(instead of passing freq array) before registering with devfreq.
The devfreq device drivers are required to choose proper frequencies anyway regardless which values the governors may give (hopefully by choosing the closest value that can support the required performance).
Yes. but then each driver needs to have conditional checks for choosing closet value even though freq table is provided.
Even with this min/max-freq, when governors suggests some values between min/max, each driver needs to have checks unless it uses OPP.
With above proposal, devfreq should suggest only supported opps.
Besides, the min/max values are to be set by userspace. Users may enter 0 in order to express that they do not want to limit the behaviors of governors.
Because of this, this patch will create discrepency from/to userspace interfaces.
As said above, there is no conflict of interest between userspace and this patch.
In summary, I propose making OPP table mandatory for all devfreq drivers, to enable devfreq core to suggest only supported opps. Any thoughts?
Cheers, MyungJoo.
Cheers, MyungJoo.
drivers/devfreq/devfreq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index a8f0173..5782c9b 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -69,6 +69,29 @@ static struct devfreq *find_device_devfreq(struct device *dev) }
/**
- devfreq_set_freq_limits() - Set min and max frequency from freq_table
- @devfreq: the devfreq instance
- */
+static void devfreq_set_freq_limits(struct devfreq *devfreq) +{
int idx;unsigned long min = ~0, max = 0;if (!devfreq->profile->freq_table)return;for (idx = 0; idx < devfreq->profile->max_state; idx++) {if (min > devfreq->profile->freq_table[idx])min = devfreq->profile->freq_table[idx];if (max < devfreq->profile->freq_table[idx])max = devfreq->profile->freq_table[idx];}devfreq->min_freq = min;devfreq->max_freq = max;+}
+/**
- devfreq_get_freq_level() - Lookup freq_table for the frequency
- @devfreq: the devfreq instance
- @freq: the target frequency
@@ -476,6 +499,7 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq->profile->max_state, GFP_KERNEL); devfreq->last_stat_updated = jiffies;
devfreq_set_freq_limits(devfreq); dev_set_name(&devfreq->dev, dev_name(dev)); err = device_register(&devfreq->dev);-- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-- MyungJoo Ham, Ph.D. Mobile Software Platform Lab, DMC Business, Samsung Electronics
-- Regards, Rajagopal
-- MyungJoo Ham, Ph.D. System S/W Lab, S/W Center, Samsung Electronics
-
MyungJoo Ham -
MyungJoo Ham -
Rajagopal Venkat