On Fri, Feb 14, 2025 at 8:09 AM Sumit Gupta <sumitg@xxxxxxxxxx> wrote: > > > > On 12/02/25 16:22, zhenglifeng (A) wrote: > > External email: Use caution opening links or attachments > > > > > > 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@xxxxxxxxxx/ > >>>> > >>>> ? > >>> > >>> 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@xxxxxxxxxx/ > >> > >> > > > > 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. > > > > Hi Lifeng, > > I think its more appropriate to keep all the CPC registers together > instead of splitting the read only registers to the acpi_cppc sysfs > and read/write registers to the cpufreq sysfs. > > Only the EPP register is written from Intel and AMD. > $ grep cpufreq_freq_attr_rw drivers/cpufreq/* | grep -v scaling > drivers/cpufreq/acpi-cpufreq.c:cpufreq_freq_attr_rw(cpb); > > drivers/cpufreq/amd-pstate.c:cpufreq_freq_attr_rw(energy_performance_preference); > > drivers/cpufreq/intel_pstate.c:cpufreq_freq_attr_rw(energy_performance_preference); > > We are currently updating four registers and there can be more in > future like 'auto_act_window' update attribute in [1]. > Changed to make this conditional with 'ifdef CONFIG_ACPI_CPPC_CPUFREQ' > to not create attributes for Intel/AMD. > > +++ b/drivers/acpi/cppc_acpi.c > @@ static struct attribute *cppc_attrs[] = { > &lowest_freq.attr, > +#ifdef CONFIG_ACPI_CPPC_CPUFREQ > &max_perf.attr, > &min_perf.attr, > &perf_limited.attr, > &auto_activity_window.attr, > &energy_perf.attr, > +#endif > > > 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. > > > > cpufreq_cpu_get() is already used by multiple non-cpufreq drivers. > So, don't think its a problem. > $ grep -inr "= cpufreq_cpu_get(.*;" drivers/*| grep -v "cpufreq/"|wc -l > 10 > > > 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? > > > > With 'cppc_cpufreq_epp_driver', we provide an easy option to boot all > CPU's in auto mode with right epp and policy min/max equivalent of > {min|max}_perf. The mode can be found clearly with scaling_driver node. > Separating the HW and SW mode based on driver instance also > makes it easy to scale later. > Advanced users can program sysfs to switch individual CPU's in and out > of the HW mode. We can update policy min/max values accordingly. > In this case, there can be some CPU's in SW mode with epp driver > instance. But a separate instance will be more convenient for the > users who want all CPU's either in HW mode or in SW mode than having > to explicitly set all the values correctly. 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!
