Re: thermal governor: does it actually work??
From: Peter Feuerer
Date: Sun Feb 17 2013 - 10:42:07 EST
Hi,
Borislav Petkov writes:
On Sun, Feb 17, 2013 at 03:43:13AM +0100, Peter Feuerer wrote:
From 7b39bd8837de6dc5658ac3e54ac5d4df9d351528 Mon Sep 17 00:00:00 2001
From: Peter Feuerer <peter@xxxxxxxx>
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...
Don't think so, I think this was already in since 2.6.<something> and I
assume with this patch applied, acerhdf works from at least this
2.6.<something> up to new 3.8 and will still work in the future.
---
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.
As far as I understand the code (and Documentation/thermal/cpu-cooling-api.txt),
the thermal api finds the appropriate trip point and then set's the fan to
the corresponding state, defined by the thermal/fan driver. This is nice
thing, if you can completely control the speed of the fan, because you have
then a good fan speed to temperature regulation. But we do only have a two
point regulation (on and off), that's why we have to handle our thresholds
within the trip=1 on our own to not get an all the time on-off-toggling of
the fan.
Now this is very fragile, IMO.
I think this is how the developer of thermal_sys intended drivers to work.
But he forgot about two-point regulators (most probably because there's no
one besides acerhdf)
/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...
I don't understand what you mean by "in our case it doesn't do anything",
acerhdf is reporting the trip temperatures correctly, when get_trip_temp is
called.
Also, come to think of it, why don't we have THERMAL_TRIP_CRITICAL and
THERMAL_TRIP_HOT trip types?
You are right, we should at least add the THERMAL_TRIP_CRITICAL, so that we
handle this, but I think we can ignore THERMAL_TRIP_HOT, as it is not really
serving anything of value in our case.
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?
Yes, crit has to go there.
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.
As I wrote above, the thermal_sys layer do not serve 2 point regulation per
se, but with this check, we are able to achieve it. - We've done this check
already partly before:
/* turn fan off only if below fanoff temperature */
if ((cur_state == ACERHDF_FAN_AUTO) &&
(cur_temp < fanoff))
acerhdf_change_fanstate(ACERHDF_FAN_OFF);
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 :-)
I still think it is the right way to go, but maybe we should ask Durgadoss
R <durgadoss.r () intel.com>. It seems like he took over the thermal
handling by this commit:
commit 0c01ebbfd3caf1dc132e0d93c8e7e9f742839d94
Author: Durgadoss R <durgadoss.r()intel.com>
Date: Tue Sep 18 11:05:04 2012 +0530
Thermal: Remove throttling logic out of thermal_sys.c
have a nice sunday evening,
--peter;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/