[3/3] PM / devfreq: account suspend/resume for stats

Message ID 1357624239-13938-3-git-send-email-rajagopal.venkat@linaro.org
State Accepted
Commit 39688ce6facd63de796def41a0b9012882b5cc14
Headers show

Commit Message

rajagopal.venkat@linaro.org Jan. 8, 2013, 5:50 a.m.
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(-)

Comments

MyungJoo Ham Jan. 14, 2013, 2:48 p.m. | #1
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
rajagopal.venkat@linaro.org Jan. 15, 2013, 11:46 a.m. | #2
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
rajagopal.venkat@linaro.org Feb. 4, 2013, 8:34 a.m. | #3
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
MyungJoo Ham Feb. 5, 2013, 6:29 a.m. | #4
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

Patch

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");