RE: [PATCH v2 0/7] CPPC optional registers AMD support

[Ghannam, Yazen]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> Sent: Wednesday, April 17, 2019 5:11 PM
> To: Ghannam, Yazen <Yazen.Ghannam@xxxxxxx>
> Cc: Rafael J. Wysocki <rafael@xxxxxxxxxx>; Natarajan, Janakarajan <Janakarajan.Natarajan@xxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx; Rafael J . Wysocki <rjw@xxxxxxxxxxxxx>; Len Brown
> <lenb@xxxxxxxxxx>; Viresh Kumar <viresh.kumar@xxxxxxxxxx>; Robert Moore <robert.moore@xxxxxxxxx>; Erik Schmauss
> <erik.schmauss@xxxxxxxxx>
> Subject: Re: [PATCH v2 0/7] CPPC optional registers AMD support
> 
> On Wed, Apr 17, 2019 at 7:28 PM Ghannam, Yazen <Yazen.Ghannam@xxxxxxx> wrote:
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > > Sent: Tuesday, April 16, 2019 4:34 AM
> > > To: Natarajan, Janakarajan <Janakarajan.Natarajan@xxxxxxx>
> > > Cc: Natarajan, Janakarajan <Janakarajan.Natarajan@xxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > > pm@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx; Rafael J . Wysocki <rjw@xxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>; Viresh Kumar
> > > <viresh.kumar@xxxxxxxxxx>; Robert Moore <robert.moore@xxxxxxxxx>; Erik Schmauss <erik.schmauss@xxxxxxxxx>; Ghannam, Yazen
> > > <Yazen.Ghannam@xxxxxxx>
> > > Subject: Re: [PATCH v2 0/7] CPPC optional registers AMD support
> > >
> > > On Tue, Apr 16, 2019 at 12:35 AM Janakarajan Natarajan <jnataraj@xxxxxxx> wrote:
> > > >
> > > > On 4/4/19 4:25 PM, Natarajan, Janakarajan wrote:
> > > > > CPPC (Collaborative Processor Performance Control) offers optional
> > > > > registers which can be used to tune the system based on energy and/or
> > > > > performance requirements.
> > > > >
> > > > > Newer AMD processors add support for a subset of these optional CPPC
> > > > > registers, based on ACPI v6.1.
> > > > >
> > > > > The following are the supported CPPC registers for which sysfs entries
> > > > > are created:
> > > > > * enable                (NEW)
> > > > > * max_perf              (NEW)
> > > > > * min_perf              (NEW)
> > > > > * energy_perf
> > > > > * lowest_perf
> > > > > * nominal_perf
> > > > > * desired_perf          (NEW)
> > > > > * feedback_ctrs
> > > > > * auto_sel_enable       (NEW)
> > > > > * lowest_nonlinear_perf
> > > > >
> > > > > The CPPC driver is updated to enable the OSPM and the userspace to
> > > > > access
> > > > > the newly supported registers.
> > > > >
> > > > > The purpose of exposing the registers via the sysfs entries is to allow
> > > > > the userspace to:
> > > > > * Tweak the values to fit its workload.
> > > > > * Apply a profile from AMD's optimization guides.
> > > > >
> > > > > Profiles will be documented in the performance/optimization guides.
> > > > >
> > > > > Note:
> > > > > * AMD systems will not have a policy applied in the kernel at this time.
> > > > > * By default, acpi_cpufreq will still be used.
> > > > >
> > > > > TODO:
> > > > > * Create a linux userspace tool that will help users generate a CPPC
> > > > > * profile
> > > > >    for their target workload.
> > > > > * Create or update a driver to apply a general CPPC policy in the
> > > > > * kernel.
> > > > >
> > > > > v1->v2:
> > > > > * Add macro to ensure BUFFER only registers have BUFFER type.
> > > > > * Add support macro to make the right check based on register type.
> > > > > * Remove support checks for registers which are mandatory.
> > > >
> > > >
> > > > Are there any concerns regarding this patchset?
> > >
> > > Yes, there are.
> > >
> > > Unfortunately, it is generally problematic.
> > >
> > > First off, the behavior of existing sysfs files cannot be changed at
> > > will, as there may be users of them out there already depending on the
> > > current behavior.
> > >
> >
> > The intent is to add new sysfs files without changing the existing files. Is that okay?
> >
> > Or is the addition of new files also not acceptable?
> >
> > > Second, at least some CPPC control registers are used by cpufreq
> > > drivers (cppc_cpufreq and intel_pstate), so modifying them behind the
> > > drivers' backs is not a good idea in general.  For this reason, adding
> > > new sysfs attributes to allow user space to do that is quite
> > > questionable.
> > >
> >
> > Yes, good point.
> >
> > What if a check is added so that writes only succeed if the CPUFREQ governor is set to userspace?
> 
> That wouldn't be particularly straightforward IMO.
> 
> What about handling this like the others do, through a proper cpufreq driver?

That seemed unnecessary. We want to expose the CPPC interface directly so that advanced users can interact with the Platform without CPUFREQ. It seemed like the simplest way to do this is to extend the existing CPPC sysfs interface. Also, doing it this way would keep the interface driver/vendor-neutral, so users on any CPPC-enabled platform could use it.

However, I understand if that's too big of a change.

In this case, would you prefer something like the following?
1) Move the new interface somewhere else (another path, debugfs, etc.?) and mark it as "testing". This could reinforce that the new interface is for advanced users, testing, debugging, etc. However, the interface would still be populated by cppc_lib and would remain driver/vendor-neutral. And we could restrict the interface to prevent interactions with the CPUFREQ drivers.
2) Or, create a new CPUFREQ driver. The cppc_lib code could still be updated to support the new CPPC registers, but the driver would enable the sysfs interface. This would prevent any interactions with existing CPUFREQ drivers, but users will need to do more to enable the interface. We can still keep this driver vendor-neutral (cppc_userspace_cpufreq?).

Thanks,
Yazen