[PATCH v2 2/4] PM / devfreq: cache the last call to get_dev_status()

[PATCH v2 2/4] PM / devfreq: cache the last call to get_dev_status()

[Javi Merino]

The return value of get_dev_status() can be reused.  Cache it so that
other parts of the kernel can reuse it instead of having to call the
same function again.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---

This patch tries to let multiple components.  To summarize the
discussion in v1[1] I can think of three alternatives:

  1) Change get_dev_status() to return absolute values for busy_time
     and total_time
  2) Make core devfreq call get_dev_status() periodically (for
     example, before calling the governor) and all the entities that
     want access can do so via a pointer in devfreq
  3) Make the simple ondemand governor call get_dev_status()
     periodically and cache it, forcing all the other entities to
     rely on that governor being active

[1] http://thread.gmane.org/gmane.linux.power-management.general/61936/focus=61993

This patch implements option 3)

 drivers/devfreq/devfreq.c                 |  5 +++++
 drivers/devfreq/governor_simpleondemand.c | 33 +++++++++++++++++--------------
 include/linux/devfreq.h                   |  7 +++++++
 3 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 5b3f2f32f1e5..83041931910f 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -810,6 +810,11 @@ unlock:
 }
 EXPORT_SYMBOL(devfreq_set_min);
 
+int devfreq_update_stats(struct devfreq *df)
+{
+	return df->profile->get_dev_status(df->dev.parent, &df->last_status);
+}
+
 static ssize_t governor_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index 0720ba84ca92..ae72ba5e78df 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -21,17 +21,20 @@
 static int devfreq_simple_ondemand_func(struct devfreq *df,
 					unsigned long *freq)
 {
-	struct devfreq_dev_status stat;
-	int err = df->profile->get_dev_status(df->dev.parent, &stat);
+	int err;
+	struct devfreq_dev_status *stat;
 	unsigned long long a, b;
 	unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
 	unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
 	struct devfreq_simple_ondemand_data *data = df->data;
 	unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
 
+	err = devfreq_update_stats(df);
 	if (err)
 		return err;
 
+	stat = &df->last_status;
+
 	if (data) {
 		if (data->upthreshold)
 			dfso_upthreshold = data->upthreshold;
@@ -43,41 +46,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 		return -EINVAL;
 
 	/* Assume MAX if it is going to be divided by zero */
-	if (stat.total_time == 0) {
+	if (stat->total_time == 0) {
 		*freq = max;
 		return 0;
 	}
 
 	/* Prevent overflow */
-	if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) {
-		stat.busy_time >>= 7;
-		stat.total_time >>= 7;
+	if (stat->busy_time >= (1 << 24) || stat->total_time >= (1 << 24)) {
+		stat->busy_time >>= 7;
+		stat->total_time >>= 7;
 	}
 
 	/* Set MAX if it's busy enough */
-	if (stat.busy_time * 100 >
-	    stat.total_time * dfso_upthreshold) {
+	if (stat->busy_time * 100 >
+	    stat->total_time * dfso_upthreshold) {
 		*freq = max;
 		return 0;
 	}
 
 	/* Set MAX if we do not know the initial frequency */
-	if (stat.current_frequency == 0) {
+	if (stat->current_frequency == 0) {
 		*freq = max;
 		return 0;
 	}
 
 	/* Keep the current frequency */
-	if (stat.busy_time * 100 >
-	    stat.total_time * (dfso_upthreshold - dfso_downdifferential)) {
-		*freq = stat.current_frequency;
+	if (stat->busy_time * 100 >
+	    stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
+		*freq = stat->current_frequency;
 		return 0;
 	}
 
 	/* Set the desired frequency based on the load */
-	a = stat.busy_time;
-	a *= stat.current_frequency;
-	b = div_u64(a, stat.total_time);
+	a = stat->busy_time;
+	a *= stat->current_frequency;
+	b = div_u64(a, stat->total_time);
 	b *= 100;
 	b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
 	*freq = (unsigned long) b;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index dce82c695955..1bad6fecf285 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -161,6 +161,7 @@ struct devfreq {
 	struct delayed_work work;
 
 	unsigned long previous_freq;
+	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
 
@@ -206,6 +207,7 @@ extern void devm_devfreq_unregister_opp_notifier(struct device *dev,
 
 int devfreq_set_min(struct devfreq *df, unsigned long value);
 int devfreq_set_max(struct devfreq *df, unsigned long value);
+int devfreq_update_stats(struct devfreq *df);
 
 #if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)
 /**
@@ -302,6 +304,11 @@ static inline int devfreq_set_max(struct devfreq *df, unsigned long value)
 {
 	return -EINVAL;
 }
+
+static inline int devfreq_update_stats(struct devfreq *df)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_PM_DEVFREQ */
 
 #endif /* __LINUX_DEVFREQ_H__ */
-- 
1.9.1

Re: [PATCH v2 2/4] PM / devfreq: cache the last call to get_dev_status()

[MyungJoo Ham]

> The return value of get_dev_status() can be reused.  Cache it so that
> other parts of the kernel can reuse it instead of having to call the
> same function again.
> 
> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Javi Merino <javi.merino@arm.com>
> ---
> 
> This patch tries to let multiple components.  To summarize the
> discussion in v1[1] I can think of three alternatives:
> 
>   1) Change get_dev_status() to return absolute values for busy_time
>      and total_time
>   2) Make core devfreq call get_dev_status() periodically (for
>      example, before calling the governor) and all the entities that
>      want access can do so via a pointer in devfreq
>   3) Make the simple ondemand governor call get_dev_status()
>      periodically and cache it, forcing all the other entities to
>      rely on that governor being active
> 
> [1] http://thread.gmane.org/gmane.linux.power-management.general/61936/focus=61993
> 
> This patch implements option 3)
> 
>  drivers/devfreq/devfreq.c                 |  5 +++++
>  drivers/devfreq/governor_simpleondemand.c | 33 +++++++++++++++++--------------
>  include/linux/devfreq.h                   |  7 +++++++
>  3 files changed, 30 insertions(+), 15 deletions(-)
> 

Dear Javi,


Another question has risen as seeing Chanwoo's RFC patches,
which have a similar objective to different devices with
much less intervention:

[RFC PATCH 0/2] thermal: Add generic devfreq cooling device
https://lkml.org/lkml/2015/7/16/334
(This patch set also requires updates as it appears not
to use opp notifiers properly, but it shows the direction
on how to implement thermal/qos inputs to devfreq devices)

Can't you simply use OPP's disable/enable functions and
let devfreq automatically update itself accordingly?
It simplifies your codes as well especially if you put
your possible frequencies to DT.


Cheers,
MyungJoo

ps. Chanwoo, I will comment on your patches on the usage of
opp-notifiers as well.

Re: [PATCH v2 2/4] PM / devfreq: cache the last call to get_dev_status()

[Javi Merino]

Hi MyungJoo,

On Fri, Jul 17, 2015 at 05:31:38AM +0100, MyungJoo Ham wrote:
> > The return value of get_dev_status() can be reused.  Cache it so that
> > other parts of the kernel can reuse it instead of having to call the
> > same function again.
> > 
> > Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> > 
> > This patch tries to let multiple components.  To summarize the
> > discussion in v1[1] I can think of three alternatives:
> > 
> >   1) Change get_dev_status() to return absolute values for busy_time
> >      and total_time
> >   2) Make core devfreq call get_dev_status() periodically (for
> >      example, before calling the governor) and all the entities that
> >      want access can do so via a pointer in devfreq
> >   3) Make the simple ondemand governor call get_dev_status()
> >      periodically and cache it, forcing all the other entities to
> >      rely on that governor being active
> > 
> > [1] http://thread.gmane.org/gmane.linux.power-management.general/61936/focus=61993
> > 
> > This patch implements option 3)
> > 
> >  drivers/devfreq/devfreq.c                 |  5 +++++
> >  drivers/devfreq/governor_simpleondemand.c | 33 +++++++++++++++++--------------
> >  include/linux/devfreq.h                   |  7 +++++++
> >  3 files changed, 30 insertions(+), 15 deletions(-)
> > 
> 
> Another question has risen as seeing Chanwoo's RFC patches,
> which have a similar objective to different devices with
> much less intervention:
> 
> [RFC PATCH 0/2] thermal: Add generic devfreq cooling device
> https://lkml.org/lkml/2015/7/16/334
> (This patch set also requires updates as it appears not
> to use opp notifiers properly, but it shows the direction
> on how to implement thermal/qos inputs to devfreq devices)
> 
> Can't you simply use OPP's disable/enable functions and
> let devfreq automatically update itself accordingly?
> It simplifies your codes as well especially if you put
> your possible frequencies to DT.

Ok, we can use OPP enable/disable to modify the available OPPs and let
devfreq take the appropriate decision. I'll send a v3 next week.

Still, the question of how to let multiple components access
get_dev_status() is still open.  Have you had time to consider it?

Cheers,
Javi