powerd doesn't decrease CPU frequency in some cases
Takeharu KATO
takeharu1219 at ybb.ne.jp
Fri Jan 4 19:18:24 PST 2008
Hi,
I met same problem on my Panasonic CF-R7 note book PC.
As far as I investigate, cpufreq(est driver) does not
check setting values for PERFCTL register which are reported
from acpi_perf driver.
According to comments in sys/i386/cpufreq/est.c,
the auther of the driver regards this as TODO thing
(see following lines.).
-- sys/i386/cpufreq/est.c
for (i = 0; i < count; i++) {
/*
* TODO: Figure out validity checks for id16. Linux checks
* that the control and status values match.
*/
--
I wrote the patch to check id16 values as comments says.
As far as I test the patch, the patch fixes the problem.
Would you try this patch?
P.S. I've send-pr this problem with this patch in this morning(kern/119350).
Andrey wrote:
> Good time of the day.
>
> I've noticed that powerd isn't able to decrease CPU frequency on my
> laptop (HP Compaq 6710b) as soon as frequency gets highest level.
>
> I've pottered a bit in the sources and it seems found the root of the
> issue.
> So those who are interested in the subject let consider it.
>
> For instance my system reports the following frequency levels:
>
> [silent at beastie][/home/silent]sysctl dev.cpu.0.freq_levels
> dev.cpu.0.freq_levels: 2001/35000 2000/35000 1750/30625 1600/25000
> 1400/21875 1200/16000 1050/14000 900/12000 800/14000 700/12250 600/10500
> 500/8750 400/7000 300/5250
>
> If I try to adjust current frequency to 2000 MHz then I'll get:
> [silent at beastie][/home/silent]sudo sysctl dev.cpu.0.freq=2000
> dev.cpu.0.freq: 300 -> 2001
> Let check:
> [silent at beastie][/home/silent]sysctl dev.cpu.0.freq
> dev.cpu.0.freq: 2001
>
> Thus, as you can see, I have level "2000" which system reports me but
> actually I can't to adjust those one exactly because it silently becomes
> "2001"
>
> Well... If I'm not mistaken powerd calculates the current "CPU idle
> mark" and if it is more then adopted value (by default 90%) then it
> shifts CPU frequency value 1 step down. In my case powerd sticks at
> "2001". It is obvious because when powerd decreases CPU frequency from
> the highest frequency level we'll get the following scenario:
>
> +-----------------------+
> | dev.cpu.0.freq=2001 +--<-+
> +-----------+-----------+ |
> | |
> Y |
> +-----------+-----------+ |
> | "CPU idle" > 90% | |
> +-----------+-----------+ |
> | |
> Y ^
> +-----------+-----------+ ^
> | powerd shifts freq. | ^
> | 1 step down: | |
> | "2001" -> "2000" | |
> +-----------+-----------+ |
> | |
> Y |
> +-----------+-----------+ |
> | actually we have here | |
> | dev.cpu.0.freq == 2001+-->-+
> +-----------------------+
>
>
> According to the things mentioned above I've came to conclusion that in
> my case it is not a good idea to rely on frequency levels reported by
> the system. (Also I saw many sysctl mibs (dev.cpu.0.freq) of many other
> people. And there were "strange" frequency levels like "2000" and
> "2001". Of course I can't state that their systems' behaviors fit my
> case. But still...)
>
> So the simple way out I see is to teach powerd recognize "fake"
> frequency levels. Here I suggest a very simple workaround (and may be
> quite ugly... sorry I'm not sure if it is my cup of tee) which allows me
> to overcome the issue. And I hope it can be useful for smb. else.
>
> Also I'd like to hear opinions of others. May be there exists another
> and simpler way to overcome an issue or even I've missed something or
> not aware of something.
>
>
> Thank you.
>
>
> --
> Sincerely,
> Andrey Kosachenko
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> freebsd-acpi at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
> To unsubscribe, send any mail to "freebsd-acpi-unsubscribe at freebsd.org"
-------------- next part --------------
--- sys.orig/i386/cpufreq/est.c 2006-05-11 17:35:44.000000000 +0000
+++ sys/i386/cpufreq/est.c 2008-01-05 10:56:14.435882213 +0000
@@ -902,6 +902,8 @@
static int est_set(device_t dev, const struct cf_setting *set);
static int est_get(device_t dev, struct cf_setting *set);
static int est_type(device_t dev, int *type);
+static int est_set_id16(uint16_t id16, int need_check);
+static void est_get_id16(uint16_t *id16_p);
static device_method_t est_methods[] = {
/* Device interface */
@@ -1068,7 +1070,6 @@
return (0);
}
-
static int
est_acpi_info(device_t dev, freq_info **freqs)
{
@@ -1076,7 +1077,8 @@
struct cf_setting *sets;
freq_info *table;
device_t perf_dev;
- int count, error, i;
+ int count, error, i, idx;
+ uint16_t saved_id16;
perf_dev = device_find_child(device_get_parent(dev), "acpi_perf", -1);
if (perf_dev == NULL || !device_is_attached(perf_dev))
@@ -1098,19 +1100,38 @@
error = ENOMEM;
goto out;
}
- for (i = 0; i < count; i++) {
+ for (i = 0, idx = 0; i < count; i++) {
/*
- * TODO: Figure out validity checks for id16. Linux checks
- * that the control and status values match.
+ * Confirm id16 value is correct.
*/
- table[i].freq = sets[i].freq;
- table[i].volts = sets[i].volts;
- table[i].id16 = sets[i].spec[0];
- table[i].power = sets[i].power;
+ if (sets[i].freq > 0) {
+
+ /* save current value */
+ est_get_id16(&saved_id16);
+
+ /*
+ * Try to set specified value
+ */
+ error = est_set_id16(sets[i].spec[0], 1);
+ if (error == 0) {
+ if (bootverbose)
+ printf("Invalid freq %u, ignored.\n",
+ sets[i].spec[0]);
+ } else {
+ table[idx].freq = sets[i].freq;
+ table[idx].volts = sets[i].volts;
+ table[idx].id16 = sets[i].spec[0];
+ table[idx].power = sets[i].power;
+ ++idx;
+ }
+
+ /* restore saved setting */
+ est_set_id16(sets[i].spec[0], 0);
+ }
}
/* Mark end of table with a terminator. */
- bzero(&table[i], sizeof(freq_info));
+ bzero(&table[idx], sizeof(freq_info));
sc->acpi_settings = TRUE;
*freqs = table;
@@ -1148,6 +1169,41 @@
*freqs = p->freqtab;
return (0);
}
+static void
+est_get_id16(uint16_t *id16_p) {
+ *id16_p = rdmsr(MSR_PERF_STATUS) & 0xffff;
+}
+
+static int
+est_set_id16(uint16_t id16, int need_check) {
+ uint64_t msr;
+ uint16_t new_id16;
+ int rc = 0;
+
+ /*
+ * Try to set freq.
+ */
+
+ /* Read the current register, mask out the old, set the new id. */
+ msr = rdmsr(MSR_PERF_CTL);
+ msr = (msr & ~0xffff) | id16;
+ wrmsr(MSR_PERF_CTL, msr);
+
+ /* Wait a short while for the new setting. XXX Is this necessary? */
+ DELAY(EST_TRANS_LAT);
+
+ if (need_check) {
+ est_get_id16(&new_id16);
+ if (new_id16 != id16) {
+ if (bootverbose)
+ printf("Invalid id16 (set, cur) = (%u, %u)\n",
+ id16, new_id16);
+ rc = ENXIO; /* Can not set this value */
+ }
+ }
+
+ return (rc);
+}
static freq_info *
est_get_current(freq_info *freq_list)
@@ -1162,7 +1218,7 @@
* we get a temporary invalid result.
*/
for (i = 0; i < 5; i++) {
- id16 = rdmsr(MSR_PERF_STATUS) & 0xffff;
+ est_get_id16(&id16);
for (f = freq_list; f->id16 != 0; f++) {
if (f->id16 == id16)
return (f);
@@ -1201,7 +1257,6 @@
{
struct est_softc *sc;
freq_info *f;
- uint64_t msr;
/* Find the setting matching the requested one. */
sc = device_get_softc(dev);
@@ -1213,9 +1268,7 @@
return (EINVAL);
/* Read the current register, mask out the old, set the new id. */
- msr = rdmsr(MSR_PERF_CTL);
- msr = (msr & ~0xffff) | f->id16;
- wrmsr(MSR_PERF_CTL, msr);
+ est_set_id16(f->id16, 0);
/* Wait a short while for the new setting. XXX Is this necessary? */
DELAY(EST_TRANS_LAT);
More information about the freebsd-acpi
mailing list