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

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

[PATCH v2 0/4] Devfreq cooling device

[Javi Merino]

This series introduces a devfreq cooling device in the thermal
framework.  Devfreq is used for DVFS for devices other than the CPUs.
With a devfreq cooling device, the thermal framework can throttle them
to control temperature.  The cooling device has the power extensions,
so it can be used by all governors in the thermal framework, including
the power allocator governor.

Changes since v1:
  - Rename devfreq_qos_set_[max|min] to devfreq_set_[max|min] as
    suggested by MyungJoo Ham
  - Calculate devfreq load in the tracepoint so that it only happens
    when the trace is enabled.  Thanks Steven Rostedt.

Javi Merino (2):
  PM / devfreq: cache the last call to get_dev_status()
  devfreq_cooling: add trace information

Ørjan Eide (2):
  PM / devfreq: Add function to set max/min frequency
  thermal: Add devfreq cooling

 drivers/devfreq/devfreq.c                 |  77 ++++--
 drivers/devfreq/governor_simpleondemand.c |  33 +--
 drivers/thermal/Kconfig                   |  10 +
 drivers/thermal/Makefile                  |   3 +
 drivers/thermal/devfreq_cooling.c         | 394 ++++++++++++++++++++++++++++++
 include/linux/devfreq.h                   |  20 ++
 include/linux/devfreq_cooling.h           |  90 +++++++
 include/trace/events/thermal.h            |  53 ++++
 8 files changed, 641 insertions(+), 39 deletions(-)
 create mode 100644 drivers/thermal/devfreq_cooling.c
 create mode 100644 include/linux/devfreq_cooling.h

-- 
1.9.1

[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