Re: thermal governor: does it actually work??

[Borislav Petkov <bp@alien8.de>]

On Sun, Feb 17, 2013 at 03:43:13AM +0100, Peter Feuerer wrote:
> Hi Boris, Alex, Andreas,
> 
> what do you think about this acerhdf patch?
> I think it makes things straight and implements the two-point regulation of 
> acerhdf to be for correctly handled by the thermal layer:
> 
> 
> From 7b39bd8837de6dc5658ac3e54ac5d4df9d351528 Mon Sep 17 00:00:00 2001
> From: Peter Feuerer <peter@piie.net>
> Date: Sun, 17 Feb 2013 03:29:19 +0100
> Subject: [PATCH] added two more trip points to acerhdf, this allows thermal
>  layer to correctly handle the two point regulation of acerhdf. Trip point 0
>  will be active from 0 degree to "fanoff" and is marked as passive, then trip
>  point 1 is valid from "fanoff" to "fanon" value and is marked as active,
>  even if it's only really active in case temperature is going down from trip
>  point 2. Trip point 2 will be valid above "fanon" value and is also marked
>  as active.

Right, so this is clearly something new in the thermal pile of code. I
still don't understand the big picture all that clearly but whatever...

> ---
>  drivers/platform/x86/acerhdf.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c
> index f94467c..c36633b 100644
> --- a/drivers/platform/x86/acerhdf.c
> +++ b/drivers/platform/x86/acerhdf.c
> @@ -400,6 +400,10 @@ static int acerhdf_get_trip_type(struct thermal_zone_device *thermal, int trip,
>  				 enum thermal_trip_type *type)
>  {
>  	if (trip == 0)
> +		*type = THERMAL_TRIP_PASSIVE;
> +	if (trip == 1)
> +		*type = THERMAL_TRIP_ACTIVE;
> +	if (trip == 2)
>  		*type = THERMAL_TRIP_ACTIVE;

So, digging deep into thermal_sys.c, those naked numbers which we get
handed down for 'trip' are some sort of trip points. Now, I'd very much
like to know what those are and there are no defines what they mean -
code simply iterates over a number of thermal_zone trips - tz->trips -
and we (try to) act accordingly.

Now this is very fragile, IMO.

/me stares at the code a bit more.

Ok, from the looks of it, I'm guessing each driver has to do its own
mapping of what each trip point is, IIUC. And the thermal_zone doodles
over those and for those which the driver has defined, it asks the
driver itself what it wants done (i.e. ->get_trip_temp) and, in our case
it doesn't do anything...

Also, come to think of it, why don't we have THERMAL_TRIP_CRITICAL and
THERMAL_TRIP_HOT trip types?

>  	return 0;
> @@ -409,6 +413,10 @@ static int acerhdf_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>  				 unsigned long *temp)
>  {
>  	if (trip == 0)
> +		*temp = 0;
> +	if (trip == 1)
> +		*temp = fanoff;
> +	if (trip == 2)
>  		*temp = fanon;

Maybe the critical and hot types need to go here? I.e., 3 and 4?

>  	return 0;
> @@ -486,7 +494,8 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev,
>  		    (cur_temp < fanoff))
>  			acerhdf_change_fanstate(ACERHDF_FAN_OFF);
>  	} else {
> -		if (cur_state == ACERHDF_FAN_OFF)
> +		if ((cur_state == ACERHDF_FAN_OFF) &&
> +		    (cur_temp > fanon))
>  			acerhdf_change_fanstate(ACERHDF_FAN_AUTO);

... and we hook in into the thermal_cdev_update() call here and do the
correction ourselves.

Oh well. I need to befriend myself with the whole concept of thermal
- still have a bad feeling about it, like a star wars character:
http://www.youtube.com/watch?v=sBknAcTaMiI :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--