Messages in this thread | | | From | Pratyush Yadav <> | Subject | Re: [PATCH v2 1/3] ACPI: processor: perflib: Use the "no limit" frequency QoS | Date | Thu, 29 Dec 2022 13:58:33 +0100 |
| |
Hi Rafael,
On Wed, Dec 28 2022, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > When _PPC returns 0, it means that the CPU frequency is not limited by > the platform firmware, so make acpi_processor_get_platform_limit() > update the frequency QoS request used by it to "no limit" in that case. > > This addresses a problem with limiting CPU frequency artificially on > some systems after CPU offline/online to the frequency that corresponds > to the first entry in the _PSS return package. > > Reported-by: Pratyush Yadav <ptyadav@amazon.de> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > > v1 -> v2: > * Move some changes into a separate patch > * Update the changelog accordingly > > --- > drivers/acpi/processor_perflib.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > Index: linux-pm/drivers/acpi/processor_perflib.c > =================================================================== > --- linux-pm.orig/drivers/acpi/processor_perflib.c > +++ linux-pm/drivers/acpi/processor_perflib.c > @@ -53,6 +53,8 @@ static int acpi_processor_get_platform_l > { > acpi_status status = 0; > unsigned long long ppc = 0; > + s32 qos_value; > + int index; > int ret; > > if (!pr) > @@ -72,17 +74,27 @@ static int acpi_processor_get_platform_l > } > } > > + index = ppc; > + > pr_debug("CPU %d: _PPC is %d - frequency %s limited\n", pr->id, > - (int)ppc, ppc ? "" : "not"); > + index, index ? "is" : "is not"); > > - pr->performance_platform_limit = (int)ppc; > + pr->performance_platform_limit = index; > > if (ppc >= pr->performance->state_count || > unlikely(!freq_qos_request_active(&pr->perflib_req))) > return 0; > > - ret = freq_qos_update_request(&pr->perflib_req, > - pr->performance->states[ppc].core_frequency * 1000); > + /* > + * If _PPC returns 0, it means that all of the available states can be > + * used ("no limit"). > + */ > + if (index == 0) > + qos_value = FREQ_QOS_MAX_DEFAULT_VALUE;
One small thing I noticed: in acpi_processor_ppc_init() "no limit" value is set to INT_MAX and here it is set to FREQ_QOS_MAX_DEFAULT_VALUE. Both should evaluate to the same value but I think it would be nice if the same thing is used in both places. Perhaps you can fix that up when applying?
Other than this,
Reviewed-by: Pratyush Yadav <ptyadav@amazon.de> Tested-by: Pratyush Yadav <ptyadav@amazon.de>
Thanks for working on this.
> + else > + qos_value = pr->performance->states[index].core_frequency * 1000; > + > + ret = freq_qos_update_request(&pr->perflib_req, qos_value); > if (ret < 0) { > pr_warn("Failed to update perflib freq constraint: CPU%d (%d)\n", > pr->id, ret); > > >
-- Regards, Pratyush Yadav
Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
| |