Message-ID: <20140819160254.GA16982@e104805>
Date:	Tue, 19 Aug 2014 17:02:54 +0100
From:	"Javi Merino" <javi.merino@....com>
To:	Eduardo Valentin <edubezval@...il.com>
Cc:	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Punit Agrawal <Punit.Agrawal@....com>,
	"broonie@...nel.org" <broonie@...nel.org>,
	Zhang Rui <rui.zhang@...el.com>
Subject: Re: [RFC PATCH v5 08/10] thermal: introduce the Power Allocator
 governor

On Tue, Aug 19, 2014 at 02:45:52PM +0100, Eduardo Valentin wrote:
> Javi,

Hi Eduardo,

> On Thu, Jul 10, 2014 at 03:18:46PM +0100, Javi Merino wrote:
> > The power allocator governor is a thermal governor that controls system
> > and device power allocation to control temperature.  Conceptually, the
> > implementation divides the sustainable power of a thermal zone among
> > all the heat sources in that zone.
> >
> > This governor relies on "power actors", entities that represent heat
> > sources.  They can report current and maximum power consumption and
> > can set a given maximum power consumption, usually via a cooling
> > device.
> >
> > The governor uses a Proportional Integral Derivative (PID) controller
> > driven by the temperature of the thermal zone.  The output of the
> > controller is a power budget that is then allocated to each power
> > actor that can have bearing on the temperature we are trying to
> > control.  It decides how much power to give each cooling device based
> > on the performance they are requesting.  The PID controller ensures
> > that the total power budget does not exceed the control temperature.
> >
> > Cc: Zhang Rui <rui.zhang@...el.com>
> > Cc: Eduardo Valentin <edubezval@...il.com>
> > Signed-off-by: Punit Agrawal <punit.agrawal@....com>
> > Signed-off-by: Javi Merino <javi.merino@....com>
> > ---
> >  Documentation/thermal/power_allocator.txt |  61 ++++
> >  drivers/thermal/Kconfig                   |  15 +
> >  drivers/thermal/Makefile                  |   1 +
> >  drivers/thermal/power_allocator.c         | 467 ++++++++++++++++++++++++++++++
> >  drivers/thermal/thermal_core.c            |   7 +-
> >  drivers/thermal/thermal_core.h            |   8 +
> >  include/linux/thermal.h                   |   8 +
> >  7 files changed, 566 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/thermal/power_allocator.txt
> >  create mode 100644 drivers/thermal/power_allocator.c
> >
> > diff --git a/Documentation/thermal/power_allocator.txt b/Documentation/thermal/power_allocator.txt
> > new file mode 100644
> > index 000000000000..1859074dadcb
> > --- /dev/null
> > +++ b/Documentation/thermal/power_allocator.txt
> > @@ -0,0 +1,61 @@
> > +Integration of the power_allocator governor in a platform
> > +=========================================================
> > +
> > +Registering thermal_zone_device
> > +-------------------------------
> > +
> > +An estimate of the sustainable dissipatable power (in mW) should be
> > +provided while registering the thermal zone.  This is the maximum
> > +sustained power for allocation at the desired maximum temperature.
> > +This number can vary for different conditions, but the closed-loop of
> > +the controller should take care of those variations, the
> > +`sustainable_power` should be an estimation of it.  Register your
> > +thermal zone with `thermal_zone_params` that have a
> > +`sustainable_power`.  If you weren't passing any
> > +`thermal_zone_params`, then something like this will do:
> > +
> > +     static const struct thermal_zone_params tz_params = {
> > +             .sustainable_power = 3500,
> > +     };
> > +
> > +and then pass `tz_params` as the 5th parameter to
> > +`thermal_zone_device_register()`
> > +
> > +Trip points
> > +-----------
> > +
> > +The governor requires the following two trip points:
> > +
> > +1.  "switch on" trip point: temperature above which the governor
> > +    control loop starts operating
> > +2.  "desired temperature" trip point: it should be higher than the
> > +    "switch on" trip point. It is the target temperature the governor
> > +    is controlling for.
> > +
> > +The trip points can be either active or passive.
> > +
> 
> I would prefer, for the sake of keeping the right concept in use, you
> state here that they are passive trip points.

Sure, I've also changed the function that checks this condition
accordingly.

> > +Power actors
> > +------------
> > +
> > +Devices controlled by this governor must be registered with the power
> > +actor API.  Read `power_actor.txt` for more information about them.
> > +
> > +Limitations of the power allocator governor
> > +===========================================
> > +
> > +The power allocator governor can't work with cooling devices directly.
> > +A power actor can be created to interface between the governor and the
> > +cooling device (see cpu_actor.c for an example).  Otherwise, if you
> > +have power actors and cooling devices that are next to the same
> > +thermal sensor create two thermal zones, one for each type.  Use the
> > +power allocator governor for the power actor thermal zone with the
> > +power actors and any other governor for the one with cooling devices.
> > +
> > +The power allocator governor's PID controller is highly dependent on a
> > +periodic tick.  If you have a driver that calls
> > +`thermal_zone_device_update()` (or anything that ends up calling the
> > +governor's `throttle()` function) repetitively, the governor response
> > +won't be very good.  Note that this is not particular to this
> > +governor, step-wise will also misbehave if you call its throttle()
> > +faster than the normal thermal framework tick (due to interrupts for
> > +example) as it will overreact.
> 
> I would recommend rephrasing the paragraph above, as I mentioned in
> other email, looks like a documented bug. Mentioning that the governor
> is still dependent on polling_delay active_delay is still worth it.
> Also, the coexistance of interrupt driven thermal zones with polling
> driven framework requires the interrupt firing to be configure to
> something meaneanful, such as, an event outside the temperature range
> the current trip is.

Ok, I'll rephrase it to clarify the point.

> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 249b196deffd..0e76c0dab5f3 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -71,6 +71,14 @@ config THERMAL_DEFAULT_GOV_USER_SPACE
> >         Select this if you want to let the user space manage the
> >         platform thermals.
> >
> > +config THERMAL_DEFAULT_GOV_POWER_ALLOCATOR
> > +     bool "power_allocator"
> > +     select THERMAL_GOV_POWER_ALLOCATOR
> > +     help
> > +       Select this if you want to control temperature based on
> > +       system and device power allocation. This governor relies on
> > +       power actors to operate.
> > +
> >  endchoice
> >
> >  config THERMAL_GOV_FAIR_SHARE
> > @@ -89,6 +97,13 @@ config THERMAL_GOV_USER_SPACE
> >       help
> >         Enable this to let the user space manage the platform thermals.
> >
> > +config THERMAL_GOV_POWER_ALLOCATOR
> > +     bool "Power allocator thermal governor"
> > +     select THERMAL_POWER_ACTOR
> > +     help
> > +       Enable this to manage platform thermals by dynamically
> > +       allocating and limiting power to devices.
> > +
> >  config THERMAL_POWER_ACTOR
> >       bool
> >
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 74f97c90a46c..e74d57d0fe61 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -13,6 +13,7 @@ thermal_sys-$(CONFIG_THERMAL_OF)            += of-thermal.o
> >  thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE) += fair_share.o
> >  thermal_sys-$(CONFIG_THERMAL_GOV_STEP_WISE)  += step_wise.o
> >  thermal_sys-$(CONFIG_THERMAL_GOV_USER_SPACE) += user_space.o
> > +thermal_sys-$(CONFIG_THERMAL_GOV_POWER_ALLOCATOR)    += power_allocator.o
> >
> >  # power actors
> >  obj-$(CONFIG_THERMAL_POWER_ACTOR) += power_actor.o
> > diff --git a/drivers/thermal/power_allocator.c b/drivers/thermal/power_allocator.c
> > new file mode 100644
> > index 000000000000..eb1797cd859b
> > --- /dev/null
> > +++ b/drivers/thermal/power_allocator.c
> > @@ -0,0 +1,467 @@
> > +/*
> > + * A power allocator to manage temperature
> > + *
> > + * Copyright (C) 2014 ARM Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#define pr_fmt(fmt) "Power allocator: " fmt
> > +
> > +#include <linux/power_actor.h>
> > +#include <linux/rculist.h>
> > +#include <linux/slab.h>
> > +#include <linux/thermal.h>
> > +
> > +#include "thermal_core.h"
> > +
> > +#define FRAC_BITS 8
> > +#define int_to_frac(x) ((x) << FRAC_BITS)
> > +#define frac_to_int(x) ((x) >> FRAC_BITS)
> > +
> > +/**
> > + * mul_frac() - multiply two fixed-point numbers
> > + * @x:       first multiplicand
> > + * @y:       second multiplicand
> > + *
> > + * Return: the result of multiplying two fixed-point numbers.  The
> > + * result is also a fixed-point number.
> > + */
> > +static inline s64 mul_frac(s64 x, s64 y)
> > +{
> > +     return (x * y) >> FRAC_BITS;
> > +}
> > +
> > +enum power_allocator_trip_levels {
> > +     TRIP_SWITCH_ON = 0,     /* Switch on PID controller */
> > +     TRIP_MAX_DESIRED_TEMPERATURE, /* Temperature we are controlling for */
> > +};
> > +
> > +/**
> > + * struct power_allocator_params - parameters for the power allocator governor
> > + * @k_po:    Proportional parameter of the PID controller when overshooting
> > + *           (i.e., when temperature is below the target)
> > + * @k_pi:    Proportional parameter of the PID controller when undershooting
> > + * @k_i:     Integral parameter of the PID controller
> > + * @k_d:     Derivative parameter of the PID controller
> 
> Po, Pi, i, and d were not documented in your document above. Did you
> miss them?
> 
> Given that they are core part of the governor behavior, I'd suggest
> having a section in its document about how to fine tune the constants.
> Explain, for instance, if it is enough to fine tune per platform, and
> if having constants defined at compilation time or at runtime are good
> point to be documented.

We need to provide a way for platforms to specify these constants.
Currently they are fixed because I thought that some heuristic would
be good enough to cover all the platforms.  It turns out that it's not
realistic so we will provide a mechanism for platforms to change them,
we will export them in debugfs and document them in the next series.

Cheers,
Javi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists