Message-ID: <8d5e0035-d8fe-49ef-bda5-f5881ff96657@huawei.com>
Date: Wed, 12 Feb 2025 18:52:02 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: Sumit Gupta <sumitg@...dia.com>, Viresh Kumar <viresh.kumar@...aro.org>
CC: <rafael@...nel.org>, <lenb@...nel.org>, <robert.moore@...el.com>,
	<corbet@....net>, <linux-pm@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <acpica-devel@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
	<treding@...dia.com>, <jonathanh@...dia.com>, <sashal@...dia.com>,
	<vsethi@...dia.com>, <ksitaraman@...dia.com>, <sanjayc@...dia.com>,
	<bbasu@...dia.com>
Subject: Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

On 2025/2/11 22:08, Sumit Gupta wrote:
> 
> 
>>
>> On 2025/2/11 18:44, Viresh Kumar wrote:
>>> On 11-02-25, 16:07, Sumit Gupta wrote:
>>>> This patchset supports the Autonomous Performance Level Selection mode
>>>> in the cppc_cpufreq driver. The feature is part of the existing CPPC
>>>> specification and already present in Intel and AMD specific pstate
>>>> cpufreq drivers. The patchset adds the support in generic acpi cppc
>>>> cpufreq driver.
>>>
>>> Is there an overlap with:
>>>
>>> https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>>
>>> ?
>>
>> Ha, it looks like we're doing something very similar.
>>
> 
> Hi Viresh,
> 
> Thank you for pointing to [1].
> 
> There seems to be some common points about updating the 'energy_perf'
> and 'auto_sel' registers for autonomous mode but the current patchset
> has more comprehensive changes to support Autonomous mode with the
> cppc_cpufreq driver.
> 
> The patches in [1]:
> 1) Make the cpc register read/write API’s generic and improves error
>    handling for 'CPC_IN_PCC'.
> 2) Expose sysfs under 'cppc_cpufreq_attr' to update 'auto_select',
>    'auto_act_window' and 'epp' registers.
> 
> The current patch series:
> 1) Exposes sysfs under 'cppc_attrs' to keep CPC registers together.
> 2) Updates existing API’s to use new registers and creates new API
>    with similar semantics to get all perf_ctrls.
> 3) Renames some existing API’s for clarity.
> 4) Use these existing API’s from acpi_cppc sysfs to update the CPC
>    registers used in Autonomous mode:
>    'auto_select', 'epp', 'min_perf', 'max_perf' registers.
> 5) Add separate 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq'
>    driver to apply different limit and policy for Autonomous mode.
>    Having it separate will avoid confusion between SW and HW mode.
>    Also, it will be easy to scale and add new features in future
>    without interference. Similar approach is used in Intel and AMD
>    pstate drivers.
> 
> Please share inputs about the preferred approach.
> 
> Best Regards,
> Sumit Gupta
> 
> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> 
> 

Hi Sumit,

Thanks for upstreaming this.

I think the changes to cppc_acpi in this patchset is inappropriate.

1) cppc_attrs are common sysfs for any system that supports CPPC. That
means, these interfaces would appear even if the cpufreq driver has already
managing it, e.g. amd-pstate and cppc_cpufreq. This would create multiple
interfaces to modify the same CPPC regs, which may probably introduce
concurrency and data consistency issues. Instead, exposing the interfaces
under cppc_cpufreq_attr decouples the write access to CPPC regs.

2) It's inappropriate to call cpufreq_cpu_get() in cppc_acpi. This file
currently provides interfaces for cpufreq drivers to use. It has no ABI
dependency on cpufreq at the moment.

Apart from the changes to cppc_acpi, I think the whole patchset in [1] can
be independent to this patchset. In other words, adding the
cppc_cpufreq_epp_driver could be standalone to discuss. I think combining
the use of ->setpolicy() and setting EPP could be a use case? Could you
explain more on the motivation of adding a new cppc_cpufreq_epp_driver?

[1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/

Regards,
Lifeng

>>>
>>>> It adds a new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver
>>>> for supporting the Autonomous Performance Level Selection and Energy
>>>> Performance Preference (EPP).
>>>> Autonomous selection will get enabled during boot if 'cppc_auto_sel'
>>>> boot argument is passed or the 'Autonomous Selection Enable' register
>>>> is already set before kernel boot. When enabled, the hardware is
>>>> allowed to autonomously select the CPU frequency within the min and
>>>> max perf boundaries using the Engergy Performance Preference hints.
>>>> The EPP values range from '0x0'(performance preference) to '0xFF'
>>>> (energy efficiency preference).
>>>>
>>>> It also exposes the acpi_cppc sysfs nodes to update the epp, auto_sel
>>>> and {min|max_perf} registers for changing the hints to hardware for
>>>> Autonomous selection.
>>>>
>>>> In a followup patch, plan to add support to dynamically switch the
>>>> cpufreq driver instance from 'cppc_cpufreq_epp' to 'cppc_cpufreq' and
>>>> vice-versa without reboot.
>>>>
>>>> The patches are divided into below groups:
>>>> - Patch [1-2]: Improvements. Can be applied independently.
>>>> - Patch [3-4]: sysfs store nodes for Auto mode. Depend on Patch [1-2].
>>>> - Patch [5]: Support for 'cppc_cpufreq_epp'. Uses a macro from [3].
>>>>
>>>> Sumit Gupta (5):
>>>>    ACPI: CPPC: add read perf ctrls api and rename few existing
>>>>    ACPI: CPPC: expand macro to create store acpi_cppc sysfs node
>>>>    ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from
>>>>      sysfs
>>>>    Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt
>>>>    cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode
>>>>
>>>>   Documentation/admin-guide/acpi/cppc_sysfs.rst |  28 ++
>>>>   .../admin-guide/kernel-parameters.txt         |  11 +
>>>>   drivers/acpi/cppc_acpi.c                      | 311 ++++++++++++++++--
>>>>   drivers/cpufreq/cppc_cpufreq.c                | 260 ++++++++++++++-
>>>>   include/acpi/cppc_acpi.h                      |  19 +-
>>>>   5 files changed, 572 insertions(+), 57 deletions(-)
>>>
>>

Powered by blists - more mailing lists