Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

zhenglifeng (A)
2025-02-26T10:22:48Z

On 2025/2/21 21:14, Sumit Gupta wrote:
>
>
> On 19/02/25 00:53, Rafael J. Wysocki 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.

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!

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.

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.

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?

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?

Regards,
Lifeng