Message-ID: <00c0f7af-2e1a-41d5-9e56-abf5ef4a2704@huawei.com>
Date: Sat, 19 Apr 2025 15:44:47 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: Sumit Gupta <sumitg@...dia.com>, "Rafael J. Wysocki" <rafael@...nel.org>,
	Viresh Kumar <viresh.kumar@...aro.org>
CC: <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

Hi Sumit,

May I resend the patch 8 in [1] first? Because I really need this new
feature.

After that patch being merged, you can resend this series base on that,
change the paths of the sysfs files, add a new cppc_cpufreq instance or do
anything in that series. Then we can continue this discussion.

Is that all right?

On 2025/4/1 21:56, zhenglifeng (A) wrote:

> Sorry for the delay.
> 
> On 2025/3/14 20:48, Sumit Gupta wrote:
>>
>>
>>>>>
>>>>> There seems to be some quite fundamental disagreement on how this
>>>>> should be done, so I'm afraid I cannot do much about it ATM.
>>>>>
>>>>> Please agree on a common approach and come back to me when you are ready.
>>>>>
>>>>> Sending two concurrent patchsets under confusingly similar names again
>>>>> and again isn't particularly helpful.
>>>>>
>>>>> Thanks!
>>>>
>>>> Hi Rafael,
>>>>
>>>> Thank you for looking into this.
>>>>
>>>> Hi Lifeng,
>>>>
>>>> As per the discussion, we can make the driver future extensible and
>>>> also can optimize the register read/write access.
>>>>
>>>> I gave some thought and below is my proposal.
>>>>
>>>> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>>>>     to read/write a cpc register.
>>>>
>>>> 2) Pick my patches in [2]:
>>>>     - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>>>>                  Also, update existing API's to read/write regs in batch.
>>>>     - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>>>>       all CPU's in Auto mode and set registers with right values.
>>>>       They can be updated after boot from sysfs to change hints to HW.
>>>>       I can use the optimized API's from [1] where required in [2].
>>>>
>>>> Let me know if you are okay with this proposal.
>>>> I can also send an updated patch series with all the patches combined?
>>>>
>>>> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>>> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
>>>>
>>>> Regards,
>>>> Sumit Gupta
>>>>
>>>
>>> Hi Sumit,
>>>
>>> Over the past few days, I've been thinking about your proposal and
>>> scenario.
>>>
>>> I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so
>>> the rest of the discussion focuses on the differences between [2] and the
>>> PATCH 8 in [1].
>>>
>>> We both tried to support autonomous selection mode in cppc_cpufreq but on
>>> different ways. I think the differences between these two approaches can be
>>> summarized into three questions:
>>>
>>> 1. Which sysfs files to expose? I think this is not a problem, we can keep
>>> all of them.
>>>
>>> 2. Where to expose these sysfs files? I understand your willing to keep all
>>> cpc registers together under acpi_cppc sysfs. But in my opinion, it is more
>>> suitable to expose them under cppc_cpufreq_attr, for these reasons:
>>>
>>>    1) It may probably introduce concurrency and data consistency issues, as
>>> I mentioned before.
>>>
>>
>> As explained in previous reply, this will be solved with the ifdef
>> check to enable the attributes for only those CPUFREQ drivers which want
>> to use the generic nodes.
>>  e.g: '#ifdef CONFIG_ACPI_CPPC_CPUFREQ' for 'cppc_cpufreq'.
>>            
>> These CPC register read/write sysfs nodes are generic as per the ACPI
>> specification and without any vendor specific logic.
>> So, adding them in the lib file 'cppc_acpi.c'(CONFIG_ACPI_CPPC_LIB) will
>> avoid code duplication if a different or new ACPI based CPUFREQ driver
>> also wants to use them just by adding their macro check. Such ifdef check is also used in other places for attributes creation like below.
>> So, don't look like a problem.
>>  $ grep -A4 "acpi_cpufreq_attr\[" drivers/cpufreq/acpi-cpufreq.c
>>  static struct freq_attr *acpi_cpufreq_attr[] = {
>>     &freqdomain_cpus,
>>  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>>     &cpb,
>>  #endif
> 
> So in the future, we will see:
> 
> static struct attribute *cppc_attrs[] = {
> 	...
> #ifdef CONFIG_XXX
> 	&xxx.attr,
> 	&xxx.attr,
> #endif
> #ifdef CONFIG_XXX
> 	&xxx.attr,
> #endif
> #ifdef CONFIG_XXX
> 	&xxx.attr,
> 	...
> };
> 
> I think you are making things more complicated.
> 
>>
>>>    2) The store functions call cpufreq_cpu_get() to get policy and update
>>> the driver_data which is a cppc_cpudata. Only the driver_data in
>>> cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate
>>> in cppc_acpi. This file currently provides interfaces for cpufreq drivers
>>> to use. Reverse calls might mess up call relationships, break code
>>> structures, and cause problems that are hard to pinpoint the root cause!
>>>
>>
>> If we don't want to update the cpufreq policy from 'cppc_acpi.c' and only update it from within the cpufreq,    then this could be one valid
>> point to not add the write syfs nodes in 'cppc_acpi.c' lib file.
>>
>> @Rafael, @Viresh : Do you have any comments on this?
> 
> I think updating cpufreq policy from 'cppc_acpi.c' should be forbidden.
> 
>>
>>>    3) Difficult to extend. Different cpufreq drivers may have different
>>> processing logic when reading from and writing to these CPC registers.
>>> Limiting all sysfs here makes it difficult for each cpufreq driver to
>>> extend. I think this is why there are only read-only interfaces under
>>> cppc_attrs before.
>>>
>>
>> We are updating the CPC registers as per the generic ACPI specification.
>> So, any ACPI based CPUFREQ driver can use these generic nodes to
>> read/write reg's until they have a vendor specific requirement or
>> implementation.
>> As explained above, If someone wants to update in different way and use
>> their own CPUFREQ driver then these generic attributes won't be created
>> due to the CPUFREQ driver macro check.
>> I think AMD and Intel are doing more than just reading/updating the registers. That's why they needed their driver specific implementations.
>>
>>> Adding a 'ifdef' is not a good way to solve these problems. Defining this
>>> config does not necessarily mean that the cpufreq driver is cppc_cpufreq.
>>>
>>
>> It means that only.
>>  ./drivers/cpufreq/Makefile:obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
> 
> Compile this file does not mean that the cpufreq driver is cppc_cpufreq.
> Driver registration may fail, and the actually loaded driver may be
> another. It'll be dangerous to expose these sysfs files for users to update
> registers' value in this case.
> 
>>
>>> 3. Is it necessary to add a new driver instance? [1] exposed the sysfs
>>> files to support users dynamically change the auto selection mode of each
>>> policy. Each policy can be operated seperately. It seems to me that if you
>>> want to boot all CPUs in auto mode, it should be sufficient to set all
>>> relevant registers to the correct values at boot time. I can't see why the
>>> new instance is necessary unless you explain it further. Could you explain
>>> more about why you add a new instance starting from answer these questions:
>>>
>>> For a specific CPU, what is the difference between using the two instances
>>> when auto_sel is 1? And what is the difference when auto_sel is 0?
>>>
>>
>> Explained this in previous reply. Let me elaborate more.
>>
>> For hundred's of CPU's, we don't need to explicitly set multiple sysfs
>> after boot to enable and configure Auto mode with right params. That's why an easy option is to pass boot argument or module param for enabling
>> and configuration.
>> A separate instance 'cppc_cpufreq_epp' of the 'cppc_cpufreq' driver is
>> added because policy min/max need to be updated to the min/max_perf
>> and not nominal/lowest nonlinear perf which is done by the default
>> init hook. Min_perf value can be lower than lowest nonlinear perf and Max_perf can be higher than nominal perf.
>> If some CPU is booted with epp instance and later the auto mode is disabled or min/max_perf is changed from sysfs then also the policy
>> min/max need to be updated accordingly.
>>
>> Another is that in Autonomous mode the freq selection and setting is
>> done by HW. So, cpufreq_driver->target() hook is not needed.
>> These are few reasons which I am aware of as of now.
>> I think in future there can be more. Having a separate instance
>> reflecting a HW based Autonomous frequency selection will make it easy
>> for any future changes.
> 
> So CPUs will act totally differently under these two instance. But what if
> I want part of the CPUs in HW mode and others in SW mode? Should I boot on
> HW mode and set some policies' auto_set to false or the other way? It seems
> like the effects of theses two approaches are completely different. In my
> opinion, this new instance is more like a completely different driver than
> cppc_cpufreq.
> 
>>
>>> If it turns out that the new instance is necessary, I think we can reach a
>>> common approach by adding this new cpufreq driver instance and place the
>>> attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did.
>>>
>>> What do you think?
>>
>> I initially thought about this but there was a problem.
>> What if we boot with non-epp instance which doesn't have these attributes and later want to enable Auto mode for few CPU's from sysfs.
> 
> That's the problem. CPUs can be set to Auto mode with or without this new
> instance. So what's the point of it?
> 
>>
>>
>> Best Regards,
>> Sumit Gupta
> 
> 
> 

Powered by blists - more mailing lists