| Message ID | 20240625134127.4464-3-Dhananjay.Ugwekar@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Mario Limonciello |
| Headers | show |
| Series | AMD Pstate driver fixes | expand |
On 6/25/2024 08:41, Dhananjay Ugwekar wrote: > On shared memory CPPC systems, with amd_pstate=active mode, the change > in scaling_min/max_freq doesn't get written to the shared memory > region. Due to this, the writes to the scaling_min/max_freq sysfs file > don't take effect. Fix this by propagating the scaling_min/max_freq > changes to the shared memory region. > > Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors") > Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 9ad62dbe8bfb..7c1c96abe5bd 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp) > cpudata->epp_cached = epp; > } else { > perf_ctrls.energy_perf = epp; > + perf_ctrls.max_perf = cpudata->max_limit_perf; > + perf_ctrls.min_perf = cpudata->min_limit_perf; > + perf_ctrls.desired_perf = 0U; > + > + ret = cppc_set_perf(cpudata->cpu, &perf_ctrls); > + if (ret) { > + pr_debug("failed to set min max limits (%d)\n", ret); > + return ret; > + } This feels like a handgrown implementation of amd_pstate_update_perf() (IE static call updated to cppc_update_perf). Can you just call that instead? > ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1); > if (ret) { > pr_debug("failed to set energy perf value (%d)\n", ret); > @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy) > } > > WRITE_ONCE(cpudata->cppc_req_cached, value); > + Spurious newline added here not relevant to this patch. > amd_pstate_set_epp(cpudata, epp); > } >
Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> writes: > On shared memory CPPC systems, with amd_pstate=active mode, the change > in scaling_min/max_freq doesn't get written to the shared memory > region. Due to this, the writes to the scaling_min/max_freq sysfs file > don't take effect. Fix this by propagating the scaling_min/max_freq > changes to the shared memory region. > > Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors") > Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> Please add the following in your v2: Reported-by: David Arcari <darcari@redhat.com> > --- > drivers/cpufreq/amd-pstate.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 9ad62dbe8bfb..7c1c96abe5bd 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp) > cpudata->epp_cached = epp; > } else { > perf_ctrls.energy_perf = epp; > + perf_ctrls.max_perf = cpudata->max_limit_perf; > + perf_ctrls.min_perf = cpudata->min_limit_perf; > + perf_ctrls.desired_perf = 0U; > + > + ret = cppc_set_perf(cpudata->cpu, &perf_ctrls); > + if (ret) { > + pr_debug("failed to set min max limits (%d)\n", ret); > + return ret; > + } > ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1); > if (ret) { > pr_debug("failed to set energy perf value (%d)\n", ret); > @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy) > } > > WRITE_ONCE(cpudata->cppc_req_cached, value); > + > amd_pstate_set_epp(cpudata, epp); > } > > -- -- Thanks and Regards gautham.
Hello Gautham, On 6/26/2024 10:54 AM, Gautham R.Shenoy wrote: > Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> writes: > >> On shared memory CPPC systems, with amd_pstate=active mode, the change >> in scaling_min/max_freq doesn't get written to the shared memory >> region. Due to this, the writes to the scaling_min/max_freq sysfs file >> don't take effect. Fix this by propagating the scaling_min/max_freq >> changes to the shared memory region. >> >> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors") >> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> > > Please add the following in your v2: > > Reported-by: David Arcari <darcari@redhat.com> Yup, will add. Thanks, Dhananjay > >> --- >> drivers/cpufreq/amd-pstate.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >> index 9ad62dbe8bfb..7c1c96abe5bd 100644 >> --- a/drivers/cpufreq/amd-pstate.c >> +++ b/drivers/cpufreq/amd-pstate.c >> @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp) >> cpudata->epp_cached = epp; >> } else { >> perf_ctrls.energy_perf = epp; >> + perf_ctrls.max_perf = cpudata->max_limit_perf; >> + perf_ctrls.min_perf = cpudata->min_limit_perf; >> + perf_ctrls.desired_perf = 0U; >> + >> + ret = cppc_set_perf(cpudata->cpu, &perf_ctrls); >> + if (ret) { >> + pr_debug("failed to set min max limits (%d)\n", ret); >> + return ret; >> + } >> ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1); >> if (ret) { >> pr_debug("failed to set energy perf value (%d)\n", ret); >> @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy) >> } >> >> WRITE_ONCE(cpudata->cppc_req_cached, value); >> + >> amd_pstate_set_epp(cpudata, epp); >> } >> >> -- > > -- > Thanks and Regards > gautham.
Hello Mario, On 6/25/2024 8:39 PM, Mario Limonciello wrote: > On 6/25/2024 08:41, Dhananjay Ugwekar wrote: >> On shared memory CPPC systems, with amd_pstate=active mode, the change >> in scaling_min/max_freq doesn't get written to the shared memory >> region. Due to this, the writes to the scaling_min/max_freq sysfs file >> don't take effect. Fix this by propagating the scaling_min/max_freq >> changes to the shared memory region. >> >> Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors") >> Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> >> --- >> drivers/cpufreq/amd-pstate.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >> index 9ad62dbe8bfb..7c1c96abe5bd 100644 >> --- a/drivers/cpufreq/amd-pstate.c >> +++ b/drivers/cpufreq/amd-pstate.c >> @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp) >> cpudata->epp_cached = epp; >> } else { >> perf_ctrls.energy_perf = epp; >> + perf_ctrls.max_perf = cpudata->max_limit_perf; >> + perf_ctrls.min_perf = cpudata->min_limit_perf; >> + perf_ctrls.desired_perf = 0U; >> + >> + ret = cppc_set_perf(cpudata->cpu, &perf_ctrls); >> + if (ret) { >> + pr_debug("failed to set min max limits (%d)\n", ret); >> + return ret; >> + } > > This feels like a handgrown implementation of amd_pstate_update_perf() (IE static call updated to cppc_update_perf). Yes, I didn't notice it, better to call the existing function. > > Can you just call that instead? > >> ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1); >> if (ret) { >> pr_debug("failed to set energy perf value (%d)\n", ret); >> @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy) >> } >> WRITE_ONCE(cpudata->cppc_req_cached, value); >> + > > Spurious newline added here not relevant to this patch. Yes, will remove it Regards, Dhananjay >> amd_pstate_set_epp(cpudata, epp); >> } >> >
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 9ad62dbe8bfb..7c1c96abe5bd 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -264,6 +264,15 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp) cpudata->epp_cached = epp; } else { perf_ctrls.energy_perf = epp; + perf_ctrls.max_perf = cpudata->max_limit_perf; + perf_ctrls.min_perf = cpudata->min_limit_perf; + perf_ctrls.desired_perf = 0U; + + ret = cppc_set_perf(cpudata->cpu, &perf_ctrls); + if (ret) { + pr_debug("failed to set min max limits (%d)\n", ret); + return ret; + } ret = cppc_set_epp_perf(cpudata->cpu, &perf_ctrls, 1); if (ret) { pr_debug("failed to set energy perf value (%d)\n", ret); @@ -1547,6 +1556,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy) } WRITE_ONCE(cpudata->cppc_req_cached, value); + amd_pstate_set_epp(cpudata, epp); }
On shared memory CPPC systems, with amd_pstate=active mode, the change in scaling_min/max_freq doesn't get written to the shared memory region. Due to this, the writes to the scaling_min/max_freq sysfs file don't take effect. Fix this by propagating the scaling_min/max_freq changes to the shared memory region. Fixes: ffa5096a7c33 ("cpufreq: amd-pstate: implement Pstate EPP support for the AMD processors") Signed-off-by: Dhananjay Ugwekar <Dhananjay.Ugwekar@amd.com> --- drivers/cpufreq/amd-pstate.c | 10 ++++++++++ 1 file changed, 10 insertions(+)