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

[Rafael J. Wysocki]

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?