[RFC][PATCH 0/0] measure speedstep-ich transition latency at CPU initialization

[RFC][PATCH 0/0] measure speedstep-ich transition latency at CPU initialization

[Mattia Dongili]

Hello,                                      
a foreword: this is my first patch here, it surely needs more work but I'd
appreciate some comments before going too far on the wrong lane.  Oh, I'm also
trying to use some git automation to send the patch, hopefully you won't
receive too many dups :)

The 2 patches are trying to implement the suggested runtime measurement
of transition latency on speedstep ich chipsets to offer the
allow the usage of the dynamic governors to ICH[234]-M owners (like me).

The first patch moves away PMBASE reading and only performs it at
cpufreq_register_driver time by exiting with -ENODEV if unable to read
the value. (git seems a little confused, I hope it's readable)

The second patch (could be split in 2 if necessary) introduces
__speedstep_set_state that performs the frequency transition without
disabling interrupts to be used in speedstep_measure_latency and
speedstep_get_freqs (IRQs are already disabled here).
It also implements the transition measurement if the parameter
'measure_latency' is set to a value other than 0.

The measurement is performed by means of do_gettimeofday as suggested by
Mr. Pallipadi.

Known problems:
- the patch is still missing a sanity check on the measured value
- it also introduces 2 more transitions at init time (2 in
  speedstep_get_freqs + 2 in speedstep_measure_latency) because I don't
  know if makes sense to rely (and hack) on speedstep_get_freqs to perform
  measurements.

thanks
--
mattia
:wq!

[PATCH 1/2] Move PMBASE reading away and do it only once at initialization time

[Mattia Dongili]

Signed-off-by: Mattia Dongili <malattia@linux.it>

---

 arch/i386/kernel/cpu/cpufreq/speedstep-ich.c |   45 ++++++++++++++++++--------
 1 files changed, 32 insertions(+), 13 deletions(-)

applies-to: 41c86716d1584bad6ddf55c9132ef8b820bc1b15
31c370b9ba20188340635fd7cebc150c4b71b097
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
index 5b7d18a..17b7864 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
@@ -40,6 +40,7 @@ static struct pci_dev *speedstep_chipset
  */
 static unsigned int speedstep_processor = 0;
 
+static u32 pmbase;
 
 /*
  *   There are only two frequency states for each processor. Values
@@ -56,34 +57,48 @@ static struct cpufreq_frequency_table sp
 
 
 /**
- * speedstep_set_state - set the SpeedStep state
- * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
+ * speedstep_find_register - read the PMBASE address
  *
- *   Tries to change the SpeedStep state.
+ * Returns: -ENODEV if no register could be found
  */
-static void speedstep_set_state (unsigned int state)
+static int speedstep_find_register (void)
 {
-	u32 pmbase;
-	u8 pm2_blk;
-	u8 value;
-	unsigned long flags;
-
-	if (!speedstep_chipset_dev || (state > 0x1))
-		return;
+	
+	if (!speedstep_chipset_dev)
+		return -ENODEV;
 
 	/* get PMBASE */
 	pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
 	if (!(pmbase & 0x01)) {
 		printk(KERN_ERR "speedstep-ich: could not find speedstep register\n");
-		return;
+		return -ENODEV;
 	}
 
 	pmbase &= 0xFFFFFFFE;
 	if (!pmbase) {
 		printk(KERN_ERR "speedstep-ich: could not find speedstep register\n");
-		return;
+		return -ENODEV;
 	}
 
+	dprintk("pmbase is 0x%x\n", pmbase);
+	return 0;
+}
+
+/**
+ * speedstep_set_state - set the SpeedStep state
+ * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
+ *
+ *   Tries to change the SpeedStep state.
+ */
+static void speedstep_set_state (unsigned int state)
+{
+	u8 pm2_blk;
+	u8 value;
+	unsigned long flags;
+
+	if (state > 0x1)
+		return;
+
 	/* Disable IRQs */
 	local_irq_save(flags);
 
@@ -400,6 +415,10 @@ static int __init speedstep_init(void)
 		return -EINVAL;
 	}
 
+	if (speedstep_find_register()) {
+		return -ENODEV;
+	}
+
 	return cpufreq_register_driver(&speedstep_driver);
 }
 
---
0.99.9.GIT

[PATCH 2/2] Measure transition latency at driver initialization

[Mattia Dongili]

Signed-off-by: Mattia Dongili <malattia@linux.it>

---

 arch/i386/kernel/cpu/cpufreq/speedstep-ich.c |   90 +++++++++++++++++++++++---
 1 files changed, 78 insertions(+), 12 deletions(-)

applies-to: 802a6f1c8699b27a187d801cf69a599409a6b5a4
1d29940b03826b73b9660abc348b8d2ede587587
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
index 17b7864..a931849 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
@@ -42,6 +42,8 @@ static unsigned int speedstep_processor 
 
 static u32 pmbase;
 
+static int measure_latency;
+
 /*
  *   There are only two frequency states for each processor. Values
  * are in kHz for the time being.
@@ -85,23 +87,20 @@ static int speedstep_find_register (void
 }
 
 /**
- * speedstep_set_state - set the SpeedStep state
+ * __speedstep_set_state - set the SpeedStep state
  * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
  *
  *   Tries to change the SpeedStep state.
+ *   NOTE: it doesn't disable interrupts use speedstep_set_state instead.
  */
-static void speedstep_set_state (unsigned int state)
+static void __speedstep_set_state (unsigned int state)
 {
 	u8 pm2_blk;
 	u8 value;
-	unsigned long flags;
 
 	if (state > 0x1)
 		return;
 
-	/* Disable IRQs */
-	local_irq_save(flags);
-
 	/* read state */
 	value = inb(pmbase + 0x50);
 
@@ -128,11 +127,8 @@ static void speedstep_set_state (unsigne
 	/* check if transition was successful */
 	value = inb(pmbase + 0x50);
 
-	/* Enable IRQs */
-	local_irq_restore(flags);
-
 	dprintk("read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
-
+	
 	if (state == (value & 0x1)) {
 		dprintk("change to %u MHz succeeded\n", (speedstep_get_processor_frequency(speedstep_processor) / 1000));
 	} else {
@@ -142,6 +138,68 @@ static void speedstep_set_state (unsigne
 	return;
 }
 
+/**
+ * speedstep_measure_latency - measure latency usec
+ *
+ * Returns: the detected latency.
+ */
+static long speedstep_measure_latency (void)
+{
+	unsigned long flags;
+	unsigned int cur_speed = 0;
+	int state = 0;
+	struct timeval t1, t2;
+
+	cur_speed = speedstep_get_processor_frequency(speedstep_processor);
+	/* find out in which order we have to switch frequencies
+	 * to really switch and measure latency time.
+	 */
+	if (speedstep_freqs[SPEEDSTEP_LOW].frequency == cur_speed) {
+		state = SPEEDSTEP_HIGH;
+	} else {
+		state = SPEEDSTEP_LOW;
+	}
+
+	/* Disable IRQs */
+	local_irq_save(flags);
+
+	do_gettimeofday(&t1);
+
+	__speedstep_set_state(state);
+
+	do_gettimeofday(&t2);
+
+	/* restore the previous frequency */
+	__speedstep_set_state(!state);
+
+	/* Enable IRQs */
+	local_irq_restore(flags);
+
+	return (t2.tv_sec - t1.tv_sec) * USEC_PER_SEC +
+		t2.tv_usec - t1.tv_usec;
+}
+
+/**
+ * speedstep_set_state - set the SpeedStep state
+ * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
+ *
+ *   Tries to change the SpeedStep state.
+ */
+static void speedstep_set_state (unsigned int state)
+{
+	unsigned long flags;
+
+	/* Disable IRQs */
+	local_irq_save(flags);
+
+	__speedstep_set_state(state);
+	
+	/* Enable IRQs */
+	local_irq_restore(flags);
+
+	return;
+}
+
 
 /**
  * speedstep_activate - activate SpeedStep control in the chipset
@@ -320,6 +378,7 @@ static int speedstep_cpu_init(struct cpu
 {
 	int result = 0;
 	unsigned int speed;
+	unsigned int transition_latency = CPUFREQ_ETERNAL;
 	cpumask_t cpus_allowed;
 
 	/* only run on CPU to be set, or on its sibling */
@@ -334,7 +393,7 @@ static int speedstep_cpu_init(struct cpu
 	result = speedstep_get_freqs(speedstep_processor,
 				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
-				     &speedstep_set_state);
+				     &__speedstep_set_state);
 	set_cpus_allowed(current, cpus_allowed);
 	if (result)
 		return result;
@@ -348,9 +407,14 @@ static int speedstep_cpu_init(struct cpu
 		(speed == speedstep_freqs[SPEEDSTEP_LOW].frequency) ? "low" : "high",
 		(speed / 1000));
 
+	if (measure_latency) {
+		transition_latency = speedstep_measure_latency() * 1000;
+		dprintk("measured transition latency is %u mS\n", transition_latency);
+	}
+	
 	/* cpuinfo and default policy values */
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+	policy->cpuinfo.transition_latency = transition_latency;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);
@@ -434,6 +498,8 @@ static void __exit speedstep_exit(void)
 	cpufreq_unregister_driver(&speedstep_driver);
 }
 
+module_param(measure_latency, int, 0444);
+MODULE_PARM_DESC(measure_latency, "Try to measure transition latency.");
 
 MODULE_AUTHOR ("Dave Jones <davej@codemonkey.org.uk>, Dominik Brodowski <linux@brodo.de>");
 MODULE_DESCRIPTION ("Speedstep driver for Intel mobile processors on chipsets with ICH-M southbridges.");
---
0.99.9.GIT

Re: [PATCH 2/2] Measure transition latency at driver initialization

[Eric Piel]

As stated before, the whole idea is probably good, at least it should 
finally get rid of CPUFREQ_ETERNAL transition. BTW, maybe some other 
drivers could benefit of the same approach :-)

However, I disagree a bit with the implementation:
* don't make it a module parameter: either this aproach works and the 
module should always use it or it doesn't. The problem with parameters 
is that only few users will end up using this feature.

* You should add a test to check that the transition measurement didn't 
go wrong. It would be a pitty that a user burns his CPU simply because 
the resolution of gettimeofday() was not good enough ;-) cf in the code

* At initialisation, the driver already changes twice of frequency (in 
speedstep_get_freqs()). No need to change twice more to measure the 
transitions! We should put the time measurement inside 
speedstep_get_freqs() or inside __speedstep_set_state(). Just check if 
transition_latency == CPUFREQ_ETERNAL, if so, do the measurement.

Few more comments below.



11/30/2005 12:58 AM, Mattia Dongili wrote/a écrit:
:
>  
> @@ -128,11 +127,8 @@ static void speedstep_set_state (unsigne
>  	/* check if transition was successful */
>  	value = inb(pmbase + 0x50);
>  
> -	/* Enable IRQs */
> -	local_irq_restore(flags);
> -
>  	dprintk("read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
> -
> +	
Watch out not to insert non needed tabs ;-)

:
>  
>  /**
>   * speedstep_activate - activate SpeedStep control in the chipset
> @@ -320,6 +378,7 @@ static int speedstep_cpu_init(struct cpu
>  {
>  	int result = 0;
>  	unsigned int speed;
> +	unsigned int transition_latency = CPUFREQ_ETERNAL;
>  	cpumask_t cpus_allowed;
>  
>  	/* only run on CPU to be set, or on its sibling */
> @@ -334,7 +393,7 @@ static int speedstep_cpu_init(struct cpu
>  	result = speedstep_get_freqs(speedstep_processor,
>  				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
>  				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
> -				     &speedstep_set_state);
> +				     &__speedstep_set_state);
>  	set_cpus_allowed(current, cpus_allowed);
>  	if (result)
>  		return result;
> @@ -348,9 +407,14 @@ static int speedstep_cpu_init(struct cpu
>  		(speed == speedstep_freqs[SPEEDSTEP_LOW].frequency) ? "low" : "high",
>  		(speed / 1000));
>  
> +	if (measure_latency) {
> +		transition_latency = speedstep_measure_latency() * 1000;
> +		dprintk("measured transition latency is %u mS\n", transition_latency);
> +	}
> +	
The measurement was done only once, while transition_latency should be 
the maximum possible. It's safer to multiply the measurement by some 
factor. As we've noticed it's very stable, maybe 120% should be OK:
/*
  * converts from microseconds to nanoseconds and add 20% security margin
  */
transition_latency = speedstep_measure_latency() * 1200;

Additionally, I'd like to see some security test like this:
/* does the measurement seem to have reasonable value ? */
if ((transition_latency > 10000000) || (transition_latency < 50000))
	transition_latency = 500000; /* 500us is a very high guess */

:


cu,
Eric

Re: [PATCH 2/2] Measure transition latency at driver initialization

[Mattia Dongili]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

Hello Eric!

On Wed, Nov 30, 2005 at 12:46:25PM +0100, Eric Piel wrote:
> As stated before, the whole idea is probably good, at least it should 
> finally get rid of CPUFREQ_ETERNAL transition. BTW, maybe some other 
> drivers could benefit of the same approach :-)

grepping around I've just seen that CPUFREQ_ETERNAL is much more in use
than I expected! It's a shame so few people can benefit from
ondemand/conservative governors...

> However, I disagree a bit with the implementation:
> * don't make it a module parameter: either this aproach works and the 
> module should always use it or it doesn't. The problem with parameters 
> is that only few users will end up using this feature.

Yes, sorry. The intention was to make it available without breaking (hmm?)
current setups.

> * You should add a test to check that the transition measurement didn't 
> go wrong. It would be a pitty that a user burns his CPU simply because 
> the resolution of gettimeofday() was not good enough ;-) cf in the code
> 
> * At initialisation, the driver already changes twice of frequency (in 
> speedstep_get_freqs()). No need to change twice more to measure the 

I have a patch ready that does it in speedstep_get_freqs but it
looks ugly, it adds a lot of do_gettimeofay's around... It's probably
still not optimal and maybe trying to think of it as a more general
solution (from which all of the drivers could also benefit) should
result in nicer code.
I also had to modify speedstep-smi that still doesn't benefit 100% of
the times of the feature.

> transitions! We should put the time measurement inside 
> speedstep_get_freqs() or inside __speedstep_set_state(). Just check if 
> transition_latency == CPUFREQ_ETERNAL, if so, do the measurement.

well, in speedstep_get_freqs the above check is not really necessary, as
the only current paths reaching that function will return true.

Oh, and do you think __speedstep_set_state should remain? The reason I
added it was to avoid disabling interrupts twice. The attached
implementation without it is in fact a little less precise (values
have increased of ~25-30uSec on my PIIIM)

[...]
> > 	dprintk("read at pmbase 0x%x + 0x50 returned 0x%x\n", pmbase, value);
> >-
> >+	
> Watch out not to insert non needed tabs ;-)

Yup! I also have removed some this time ;)

Signed-off-by: Mattia Dongili <malattia@linux.it>
-- 
mattia
:wq!

[-- Attachment #2: measure_transition_latency.diff --]
[-- Type: text/plain, Size: 6275 bytes --]

diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
index 77efe8e..b425cd3 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
@@ -329,10 +329,11 @@ static int speedstep_cpu_init(struct cpu
 	cpus_allowed = current->cpus_allowed;
 	set_cpus_allowed(current, policy->cpus);
 
-	/* detect low and high frequency */
+	/* detect low and high frequency and transition latency */
 	result = speedstep_get_freqs(speedstep_processor,
 				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+				     &policy->cpuinfo.transition_latency,
 				     &speedstep_set_state);
 	set_cpus_allowed(current, cpus_allowed);
 	if (result)
@@ -349,7 +350,6 @@ static int speedstep_cpu_init(struct cpu
 
 	/* cpuinfo and default policy values */
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
index d368b3f..9661d9e 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
@@ -19,6 +19,8 @@
 #include <asm/msr.h>
 #include "speedstep-lib.h"
 
+#define MAX(a,b)	((a)>(b) ? (a) : (b))
+
 #define dprintk(msg...) cpufreq_debug_printk(CPUFREQ_DEBUG_DRIVER, "speedstep-lib", msg)
 
 #ifdef CONFIG_X86_SPEEDSTEP_RELAXED_CAP_CHECK
@@ -320,13 +322,17 @@ EXPORT_SYMBOL_GPL(speedstep_detect_proce
 unsigned int speedstep_get_freqs(unsigned int processor,
 				  unsigned int *low_speed,
 				  unsigned int *high_speed,
+				  unsigned int *transition_latency,
 				  void (*set_state) (unsigned int state))
 {
 	unsigned int prev_speed;
 	unsigned int ret = 0;
 	unsigned long flags;
+	unsigned int latency;
+	struct timeval tv1, tv2;
 
-	if ((!processor) || (!low_speed) || (!high_speed) || (!set_state))
+	if ((!processor) || (!low_speed) || (!high_speed)
+			|| (!set_state) || (!transition_latency))
 		return -EINVAL;
 
 	dprintk("trying to determine both speeds\n");
@@ -337,37 +343,72 @@ unsigned int speedstep_get_freqs(unsigne
 		return -EIO;
 
 	dprintk("previous speed is %u\n", prev_speed);
-	
+
 	local_irq_save(flags);
 
+	do_gettimeofday(&tv1);
 	/* switch to low state */
 	set_state(SPEEDSTEP_LOW);
+	do_gettimeofday(&tv2);
+
 	*low_speed = speedstep_get_processor_frequency(processor);
 	if (!*low_speed) {
 		ret = -EIO;
 		goto out;
 	}
 
-	dprintk("low speed is %u\n", *low_speed);
+	latency = (tv2.tv_sec - tv1.tv_sec) * USEC_PER_SEC +
+		tv2.tv_usec - tv1.tv_usec;
 
+	dprintk("low speed is %u - latency is %u\n",
+			*low_speed, latency);
+
+	do_gettimeofday(&tv1);
 	/* switch to high state */
 	set_state(SPEEDSTEP_HIGH);
+	do_gettimeofday(&tv2);
+
 	*high_speed = speedstep_get_processor_frequency(processor);
 	if (!*high_speed) {
 		ret = -EIO;
 		goto out;
 	}
 
-	dprintk("high speed is %u\n", *high_speed);
+	*transition_latency = (tv2.tv_sec - tv1.tv_sec) * USEC_PER_SEC +
+		tv2.tv_usec - tv1.tv_usec;
+
+	dprintk("high speed is %u - latency is %u\n",
+			*high_speed, *transition_latency);
 
 	if (*low_speed == *high_speed) {
 		ret = -ENODEV;
 		goto out;
 	}
 
-	/* switch to previous state, if necessary */
-	if (*high_speed != prev_speed)
+	/* switch to previous state, if necessary
+	 * and measure the latency, we want at
+	 * least 2 samples
+	 */
+	if (*high_speed != prev_speed) {
+		do_gettimeofday(&tv1);
 		set_state(SPEEDSTEP_LOW);
+		do_gettimeofday(&tv2);
+		latency = (tv2.tv_sec - tv1.tv_sec) * USEC_PER_SEC +
+			tv2.tv_usec - tv1.tv_usec;
+	}
+
+	/* convert uSec to nSec and add 20% for safety reasons */
+	*transition_latency = MAX(latency, *transition_latency) * 1200;
+
+	/* sanytize the latency measurement if too high or too low
+	 * and ask for reports of out-of-range values
+	 */
+	if (*transition_latency > 10000000 || *transition_latency < 50000) {
+		printk (KERN_ERR "CPUFreq: measured latency in frequency transition out "
+				"of range, please report the following value to "
+				"cpufreq@lists.linux.org.uk\n%u nSec\n", *transition_latency);
+		*transition_latency = CPUFREQ_ETERNAL;
+	}
 
  out:
 	local_irq_restore(flags);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
index 261a2c9..6a727fd 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
@@ -44,4 +44,5 @@ extern unsigned int speedstep_get_proces
 extern unsigned int speedstep_get_freqs(unsigned int processor,
 	  unsigned int *low_speed,
 	  unsigned int *high_speed,
+	  unsigned int *transition_latency,
 	  void (*set_state) (unsigned int state));
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
index 2718fb6..52d7c91 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
@@ -260,15 +260,21 @@ static int speedstep_cpu_init(struct cpu
 		return -EINVAL;
 	}
 
+	/* set default policy values */
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+
 	/* detect low and high frequency */
 	result = speedstep_smi_get_freqs(&speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				&speedstep_freqs[SPEEDSTEP_HIGH].frequency);
+
 	if (result) {
 		/* fall back to speedstep_lib.c dection mechanism: try both states out */
 		dprintk("could not detect low and high frequencies by SMI call.\n");
 		result = speedstep_get_freqs(speedstep_processor,
 				&speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				&speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+			        &policy->cpuinfo.transition_latency,
 				&speedstep_set_state);
 
 		if (result) {
@@ -286,9 +292,6 @@ static int speedstep_cpu_init(struct cpu
 		(speed == speedstep_freqs[SPEEDSTEP_LOW].frequency) ? "low" : "high",
 		(speed / 1000));
 
-	/* cpuinfo and default policy values */
-	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH 2/2] Measure transition latency at driver initialization

[Eric Piel]

30.11.2005 23:30, Mattia Dongili wrote/a écrit:
> Hello Eric!
> 
> On Wed, Nov 30, 2005 at 12:46:25PM +0100, Eric Piel wrote:
> 
>>As stated before, the whole idea is probably good, at least it should 
>>finally get rid of CPUFREQ_ETERNAL transition. BTW, maybe some other 
>>drivers could benefit of the same approach :-)
> 
> 
> grepping around I've just seen that CPUFREQ_ETERNAL is much more in use
> than I expected! It's a shame so few people can benefit from
> ondemand/conservative governors...
Well, finding out the maximum transition is quite difficult (as the 
technical papers usually don't describe it directly). Once we've done 
with speedstep we could try to bother some other drivers, but let's stay 
concentrated ;-)

:
>>* At initialisation, the driver already changes twice of frequency (in 
>>speedstep_get_freqs()). No need to change twice more to measure the 
> 
> 
> I have a patch ready that does it in speedstep_get_freqs but it
> looks ugly, it adds a lot of do_gettimeofay's around... It's probably
> still not optimal and maybe trying to think of it as a more general
> solution (from which all of the drivers could also benefit) should
> result in nicer code.
IMHO, this patch looks already quite good. Granted, the 
speedstep_get_freqs() contains too many do_gettimeofday()s. Why not 
removing all the MAX() stuff and triple measurements and only measure 
set_state(SPEEDSTEP_HIGH) ? It will always do a _real_ transition, and 
we've noticed that transitions from high to low or from low to high are 
similar in term of duration.

> I also had to modify speedstep-smi that still doesn't benefit 100% of
> the times of the feature.
Indeed, only buggy systems will benefit of it, quite weird! Well, let's 
keep the things simple for now. If Venki and/or Dave approve your 
patches, then in a second time, we could try to generalise this idea (to 
all speedstep-smi and even more).


>>transitions! We should put the time measurement inside 
>>speedstep_get_freqs() or inside __speedstep_set_state(). Just check if 
>>transition_latency == CPUFREQ_ETERNAL, if so, do the measurement.
> 
> 
> well, in speedstep_get_freqs the above check is not really necessary, as
> the only current paths reaching that function will return true.
Sure :-)

> 
> Oh, and do you think __speedstep_set_state should remain? The reason I
> added it was to avoid disabling interrupts twice. The attached
> implementation without it is in fact a little less precise (values
> have increased of ~25-30uSec on my PIIIM)
You're right, __speedstep_set_state() is probably not needed. Actually, 
in speedstep_get_freqs(), local_irq_save()/local_irq_restore() could go 
away as all the set_state() functions do it internally.

Concerning the precision, this is probably even better now. What we are 
really interested in is how long the _whole_ latency transition impacts 
the system. So if the transition contains the irq stuff, it should be 
measured too.

c u
Eric

[PATCH 2/2 updated] Measure transition latency at driver initialization

[Mattia Dongili]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

On Thu, Dec 01, 2005 at 12:41:39AM +0100, Eric Piel wrote:
> 30.11.2005 23:30, Mattia Dongili wrote/a écrit:
[...]
> but let's stay 
> concentrated ;-)

Agreed :)

> >>* At initialisation, the driver already changes twice of frequency (in 
> >>speedstep_get_freqs()). No need to change twice more to measure the 
> >
> >
> >I have a patch ready that does it in speedstep_get_freqs but it
> >looks ugly, it adds a lot of do_gettimeofay's around... It's probably
> >still not optimal and maybe trying to think of it as a more general
> >solution (from which all of the drivers could also benefit) should
> >result in nicer code.
> IMHO, this patch looks already quite good. Granted, the 
> speedstep_get_freqs() contains too many do_gettimeofday()s. Why not 
> removing all the MAX() stuff and triple measurements and only measure 
> set_state(SPEEDSTEP_HIGH) ? It will always do a _real_ transition, and 
> we've noticed that transitions from high to low or from low to high are 
> similar in term of duration.

ok, updated patch attached (with a shiny diffstat!!). Looks a lot
nicer now :)

Oh, what do you think of the request to report out-of-range frequencies
and set CPUFREQ_ETERNAL instead of setting an arbitrary high value? If
ok, shall I use my email address instead of the list's?

Last but not least, the patch (as the previous also) is against
2.6.15-rc1-mm2.

Signed-off-by: Mattia Dongili <malattia@linux.it>

 speedstep-ich.c |    4 ++--
 speedstep-lib.c |   31 +++++++++++++++++++++++++++++--
 speedstep-lib.h |    1 +
 speedstep-smi.c |    9 ++++++---
 4 files changed, 38 insertions(+), 7 deletions(-)

-- 
mattia
:wq!

[-- Attachment #2: measure_transition_latency-2.diff --]
[-- Type: text/plain, Size: 5327 bytes --]

diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
index 77efe8e..b425cd3 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
@@ -329,10 +329,11 @@ static int speedstep_cpu_init(struct cpu
 	cpus_allowed = current->cpus_allowed;
 	set_cpus_allowed(current, policy->cpus);
 
-	/* detect low and high frequency */
+	/* detect low and high frequency and transition latency */
 	result = speedstep_get_freqs(speedstep_processor,
 				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+				     &policy->cpuinfo.transition_latency,
 				     &speedstep_set_state);
 	set_cpus_allowed(current, cpus_allowed);
 	if (result)
@@ -349,7 +350,6 @@ static int speedstep_cpu_init(struct cpu
 
 	/* cpuinfo and default policy values */
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
index d368b3f..9acfe46 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
@@ -320,13 +320,16 @@ EXPORT_SYMBOL_GPL(speedstep_detect_proce
 unsigned int speedstep_get_freqs(unsigned int processor,
 				  unsigned int *low_speed,
 				  unsigned int *high_speed,
+				  unsigned int *transition_latency,
 				  void (*set_state) (unsigned int state))
 {
 	unsigned int prev_speed;
 	unsigned int ret = 0;
 	unsigned long flags;
+	struct timeval tv1, tv2;
 
-	if ((!processor) || (!low_speed) || (!high_speed) || (!set_state))
+	if ((!processor) || (!low_speed) || (!high_speed)
+			|| (!set_state) || (!transition_latency))
 		return -EINVAL;
 
 	dprintk("trying to determine both speeds\n");
@@ -337,7 +340,7 @@ unsigned int speedstep_get_freqs(unsigne
 		return -EIO;
 
 	dprintk("previous speed is %u\n", prev_speed);
-	
+
 	local_irq_save(flags);
 
 	/* switch to low state */
@@ -350,8 +353,15 @@ unsigned int speedstep_get_freqs(unsigne
 
 	dprintk("low speed is %u\n", *low_speed);
 
+	/* start latency measurement */
+	do_gettimeofday(&tv1);
+
 	/* switch to high state */
 	set_state(SPEEDSTEP_HIGH);
+
+	/* end latency measurement */
+	do_gettimeofday(&tv2);
+
 	*high_speed = speedstep_get_processor_frequency(processor);
 	if (!*high_speed) {
 		ret = -EIO;
@@ -369,6 +379,23 @@ unsigned int speedstep_get_freqs(unsigne
 	if (*high_speed != prev_speed)
 		set_state(SPEEDSTEP_LOW);
 
+	*transition_latency = (tv2.tv_sec - tv1.tv_sec) * USEC_PER_SEC +
+		tv2.tv_usec - tv1.tv_usec;
+	dprintk("transition latency is %u uSec\n", *transition_latency);
+
+	/* convert uSec to nSec and add 20% for safety reasons */
+	*transition_latency *= 1200;
+
+	/* sanytize the latency measurement if too high or too low
+	 * and ask for reports of out-of-range values
+	 */
+	if (*transition_latency > 10000000 || *transition_latency < 50000) {
+		printk (KERN_ERR "CPUFreq: measured latency of the frequency transition out "
+				"of range, please report the following value to "
+				"cpufreq@lists.linux.org.uk\n%u nSec\n", *transition_latency);
+		*transition_latency = CPUFREQ_ETERNAL;
+	}
+
  out:
 	local_irq_restore(flags);
 	return (ret);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
index 261a2c9..6a727fd 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
@@ -44,4 +44,5 @@ extern unsigned int speedstep_get_proces
 extern unsigned int speedstep_get_freqs(unsigned int processor,
 	  unsigned int *low_speed,
 	  unsigned int *high_speed,
+	  unsigned int *transition_latency,
 	  void (*set_state) (unsigned int state));
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
index 2718fb6..52d7c91 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
@@ -260,15 +260,21 @@ static int speedstep_cpu_init(struct cpu
 		return -EINVAL;
 	}
 
+	/* set default policy values */
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+
 	/* detect low and high frequency */
 	result = speedstep_smi_get_freqs(&speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				&speedstep_freqs[SPEEDSTEP_HIGH].frequency);
+
 	if (result) {
 		/* fall back to speedstep_lib.c dection mechanism: try both states out */
 		dprintk("could not detect low and high frequencies by SMI call.\n");
 		result = speedstep_get_freqs(speedstep_processor,
 				&speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				&speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+			        &policy->cpuinfo.transition_latency,
 				&speedstep_set_state);
 
 		if (result) {
@@ -286,9 +292,6 @@ static int speedstep_cpu_init(struct cpu
 		(speed == speedstep_freqs[SPEEDSTEP_LOW].frequency) ? "low" : "high",
 		(speed / 1000));
 
-	/* cpuinfo and default policy values */
-	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Eric Piel]

01.12.2005 20:31, Mattia Dongili wrote/a écrit:
> ok, updated patch attached (with a shiny diffstat!!). Looks a lot
> nicer now :)
> 
> Oh, what do you think of the request to report out-of-range frequencies
> and set CPUFREQ_ETERNAL instead of setting an arbitrary high value? If
> ok, shall I use my email address instead of the list's?
> 
I was about to say this is fairly reasonable... until I tried it on my 
laptop and notice that _I_ got this message :-( Transition measured: 
4500 nSec. It's probably because the clock source on my laptop is the 
TSC, which on this old processor stops during transition. Therefore, 
after being in the shoe of the poor user still not having the ondemand 
governor working, I'd tend to prefer if the fallback transition latency 
could be a high and safe, but still usable, value. Anyway, we _know_ 
that those processors are not so slugish to do transition that it should 
be considered "eternal" :-)

So what about putting it to 500 uSec, knowing that on my 1GHz PIII it 
took something like 200uSec, it should be safe everywhere:


:
> +	/* sanytize the latency measurement if too high or too low
> +	 * and ask for reports of out-of-range values
> +	 */
> +	if (*transition_latency > 10000000 || *transition_latency < 50000) {
> +		printk (KERN_ERR "CPUFreq: measured latency of the frequency transition out "
> +				"of range, please report the following value to "
> +				"cpufreq@lists.linux.org.uk\n%u nSec\n", *transition_latency);
> +		*transition_latency = CPUFREQ_ETERNAL;
> +	}
It would be something like:
*transition_latency = 500000;
KERN_WARNING "speedstep: frequency transition measured seems out of "
              "range, falling back to a safe one of %u nSec.\n", 
*transition_latency


Eric

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Mattia Dongili]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

On Thu, Dec 01, 2005 at 10:05:31PM +0100, Eric Piel wrote:
> 01.12.2005 20:31, Mattia Dongili wrote/a écrit:
> >ok, updated patch attached (with a shiny diffstat!!). Looks a lot
> >nicer now :)
> >
> >Oh, what do you think of the request to report out-of-range frequencies
> >and set CPUFREQ_ETERNAL instead of setting an arbitrary high value? If
> >ok, shall I use my email address instead of the list's?
> >
> I was about to say this is fairly reasonable... until I tried it on my 
> laptop and notice that _I_ got this message :-( Transition measured: 
> 4500 nSec. It's probably because the clock source on my laptop is the 
> TSC, which on this old processor stops during transition. Therefore, 

Hehe, oh that was the reason you tried pmtimer measurement, right? :)

> So what about putting it to 500 uSec, knowing that on my 1GHz PIII it 
> took something like 200uSec, it should be safe everywhere:

Fine with me. Following your suggested code, I only changed the order
of the assigment and the printk because it might still be interesting to
see the measured value without having to enable cpufreq_debug.

So, to summarize:
the attached patch introduces runtime latency measurement for many
speedstep based processors instead of using CPUFREQ_ETERNAL. It includes
some sanity checks in case the measured value is out of range and
assigns a safe value of 500uSec that should still be enough on
problematics chipsets (current testing report values ~200uSec).

Signed-off-by: Mattia Dongili <malattia@linux.it>
-- 
mattia
:wq!

[-- Attachment #2: measure_transition_latency-3.diff --]
[-- Type: text/plain, Size: 5303 bytes --]

diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
index 77efe8e..b425cd3 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
@@ -329,10 +329,11 @@ static int speedstep_cpu_init(struct cpu
 	cpus_allowed = current->cpus_allowed;
 	set_cpus_allowed(current, policy->cpus);
 
-	/* detect low and high frequency */
+	/* detect low and high frequency and transition latency */
 	result = speedstep_get_freqs(speedstep_processor,
 				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+				     &policy->cpuinfo.transition_latency,
 				     &speedstep_set_state);
 	set_cpus_allowed(current, cpus_allowed);
 	if (result)
@@ -349,7 +350,6 @@ static int speedstep_cpu_init(struct cpu
 
 	/* cpuinfo and default policy values */
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
index d368b3f..cfefc37 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
@@ -320,13 +320,16 @@ EXPORT_SYMBOL_GPL(speedstep_detect_proce
 unsigned int speedstep_get_freqs(unsigned int processor,
 				  unsigned int *low_speed,
 				  unsigned int *high_speed,
+				  unsigned int *transition_latency,
 				  void (*set_state) (unsigned int state))
 {
 	unsigned int prev_speed;
 	unsigned int ret = 0;
 	unsigned long flags;
+	struct timeval tv1, tv2;
 
-	if ((!processor) || (!low_speed) || (!high_speed) || (!set_state))
+	if ((!processor) || (!low_speed) || (!high_speed)
+			|| (!set_state) || (!transition_latency))
 		return -EINVAL;
 
 	dprintk("trying to determine both speeds\n");
@@ -337,7 +340,7 @@ unsigned int speedstep_get_freqs(unsigne
 		return -EIO;
 
 	dprintk("previous speed is %u\n", prev_speed);
-	
+
 	local_irq_save(flags);
 
 	/* switch to low state */
@@ -350,8 +353,15 @@ unsigned int speedstep_get_freqs(unsigne
 
 	dprintk("low speed is %u\n", *low_speed);
 
+	/* start latency measurement */
+	do_gettimeofday(&tv1);
+
 	/* switch to high state */
 	set_state(SPEEDSTEP_HIGH);
+
+	/* end latency measurement */
+	do_gettimeofday(&tv2);
+
 	*high_speed = speedstep_get_processor_frequency(processor);
 	if (!*high_speed) {
 		ret = -EIO;
@@ -369,6 +379,23 @@ unsigned int speedstep_get_freqs(unsigne
 	if (*high_speed != prev_speed)
 		set_state(SPEEDSTEP_LOW);
 
+	*transition_latency = (tv2.tv_sec - tv1.tv_sec) * USEC_PER_SEC +
+		tv2.tv_usec - tv1.tv_usec;
+	dprintk("transition latency is %u uSec\n", *transition_latency);
+
+	/* convert uSec to nSec and add 20% for safety reasons */
+	*transition_latency *= 1200;
+
+	/* check if the latency measurement is too high or too low
+	 * and set it to a safe value (500uSec) in that case
+	 */
+	if (*transition_latency > 10000000 || *transition_latency < 50000) {
+		printk (KERN_WARNING "speedstep: frequency transition measured seems out of "
+				"range (%u nSec), falling back to a safe one of %u nSec.\n",
+				*transition_latency, 500000);
+		*transition_latency = 500000;
+	}
+
  out:
 	local_irq_restore(flags);
 	return (ret);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
index 261a2c9..6a727fd 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
@@ -44,4 +44,5 @@ extern unsigned int speedstep_get_proces
 extern unsigned int speedstep_get_freqs(unsigned int processor,
 	  unsigned int *low_speed,
 	  unsigned int *high_speed,
+	  unsigned int *transition_latency,
 	  void (*set_state) (unsigned int state));
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
index 2718fb6..52d7c91 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
@@ -260,15 +260,21 @@ static int speedstep_cpu_init(struct cpu
 		return -EINVAL;
 	}
 
+	/* set default policy values */
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
+
 	/* detect low and high frequency */
 	result = speedstep_smi_get_freqs(&speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				&speedstep_freqs[SPEEDSTEP_HIGH].frequency);
+
 	if (result) {
 		/* fall back to speedstep_lib.c dection mechanism: try both states out */
 		dprintk("could not detect low and high frequencies by SMI call.\n");
 		result = speedstep_get_freqs(speedstep_processor,
 				&speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				&speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+			        &policy->cpuinfo.transition_latency,
 				&speedstep_set_state);
 
 		if (result) {
@@ -286,9 +292,6 @@ static int speedstep_cpu_init(struct cpu
 		(speed == speedstep_freqs[SPEEDSTEP_LOW].frequency) ? "low" : "high",
 		(speed / 1000));
 
-	/* cpuinfo and default policy values */
-	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Eric Piel]

12/02/2005 12:34 AM, Mattia Dongili wrote/a écrit:
> Fine with me. Following your suggested code, I only changed the order
> of the assigment and the printk because it might still be interesting to
> see the measured value without having to enable cpufreq_debug.
Yes, that's a good idea :-)

> 
> So, to summarize:
> the attached patch introduces runtime latency measurement for many
> speedstep based processors instead of using CPUFREQ_ETERNAL. It includes
> some sanity checks in case the measured value is out of range and
> assigns a safe value of 500uSec that should still be enough on
> problematics chipsets (current testing report values ~200uSec).
Again, I though it was done... and then Ville showed that's it's plainly 
wrong to assume anything with speedstep-smi: it could take up to 1 
second to change of frequency or few uSec. So we should probably back 
out the smi part.

What I propose: modify slightly the interface of speedstep_get_freqs(): 
  * if transition_latency is NULL, we don't try to update the transition 
latency,
* otherwise we so as usual.
Then, in speedstep-smi we can call speedstep_get_freqs() with NULL, 
voila :-)

Later we could change the transition latency of speedstep-smi from 
ETERNAL to 1s... but that's not going to help much anyway for the 
ondemand governor.

Eric

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Mattia Dongili]

On Fri, December 2, 2005 12:37 pm, Eric Piel said:
> 12/02/2005 12:34 AM, Mattia Dongili wrote/a écrit:
[...]
>> problematics chipsets (current testing report values ~200uSec).
> Again, I though it was done... and then Ville showed that's it's plainly
> wrong to assume anything with speedstep-smi: it could take up to 1
> second to change of frequency or few uSec. So we should probably back
> out the smi part.
>
> What I propose: modify slightly the interface of speedstep_get_freqs():
>   * if transition_latency is NULL, we don't try to update the transition
> latency,
> * otherwise we so as usual.
> Then, in speedstep-smi we can call speedstep_get_freqs() with NULL,
> voila :-)

Ok, I had something similar in mind when thinking about creating a more
general function/interface for latency measurement. We will surely need
some way to disable it.

Updated patch will follow soon.
-- 
mattia
:wq!

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Mattia Dongili]

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

On Fri, Dec 02, 2005 at 02:34:35PM +0100, Mattia Dongili wrote:
> On Fri, December 2, 2005 12:37 pm, Eric Piel said:
> > 12/02/2005 12:34 AM, Mattia Dongili wrote/a écrit:
> [...]
> >> problematics chipsets (current testing report values ~200uSec).
> > Again, I though it was done... and then Ville showed that's it's plainly
> > wrong to assume anything with speedstep-smi: it could take up to 1
> > second to change of frequency or few uSec. So we should probably back
> > out the smi part.
> >
> > What I propose: modify slightly the interface of speedstep_get_freqs():
> >   * if transition_latency is NULL, we don't try to update the transition
> > latency,
> > * otherwise we so as usual.
> > Then, in speedstep-smi we can call speedstep_get_freqs() with NULL,
> > voila :-)
> 
> Ok, I had something similar in mind when thinking about creating a more
> general function/interface for latency measurement. We will surely need
> some way to disable it.
>
> Updated patch will follow soon.

here it is:
the attached patch introduces runtime latency measurement for ICH[234]
based chipsets instead of using CPUFREQ_ETERNAL. It includes
some sanity checks in case the measured value is out of range and
assigns a safe value of 500uSec that should still be enough on
problematics chipsets (current testing report values ~200uSec). The
measurement is currently done in speedstep_get_freqs in order to avoid
further unnecessary transitions and in the hope it'll come handy for SMI
also.

Signed-off-by: Mattia Dongili <malattia@linux.it>

 speedstep-ich.c |    4 ++--
 speedstep-lib.c |   32 +++++++++++++++++++++++++++++++-
 speedstep-lib.h |    1 +
 speedstep-smi.c |    1 +
 4 files changed, 35 insertions(+), 3 deletions(-)

-- 
mattia
:wq!

[-- Attachment #2: measure_transition_latency-4.diff --]
[-- Type: text/plain, Size: 4323 bytes --]

diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
index 77efe8e..b425cd3 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
@@ -329,10 +329,11 @@ static int speedstep_cpu_init(struct cpu
 	cpus_allowed = current->cpus_allowed;
 	set_cpus_allowed(current, policy->cpus);
 
-	/* detect low and high frequency */
+	/* detect low and high frequency and transition latency */
 	result = speedstep_get_freqs(speedstep_processor,
 				     &speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				     &speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+				     &policy->cpuinfo.transition_latency,
 				     &speedstep_set_state);
 	set_cpus_allowed(current, cpus_allowed);
 	if (result)
@@ -349,7 +350,6 @@ static int speedstep_cpu_init(struct cpu
 
 	/* cpuinfo and default policy values */
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
-	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	policy->cur = speed;
 
 	result = cpufreq_frequency_table_cpuinfo(policy, speedstep_freqs);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
index d368b3f..7c47005 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.c
@@ -320,11 +320,13 @@ EXPORT_SYMBOL_GPL(speedstep_detect_proce
 unsigned int speedstep_get_freqs(unsigned int processor,
 				  unsigned int *low_speed,
 				  unsigned int *high_speed,
+				  unsigned int *transition_latency,
 				  void (*set_state) (unsigned int state))
 {
 	unsigned int prev_speed;
 	unsigned int ret = 0;
 	unsigned long flags;
+	struct timeval tv1, tv2;
 
 	if ((!processor) || (!low_speed) || (!high_speed) || (!set_state))
 		return -EINVAL;
@@ -337,7 +339,7 @@ unsigned int speedstep_get_freqs(unsigne
 		return -EIO;
 
 	dprintk("previous speed is %u\n", prev_speed);
-	
+
 	local_irq_save(flags);
 
 	/* switch to low state */
@@ -350,8 +352,17 @@ unsigned int speedstep_get_freqs(unsigne
 
 	dprintk("low speed is %u\n", *low_speed);
 
+	/* start latency measurement */
+	if (transition_latency)
+		do_gettimeofday(&tv1);
+
 	/* switch to high state */
 	set_state(SPEEDSTEP_HIGH);
+
+	/* end latency measurement */
+	if (transition_latency)
+		do_gettimeofday(&tv2);
+
 	*high_speed = speedstep_get_processor_frequency(processor);
 	if (!*high_speed) {
 		ret = -EIO;
@@ -369,6 +380,25 @@ unsigned int speedstep_get_freqs(unsigne
 	if (*high_speed != prev_speed)
 		set_state(SPEEDSTEP_LOW);
 
+	if (transition_latency) {
+		*transition_latency = (tv2.tv_sec - tv1.tv_sec) * USEC_PER_SEC +
+			tv2.tv_usec - tv1.tv_usec;
+		dprintk("transition latency is %u uSec\n", *transition_latency);
+
+		/* convert uSec to nSec and add 20% for safety reasons */
+		*transition_latency *= 1200;
+
+		/* check if the latency measurement is too high or too low
+		 * and set it to a safe value (500uSec) in that case
+		 */
+		if (*transition_latency > 10000000 || *transition_latency < 50000) {
+			printk (KERN_WARNING "speedstep: frequency transition measured seems out of "
+					"range (%u nSec), falling back to a safe one of %u nSec.\n",
+					*transition_latency, 500000);
+			*transition_latency = 500000;
+		}
+	}
+
  out:
 	local_irq_restore(flags);
 	return (ret);
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
index 261a2c9..6a727fd 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-lib.h
@@ -44,4 +44,5 @@ extern unsigned int speedstep_get_proces
 extern unsigned int speedstep_get_freqs(unsigned int processor,
 	  unsigned int *low_speed,
 	  unsigned int *high_speed,
+	  unsigned int *transition_latency,
 	  void (*set_state) (unsigned int state));
diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
index 2718fb6..28cc5d5 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-smi.c
@@ -269,6 +269,7 @@ static int speedstep_cpu_init(struct cpu
 		result = speedstep_get_freqs(speedstep_processor,
 				&speedstep_freqs[SPEEDSTEP_LOW].frequency,
 				&speedstep_freqs[SPEEDSTEP_HIGH].frequency,
+				NULL,
 				&speedstep_set_state);
 
 		if (result) {

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Eric Piel]

02.12.2005 21:59, Mattia Dongili wrote/a écrit:

> here it is:
> the attached patch introduces runtime latency measurement for ICH[234]
> based chipsets instead of using CPUFREQ_ETERNAL. It includes
> some sanity checks in case the measured value is out of range and
> assigns a safe value of 500uSec that should still be enough on
> problematics chipsets (current testing report values ~200uSec). The
> measurement is currently done in speedstep_get_freqs in order to avoid
> further unnecessary transitions and in the hope it'll come handy for SMI
> also.

It looks fine for me :-) Dave, Venki, do you have any objections? Please 
apply those two patchs to finally get over the transition latency 
problem on speedstep-ich.

cheers,
Eric

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Dominik Brodowski]

On Fri, Dec 02, 2005 at 09:59:41PM +0100, Mattia Dongili wrote:
> the attached patch introduces runtime latency measurement for ICH[234]
> based chipsets instead of using CPUFREQ_ETERNAL. It includes
> some sanity checks in case the measured value is out of range and
> assigns a safe value of 500uSec that should still be enough on
> problematics chipsets (current testing report values ~200uSec). The
> measurement is currently done in speedstep_get_freqs in order to avoid
> further unnecessary transitions and in the hope it'll come handy for SMI
> also.
> 
> Signed-off-by: Mattia Dongili <malattia@linux.it>
Acked-by: Dominik Brodowski <linux@dominikbrodowski.net>

Re: [PATCH 2/2 updated] Measure transition latency at driver initialization

[Ville Syrjälä]

On Thu, Dec 01, 2005 at 10:05:31PM +0100, Eric Piel wrote:
> 01.12.2005 20:31, Mattia Dongili wrote/a écrit:
> >ok, updated patch attached (with a shiny diffstat!!). Looks a lot
> >nicer now :)
> >
> >Oh, what do you think of the request to report out-of-range frequencies
> >and set CPUFREQ_ETERNAL instead of setting an arbitrary high value? If
> >ok, shall I use my email address instead of the list's?
> >
> I was about to say this is fairly reasonable... until I tried it on my 
> laptop and notice that _I_ got this message :-( Transition measured: 
> 4500 nSec. It's probably because the clock source on my laptop is the 
> TSC, which on this old processor stops during transition. Therefore, 
> after being in the shoe of the poor user still not having the ondemand 
> governor working, I'd tend to prefer if the fallback transition latency 
> could be a high and safe, but still usable, value. Anyway, we _know_ 
> that those processors are not so slugish to do transition that it should 
> be considered "eternal" :-)
> 
> So what about putting it to 500 uSec, knowing that on my 1GHz PIII it 
> took something like 200uSec, it should be safe everywhere:

I'm going to jump in here with some measurements I got with 
do_gettimeofday().

speedstep-ich on my 933 MHz Tualatin seems to agree with the 200 usec 
figure.

speedstep-smi takes ~1670 usec on my 700 MHz Coppermine. That is when it 
succeeds to change the frequency on the first try. That fails quite 
often and with one retry the time goes up to ~72 msec (thanks to the 
mdelay() in the loop). It will try to change the frequency six times 
before giving up and at that point it may have wasted over one second. 
Isn't that a "bit" too long to spend with irqs disabled?

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [PATCH 1/2] Move PMBASE reading away and do it only once at initialization time

[Eric Piel]

IMHO, this looks very fine :-)


11/30/2005 12:58 AM, Mattia Dongili wrote/a écrit:
> Signed-off-by: Mattia Dongili <malattia@linux.it>
> 
> ---
> 
>  arch/i386/kernel/cpu/cpufreq/speedstep-ich.c |   45 ++++++++++++++++++--------
>  1 files changed, 32 insertions(+), 13 deletions(-)
> 
> applies-to: 41c86716d1584bad6ddf55c9132ef8b820bc1b15
> 31c370b9ba20188340635fd7cebc150c4b71b097
> diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
> index 5b7d18a..17b7864 100644
> --- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
> +++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
:
> @@ -400,6 +415,10 @@ static int __init speedstep_init(void)
>  		return -EINVAL;
>  	}
>  
> +	if (speedstep_find_register()) {
> +		return -ENODEV;
> +	}
> +
Please, keep it simple and do put { and } when there is only one line.


>  	return cpufreq_register_driver(&speedstep_driver);
>  }
>  

Re: [PATCH 1/2] Move PMBASE reading away and do it only once at initialization time

[Mattia Dongili]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Wed, Nov 30, 2005 at 12:02:59PM +0100, Eric Piel wrote:
[...]
> >@@ -400,6 +415,10 @@ static int __init speedstep_init(void)
> > 		return -EINVAL;
> > 	}
> > 
> >+	if (speedstep_find_register()) {
> >+		return -ENODEV;
> >+	}
> >+
> Please, keep it simple and do put { and } when there is only one line.

I assume you meant "do not put" :)
attached the fixed patch (with removed extra white space also).

This patch moves away PMBASE reading and only performs it at
cpufreq_register_driver time by exiting with -ENODEV if unable to read                                                                                
the value.

Signed-off-by: Mattia Dongili <malattia@linux.it>
-- 
mattia
:wq!

[-- Attachment #2: move_pmbase_reading.diff --]
[-- Type: text/plain, Size: 2063 bytes --]

diff --git a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
index 5b7d18a..77efe8e 100644
--- a/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
+++ b/arch/i386/kernel/cpu/cpufreq/speedstep-ich.c
@@ -40,6 +40,7 @@ static struct pci_dev *speedstep_chipset
  */
 static unsigned int speedstep_processor = 0;
 
+static u32 pmbase;
 
 /*
  *   There are only two frequency states for each processor. Values
@@ -56,34 +57,47 @@ static struct cpufreq_frequency_table sp
 
 
 /**
- * speedstep_set_state - set the SpeedStep state
- * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
+ * speedstep_find_register - read the PMBASE address
  *
- *   Tries to change the SpeedStep state.
+ * Returns: -ENODEV if no register could be found
  */
-static void speedstep_set_state (unsigned int state)
+static int speedstep_find_register (void)
 {
-	u32 pmbase;
-	u8 pm2_blk;
-	u8 value;
-	unsigned long flags;
-
-	if (!speedstep_chipset_dev || (state > 0x1))
-		return;
+	if (!speedstep_chipset_dev)
+		return -ENODEV;
 
 	/* get PMBASE */
 	pci_read_config_dword(speedstep_chipset_dev, 0x40, &pmbase);
 	if (!(pmbase & 0x01)) {
 		printk(KERN_ERR "speedstep-ich: could not find speedstep register\n");
-		return;
+		return -ENODEV;
 	}
 
 	pmbase &= 0xFFFFFFFE;
 	if (!pmbase) {
 		printk(KERN_ERR "speedstep-ich: could not find speedstep register\n");
-		return;
+		return -ENODEV;
 	}
 
+	dprintk("pmbase is 0x%x\n", pmbase);
+	return 0;
+}
+
+/**
+ * speedstep_set_state - set the SpeedStep state
+ * @state: new processor frequency state (SPEEDSTEP_LOW or SPEEDSTEP_HIGH)
+ *
+ *   Tries to change the SpeedStep state.
+ */
+static void speedstep_set_state (unsigned int state)
+{
+	u8 pm2_blk;
+	u8 value;
+	unsigned long flags;
+
+	if (state > 0x1)
+		return;
+
 	/* Disable IRQs */
 	local_irq_save(flags);
 
@@ -400,6 +414,9 @@ static int __init speedstep_init(void)
 		return -EINVAL;
 	}
 
+	if (speedstep_find_register())
+		return -ENODEV;
+
 	return cpufreq_register_driver(&speedstep_driver);
 }
 

[-- Attachment #3: Type: text/plain, Size: 147 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@lists.linux.org.uk
http://lists.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH 1/2] Move PMBASE reading away and do it only once at initialization time

[Dominik Brodowski]

On Wed, Nov 30, 2005 at 10:00:59PM +0100, Mattia Dongili wrote:
> On Wed, Nov 30, 2005 at 12:02:59PM +0100, Eric Piel wrote:
> [...]
> > >@@ -400,6 +415,10 @@ static int __init speedstep_init(void)
> > > 		return -EINVAL;
> > > 	}
> > > 
> > >+	if (speedstep_find_register()) {
> > >+		return -ENODEV;
> > >+	}
> > >+
> > Please, keep it simple and do put { and } when there is only one line.
> 
> I assume you meant "do not put" :)
> attached the fixed patch (with removed extra white space also).
> 
> This patch moves away PMBASE reading and only performs it at
> cpufreq_register_driver time by exiting with -ENODEV if unable to read                                                                                
> the value.
> 
> Signed-off-by: Mattia Dongili <malattia@linux.it>
Acked-by: Dominik Brodowski <linux@dominikbrodowski.net>

RE: [PATCH 2/2] Measure transition latency at driver initialization

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: cpufreq-bounces@lists.linux.org.uk 
>[mailto:cpufreq-bounces@lists.linux.org.uk] On Behalf Of Eric Piel
>30.11.2005 23:30, Mattia Dongili wrote/a écrit:
>
>> I also had to modify speedstep-smi that still doesn't benefit 100% of
>> the times of the feature.
>Indeed, only buggy systems will benefit of it, quite weird! 
>Well, let's 
>keep the things simple for now. If Venki and/or Dave approve your 
>patches, then in a second time, we could try to generalise 
>this idea (to 
>all speedstep-smi and even more).
>

I am all for "OS measuring the latency itself". Infact, the idea can be extended to acpi-cpufreq driver as well, where currently BIOS reports the latency and more often than not, it reports the latency wrongly. OK. OK. Going by the Eric's words, we need to stay concentrated here. :)

Thanks,
Venki