ACPI Processor update [idle,throttling,thermal,cpufreq]

Over the last few hours, I've rewritten large portions of ACPI processor handling. Due to lack of appropriate hardware, I can hardly test these changes, so expect a few rough edges.

These patches
- a) replace the broken ACPI CPUfreq driver with a better, flexible variant,
- b) modularize the processor.c code: instead of one large file there are many small files.
- c) improve passive cooling support
- d) allow for easy adding of proper locking and ref-counting in processor.c

http://www.brodo.de/cpufreq_tmp/cpufreq-2.6.0-test4-ALL-series1
Large CPUfreq update -- needed for other changes [in CPUfreq BK tree, merge with Linus expected soon]

http://www.brodo.de/cpufreq_tmp/cpufreq-2.6.0-test4-update_policy
http://www.brodo.de/cpufreq_tmp/cpufreq-2.6.0-test4-Kconfig-speedstep
Additional CPUfreq updates -- needed for other changes [Already sent to CPUfreq list]

http://www.brodo.de/cpufreq_tmp/drivermodel-2.6.0-test4-exports
Missing EXPORTs in sysdevice -- needed for other changes [Already sent to Patrick]

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-remove_previous_pstates_implementation
The previous arch/i386/kernel/cpu/cpufreq/acpi.c CPUfreq driver was broken, of bad design, and needs to replaced by something better. But, as a first step, remove all parts related to P-States from ACPI code.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_submodules
Add a "submodule interface" to drivers/acpi/processor.c

It allows to create other "modules" which access the acpi_handle for the processor, and which get notified if the event value maches the value passed in acpi_processor_register_notify.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_perflib
This patch adds a new "P-States library" to drivers/acpi/

CPUfreq drivers can now easly access the contents of the _PCT and the _PSS. For example, the speedstep-centrino driver could be appended so that it passes the appropriate value to the P-States library which then evaluates _PDC, and then returns the updated _PCT and _PSS.

Also, the platform limit is now handled as a cpufreq notifier and as a call to cpufreq_policy_update.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-new_acpi_io_driver
This re-adds the acpi P-States I/O driver. It is much smaller, leaner and cleaner.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_idle_submodule
This moves the idle handler out of drivers/acpi/processor and into an own module.

Even if only C1 is available, it is now used. If the user prefers the default pm_idle, he can unload processor_idle, and still have the other functionality available.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_submodule
This adds a new mechanism to manage passive cooling. Instead of hardcoded -and partly wrong- access to CPUfreq and ACPI throttling-, add a generic mechanism which other modules can register with.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_cpufreq
Use _any_ CPUfreq driver for passive cooling. Implemented by an cpufreq policy notifier and cpufreq_update_policy.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling
Move throttling into its own submodule, and register it with the new passive cooling module. Also, the now-useless "limit" interface is removed.

Please review, test (carefully), and -if appropriate- merge,

Dominik

Dominik,

I have couple of concerns regarding P-states IO library, esp. acpi_processor_get_frequency. It seems that ACPI does not allow to read current state without setting it first, at least with the following code on my notebook I was always getting garbage in the status register:

+static int
+acpi_processor_get_current_state(
+ struct acpi_processor_performance *perf)
+{

+       int     result = 0;
+       int     i;
+       u8      value;

+
+ ACPI_FUNCTION_TRACE("acpi_processor_get_current_state");
+
+ if (!perf->pr->flags.performance)
+ goto out;
+
+ value = inb(perf->status_register);
+
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "Got 0x%02x from port 0x%04x\n", value, perf->status_register));
+
+ for (i = 0; i < perf->state_count; i++) {
+ if (value == (u8) perf->states[i].status) {
+ perf->state = i;
+ goto out;
+ }
+ }
+
+ /* couldn't match our state table - garbage in status register */
+ ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+ "Bad data 0x%02x in performance status register 0x%04x\n",
+ value, perf->status_register));
+ result = -EFAULT;
+out:
+ return_VALUE(result);
+}

I am wondering is it just me, my code or general deficiency.

Also, do you really need to do notify_transition twice (acpi_processor_set_performance)?

Dmitry

Dmitry,

Thanks for your input.

Indeed. The specification allows this behaviour. That's really annoying, but well...

No, it's a bug.

The attached patch should fix both issues.

diff -ruN linux-original/arch/i386/kernel/cpu/cpufreq/acpi-io-cpufreq.c linux/arch/i386/kernel/cpu/cpufreq/acpi-io-cpufreq.c
@@ -33,6 +33,7 @@
 #include <asm/io.h>
 #include <asm/delay.h>
 #include <asm/uaccess.h>
+#include <asm/timex.h>
 
 #include <linux/acpi.h>
 #include <acpi/processor.h>

@@ -54,32 +55,16 @@
 	u16 				control_register;
 	u16 				status_register;
 
+	unsigned long			previous_freq;
 	struct cpufreq_frequency_table	*table;
 };
 
 static struct cpufreq_acpi_io_data acpi_io_data[NR_CPUS];
 
-static unsigned long acpi_processor_get_frequency (
-	struct cpufreq_acpi_io_data 	*data)
-{
-	unsigned int		i;
-	u16			port;
-	u8			value;
-
-	port = data->status_register;
-	value = inb(port);
-
-	for (i=0; i<data->data->state_count; i++) {
-		if (value == (u8) data->data->states[i].status)
-			return (1000 * data->data->states[i].core_frequency);
-	}
-
-	return 0;
-}
-
 static int acpi_processor_set_performance (
 	struct cpufreq_acpi_io_data 	*data,
-	int				state)
+	int				state,
+	unsigned int			notify)
 {
 	u16			port;
 	u8			value;

@@ -88,14 +73,12 @@
 
 	/* cpufreq frequency struct */
 	cpufreq_freqs.cpu = data->cpu;
-	cpufreq_freqs.old = acpi_processor_get_frequency(data);
+	cpufreq_freqs.old = data->previous_freq;
 	cpufreq_freqs.new = data->data->states[state].core_frequency;
 
 	/* notify cpufreq */
-	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
-
-	/* notify cpufreq */
-	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
+	if (notify)
+		cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
 
 	/*
 	 * First we write the target state's 'control' value to the

@@ -129,18 +112,21 @@
 	}
 
 	/* notify cpufreq */
+	if (notify)
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
 
 	if (value != data->data->states[state].status) {
 		unsigned int tmp = cpufreq_freqs.new;
 		cpufreq_freqs.new = cpufreq_freqs.old;
 		cpufreq_freqs.old = tmp;
-		cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
-		cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
+		if (notify) {
+			cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
+			cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
+		}
 		printk(KERN_ERR "Transition failed\n");
 		return -EINVAL;
 	}
-
+	data->previous_freq = data->data->states[state].core_frequency * 1000;
 	dprintk(KERN_INFO "Transition successful after %d microseconds\n", i * 10);
 	return 0;
 }

@@ -162,7 +148,7 @@
 	if (result)
 		return (result);
 
-	return acpi_processor_set_performance (data, next_state);
+	return acpi_processor_set_performance (data, next_state, 1);
 }
 
 static int acpi_cpufreq_verify (struct cpufreq_policy   *policy)

@@ -183,6 +169,7 @@
 	struct acpi_perflib_data *data;
 	struct cpufreq_frequency_table *table;
 	unsigned int i;
+	unsigned long tmp_speed;
 
 	data = acpi_processor_perflib_register (&acpi_cpufreq_perflib_driver,
 						 policy->cpu);

@@ -221,7 +208,29 @@
 	acpi_io_data[policy->cpu].status_register = (u16) data->pct_status.address;
 	acpi_io_data[policy->cpu].control_register = (u16) data->pct_control.address;
 
-	return 0;
+	/* 
+	 * The ACPI specificiation is broken in the regard that
+	 * it is impossible to detect the _current_ P-State.
+	 * The control_register is only valid _after_ a frequency
+	 * transition.
+	 * So, we try to get the current setting from cpu_khz,
+	 * but to make sure, this state we think the CPU is in
+	 * is also set.
+	 */
+	if (cpu_khz) {
+		tmp_speed = cpu_khz / 100;
+		tmp_speed *= 105;
+	} else {
+		tmp_speed = policy->cpuinfo.max_freq;
+	}
+	result = cpufreq_frequency_table_target(policy, data->table,
+						tmp_speed,
+						CPUFREQ_RELATION_L,
+						&i);
+	if (result)
+		return (result);
+
+	return acpi_processor_set_performance (data, i, 0);
 }

Hi!

Good - it is going to help a lot on k8 notebooks.

http://www.brodo.de/cpufreq_tmp/acpi-2.6.0-test4-processor_thermal_throttling

Actually I liked to be able to echo 0:7 > limit and conserve battery/ make sure machine does not overheat with it...

				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...

Hi Pavel,

Glad you liked [at least parts] of my patches.

echo 7 > throttling should do the same thing. However, don't expect any battey conservation if ACPI C2 throttling works, and to set custom levels against "overheating" there's a write access to /proc/acpi/thermal/*/trip_points too...

Dominik

Hi!

:-). [BTW what is status of powernow-k8? is it pending or does it need to be fixed some more?]

Will not first change in thermal system unthrottle it for me, behind my back?

I'd should not expect battery conservation *when cpu is idle*. If I'm moving windows in X, 0:7 limit will actually help. And it should allow battery to be drained further by limiting maximum amps eaten by CPU. [Ok, that's little evil, but being able to run for 30 minutes+ on 0% battery is nice ;-)].
Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Hi,

[powernow-k8 see separate e-mail]

Well yes, it might, if the temperature had been higher than the passive cooling trip point. If you want me to, I'll change the behaviour so that the /proc/acpi/.../throttling input is consistent (minimum limit) until the user choses a different throttling level.

depends if you see it relative to the time or relative to the energy...

Well yes, for this case, setting throttling to 0:7 might indeed make sense...

Dominik

Hi!

Yes I guess that would make sense.

Pavel

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

As Pavel Machek has noted, it may be desireable for an user to force a ceratin throttling level which is kept even if a thermal "decrease cooling" commands are issued. So, save the user input, and make sure throttling is at least set to this user_state

diff -ruN linux-original/drivers/acpi/processor_throttling.c linux/drivers/acpi/processor_throttling.c
@@ -77,6 +77,7 @@
 struct acpi_processor_throttling {
 	acpi_handle		handle;
 	int			state;
+	int			user_state;
 	u32			address;
 	u8			duty_offset;
 	u8			duty_width;

@@ -211,8 +212,9 @@
 
 	seq_puts(seq, "states:\n");
 	for (i = 0; i < throttle_data[cpu].state_count; i++)
-		seq_printf(seq, "   %cT%d:                  %02d%%\n",
+		seq_printf(seq, " %c%cT%d:                  %02d%%\n",
 			(i == throttle_data[cpu].state?'*':' '), i,
+			(i >= throttle_data[cpu].user_state?'u':' '), i,
 			(throttle_data[cpu].states[i].performance?throttle_data[cpu].states[i].performance/10:0));
 
 end:

@@ -252,6 +254,8 @@
 	if (result)
 		return_VALUE(result);
 
+	throttle_data[cpu].user_state = simple_strtoul(state_string, NULL, 0);
+
 	return_VALUE(count);
 }
 

@@ -536,6 +540,9 @@
 	if (throttle_data[cpu].state == 0)
 		return -ERANGE;
 
+	if (throttle_data[cpu].state == throttle_data[cpu].user_state)
+		return -ERANGE;
+
 	ret = acpi_processor_set_throttling(cpu, throttle_data[cpu].state - 1);
 
 	return ret;

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Am Mi, 2003-09-10 um 02.17 schrieb [email protected]:

[...]

This makes sense! Thanks!
Additionally also the performance setting should be kept like this. I always have the problem when having used passive cooling that on its way down from max-temp the CPU is forced into P0 whereas P1 would conserve power and cause less heat.
Manually resetting to P1 just causes it to be reset to P0 after some seconds. Tools like autospeedstepd (very nice BTW!) are afterwards also unable to switch the CPU to P1 again. If this would work it, passive cooling would make more sense ;)

CU
nils faerber

kernel concepts          Tel: +49-271-771091-12
Dreisbachstr. 24         Fax: +49-271-771091-19
D-57250 Netphen          D1 : +49-170-2729106
--



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

This is one of the "features" in my patchset :-)

Dominik

-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf