On my laptop (Dell XPS 13 9333) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is 0 when AC is present, 1 otherwise. Because of this, setting turbo_disabled on init leads to some issues: * If I turn on the laptop while the AC is present, the sysfs interface always allows me to change the state of no_turbo. However, the changes are only effective when the AC is present. * If I turn on the laptop while on battery, the sysfs interface doesn't allow me to change the state of no_turbo. That means I'm not able to disable it when it's automatically enabled when I plug the AC. Note that I don't have the possibility to disable Intel Turbo from the BIOS settings.
I also noticed that because of the changes of the turbo state not expected by intel_pstate, max_perf is sometimes wrong. The patch should fix my problem. I don't know if there's a way to avoid reading the register every time or if there's a better way to deal with my problem. diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index c5eac94..b8adfec 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -218,6 +218,13 @@ static inline void intel_pstate_reset_all_pid(void) } } +static int turbo_disabled(void) { + u64 misc_en; + rdmsrl(MSR_IA32_MISC_ENABLE, misc_en); + return (limits.turbo_disabled || + (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE)); +} + /************************** debugfs begin ************************/ static int pid_param_set(void *data, u64 val) { @@ -284,10 +291,8 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b, if (ret != 1) return -EINVAL; limits.no_turbo = clamp_t(int, input, 0 , 1); - if (limits.turbo_disabled) { + if (turbo_disabled()) pr_warn("Turbo disabled by BIOS or unavailable on processor\n"); - limits.no_turbo = limits.turbo_disabled; - } return count; } @@ -323,6 +328,12 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b, return count; } +static ssize_t show_actual_no_turbo(struct kobject *kobj, + struct attribute *attr, char *buf) +{ + return sprintf(buf, "%u\n", limits.no_turbo || turbo_disabled()); +} + show_one(no_turbo, no_turbo); show_one(max_perf_pct, max_perf_pct); show_one(min_perf_pct, min_perf_pct); @@ -330,11 +341,13 @@ show_one(min_perf_pct, min_perf_pct); define_one_global_rw(no_turbo); define_one_global_rw(max_perf_pct); define_one_global_rw(min_perf_pct); +define_one_global_ro(actual_no_turbo); static struct attribute *intel_pstate_attributes[] = { &no_turbo.attr, &max_perf_pct.attr, &min_perf_pct.attr, + &actual_no_turbo.attr, NULL }; @@ -386,7 +399,7 @@ static void byt_set_pstate(struct cpudata *cpudata, int pstate) u32 vid; val = pstate << 8; - if (limits.no_turbo && !limits.turbo_disabled) + if (limits.no_turbo && !turbo_disabled()) val |= (u64)1 << 32; vid_fp = cpudata->vid.min + mul_fp( @@ -454,7 +467,7 @@ static void core_set_pstate(struct cpudata *cpudata, int pstate) u64 val; val = pstate << 8; - if (limits.no_turbo && !limits.turbo_disabled) + if (limits.no_turbo && !turbo_disabled()) val |= (u64)1 << 32; wrmsrl_on_cpu(cpudata->cpu, MSR_IA32_PERF_CTL, val); @@ -501,7 +514,7 @@ static void intel_pstate_get_min_max(struct cpudata *cpu, int *min, int *max) int max_perf_adj; int min_perf; - if (limits.no_turbo) + if (limits.no_turbo || turbo_disabled()) max_perf = cpu->pstate.max_pstate; max_perf_adj = fp_toint(mul_fp(int_tofp(max_perf), limits.max_perf)); @@ -719,7 +732,6 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy) limits.min_perf = int_tofp(1); limits.max_perf_pct = 100; limits.max_perf = int_tofp(1); - limits.no_turbo = limits.turbo_disabled; return 0; } limits.min_perf_pct = (policy->min * 100) / policy->cpuinfo.max_freq; @@ -762,7 +774,6 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy) { struct cpudata *cpu; int rc; - u64 misc_en; rc = intel_pstate_init_cpu(policy->cpu); if (rc) @@ -770,12 +781,8 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy) cpu = all_cpu_data[policy->cpu]; - rdmsrl(MSR_IA32_MISC_ENABLE, misc_en); - if (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE || - cpu->pstate.max_pstate == cpu->pstate.turbo_pstate) { + if (cpu->pstate.max_pstate == cpu->pstate.turbo_pstate) limits.turbo_disabled = 1; - limits.no_turbo = 1; - } if (limits.min_perf_pct == 100 && limits.max_perf_pct == 100) policy->policy = CPUFREQ_POLICY_PERFORMANCE; else
I am just back from vacation give me a couple of days to dig out of the inbox to review/test
Created attachment 148771 [details] Allow modification of no_turbo sysfs file based on current state of MSR_IA32_MISC_ENABLE_TURBO_DISABLE Can you try the attached patch? I do not have a system that dynamically sets MSR_IA32_MISC_ENABLE_TURBO_DISABLE
It doesn't work correctly. If the turbo is disabled by the BIOS and then I try to do something with no_turbo, it will no longer be possible for me to do any change as nothing sets turbo_disabled back to 0. I modified it, but still doesn't work perfectly. I think that the problem is that max_perf depends on limits.no_turbo which is updated when the user tries to access it through the sysfs interface. With the current patch, this is a problem when the AC is present and the turbo is disabled. If I unplug my laptop, then the max frequency will be slightly lower than what it should be. That's at least what I noticed, I thought there would be more problematic situations. I will try to see if this can be avoided, in the meantime, here what I did so far. diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index c5eac94..1a10971 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -274,20 +274,54 @@ static void __init intel_pstate_debug_expose_params(void) return sprintf(buf, "%u\n", limits.object); \ } +static ssize_t show_no_turbo(struct kobject *kobj, + struct attribute *attr, char *buf) +{ + u64 misc_en; + int turbo_disabled = limits.turbo_disabled; + struct cpudata *cpu; + + cpu = all_cpu_data[0]; + rdmsrl(MSR_IA32_MISC_ENABLE, misc_en); + if (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE || + cpu->pstate.max_pstate == cpu->pstate.turbo_pstate) { + limits.turbo_disabled = 1; + else + limits.turbo_disabled = 0; + + if (limits.turbo_disabled != turbo_disabled) + limits.no_turbo = limits.turbo_disabled; + + return sprintf(buf, "%u\n", limits.no_turbo); +} + + static ssize_t store_no_turbo(struct kobject *a, struct attribute *b, const char *buf, size_t count) { unsigned int input; int ret; + u64 misc_en; + struct cpudata *cpu; + cpu = all_cpu_data[0]; ret = sscanf(buf, "%u", &input); if (ret != 1) return -EINVAL; - limits.no_turbo = clamp_t(int, input, 0 , 1); + + rdmsrl(MSR_IA32_MISC_ENABLE, misc_en); + if (misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE || + cpu->pstate.max_pstate == cpu->pstate.turbo_pstate) { + limits.turbo_disabled = 1; + else + limits.turbo_disabled = 0; + if (limits.turbo_disabled) { pr_warn("Turbo disabled by BIOS or unavailable on processor\n"); limits.no_turbo = limits.turbo_disabled; - } + } else + limits.no_turbo = clamp_t(int, input, 0 , 1); + return count; } @@ -323,7 +357,6 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b, return count; } -show_one(no_turbo, no_turbo); show_one(max_perf_pct, max_perf_pct); show_one(min_perf_pct, min_perf_pct);
Created attachment 148851 [details] Don't rely on the initial state of turbo I modified the first patch I sent as I couldn't find a better alternative solution as reliable as it. I just made few changes, "actual_no_turbo" is not really necessary, but it prevents any change of "no_turbo" and yet know whether the turbo is enabled or not.
Created attachment 149081 [details] cpufreq: intel_pstate: Don't rely on the initial state of the turbo I noticed that the last time I uploaded the wrong file, the kernel doesn't even compile with that patch, sorry. Anyway, I did some more changes. MSR_IA32_MISC_ENABLE_TURBO_DISABLE is still read frequently, but less now. I also got rid of actual_no_turbo, no_turbo is changed when turbo_disabled changes as it is currently done. The only problem I see with this patch is that the BIOS could change MSR_IA32_MISC_ENABLE_TURBO_DISABLE after update_turbo_state(), but before MSR_IA32_PERF_CTL is written. It shouldn't be a problem on my system though. I have a question. Is it normal that when the turbo is disabled and all the cores are at the max frequency allowed, the frequency doesn't change when I re-enable the turbo? The frequency has to decrease first and then can increase. The opposite transition (from turbo freq to non-turbo freq) is immediate.
Created attachment 149201 [details] cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update) Gabriele modified you patch slightly to not touch limits.no_turbo so user settings can survive limits.turbo_disable changes. I the correct value for the no_turbo sysfs file should be shown in all cases. Please take a look. TIA
Created attachment 149221 [details] cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update 2) I tested the patch, it's working fine. It's exactly what I initially wanted to do. However, I would make some changes in store_no_turbo(). If the turbo is disabled by the BIOS and I try to enable it, I don't want to have it disabled once the BIOS re-enable it. We could set no_turbo to 0 (its value has no effect in case turbo_disabled is 1 now), don't allow changes or allow changes. - If we don't allow changes, it could be annoying as the user maybe doesn't know the conditions under which he can change no_turbo. - If we set no_turbo to 0, the user preference doesn't change unless the user tries to change it when changes are not allowed. - If we allow changes, it could be confusing as the user can't verify the success of his changes. These are the considerations that led me to introduce "actual_no_turbo" (and make no_turbo dependant on turbo_disabled once I removed it). That said, I updated the patch in order to always allow changes of no_turbo. Note that I brought back "no_turbo = 0" in intel_pstate_set_policy() as it was before limits.turbo_disabled was introduced, even though I prefer when it's not there as it continuosly override my no_turbo preference. In any case, I would do that change in a different commit.
(In reply to Gabriele Mazzotta from comment #8) > Created attachment 149221 [details] > cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update > 2) > > I tested the patch, it's working fine. It's exactly what I initially wanted > to do. > Good :-) > However, I would make some changes in store_no_turbo(). If the turbo is > disabled by the BIOS and I try to enable it, I don't want to have it disabled > once the BIOS re-enable it. > We could set no_turbo to 0 (its value has no effect in case turbo_disabled > is 1 > now), don't allow changes or allow changes. > > - If we don't allow changes, it could be annoying as the user maybe doesn't > know the conditions under which he can change no_turbo. > - If we set no_turbo to 0, the user preference doesn't change unless the > user tries to change it when changes are not allowed. > - If we allow changes, it could be confusing as the user can't verify the > success of his changes. > > These are the considerations that led me to introduce "actual_no_turbo" (and > make no_turbo dependant on turbo_disabled once I removed it). > So i can argue both sides of this :-) What I went for was to accurately reflect the current state of the system while not crushing the user setting when the BIOS changed the availability of turbo. I would rather the user be annoyed that the setting was not changeable and log the message to the log than have them think that they had forced turbo on when they really hadn't. They will flip no_trubo to 0 then go measure the system with turbostat or other system monitoring tools and then here the "turbo doesn't work" bug report comes. The BIOS changing the availability of turbo takes the decision out of the users hands I think the interface should reflect this. Maybe the message logged when the user tries to enable turbo when it not available needs to be better. > That said, I updated the patch in order to always allow changes of no_turbo. > > Note that I brought back "no_turbo = 0" in intel_pstate_set_policy() as it > was > before limits.turbo_disabled was introduced, even though I prefer when it's > not > there as it continuosly override my no_turbo preference. In any case, I would > do that change in a different commit.
Created attachment 149551 [details] cpufreq: intel_pstate: Don't rely on the initial state of the turbo (update 3) Ok then. I did a minor change. Tying to change the state of the turbo when it's not allowed will now result in an error. I think this makes things a bit more clear.
I don't see this patch upstream -- re-opening. moving to intel_pstate from cpufreq category.
Hi, the fix is upstream (Linux v3.18-rc2) and was applied to the stable trees, so I mark this bug as resolved. commit 4521e1a0ce173daa23dfef8312d09051e057ff8e Author: Gabriele Mazzotta <gabriele.mzt@gmail.com> Date: Mon Oct 13 08:37:41 2014 -0700 cpufreq: intel_pstate: Reflect current no_turbo state correctly
thanks Gabriele -- marking this one closed.