Message-ID: 
 <CAGwozwGNV0gccAH-TXCi4PCnuWFOA0v8KkiZJ8Z+fZ+_ft6UAA@mail.gmail.com>
Date: Thu, 27 Feb 2025 18:18:35 +0100
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Mario Limonciello <mario.limonciello@....com>
Cc: mpearson-lenovo@...ebb.ca, ilpo.jarvinen@...ux.intel.com, lenb@...nel.org,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
	platform-driver-x86@...r.kernel.org, rafael@...nel.org, hdegoede@...hat.com,
	me@...egospodneti.ch, luke@...nes.dev
Subject: Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary
 handler

On Thu, 27 Feb 2025 at 18:10, Mario Limonciello
<mario.limonciello@....com> wrote:
>
> On 2/27/2025 11:04, Antheas Kapenekakis wrote:
> > On Thu, 27 Feb 2025 at 17:46, Mario Limonciello
> > <mario.limonciello@....com> wrote:
> >>
> >> On 2/27/2025 09:36, Antheas Kapenekakis wrote:
> >>> Since amd-pmf is expected to run alongside other platform handlers, it
> >>> should be able to accept all platform profiles. Therefore, mark it as
> >>> secondary and in the case of a custom profile, make it NOOP without an
> >>> error to allow primary handlers to receive a custom profile.
> >>> The sysfs endpoint will still report custom, after all.
> >>>
> >>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> >>> ---
> >>>    drivers/platform/x86/amd/pmf/spc.c | 3 +++
> >>>    drivers/platform/x86/amd/pmf/sps.c | 8 ++++++++
> >>>    2 files changed, 11 insertions(+)
> >>>
> >>> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> >>> index f34f3130c330..99c48378f943 100644
> >>> --- a/drivers/platform/x86/amd/pmf/spc.c
> >>> +++ b/drivers/platform/x86/amd/pmf/spc.c
> >>> @@ -219,12 +219,15 @@ static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_
> >>>
> >>>        switch (dev->current_profile) {
> >>>        case PLATFORM_PROFILE_PERFORMANCE:
> >>> +     case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> >>>                val = TA_BEST_PERFORMANCE;
> >>>                break;
> >>>        case PLATFORM_PROFILE_BALANCED:
> >>>                val = TA_BETTER_PERFORMANCE;
> >>>                break;
> >>>        case PLATFORM_PROFILE_LOW_POWER:
> >>> +     case PLATFORM_PROFILE_COOL:
> >>> +     case PLATFORM_PROFILE_QUIET:
> >>>                val = TA_BEST_BATTERY;
> >>
> >> I would really prefer we do the absolute bare minimum to help this issue
> >> on ASUS (just special case quiet) and leave adding compat for other
> >> profiles for other development.
> >
> > I cannot risk other drivers having their options disabled. This can
> > have carry-on effects in other drivers too.
> >
> > Including in the legion v3 driver, in which you will end up disabling
> > balanced-performance. Since Derek posted the v3 for that today.
> >
>
> Sure - but let's handle that separately from this bug fix.  That driver
> will be targeted to 6.15 or later.
>
> We need to be cognizant about what can go into 6.14 needs to be bug
> fixes for drivers in tree.

For me to consider this problem resolved, I need a mitigation that
matches the behavior of this patch series 1-1.

If you have a better suggestion, I can implement it and test it real quick.

If this issue is not fully resolved, it will cause a lot of downstream
issues that will result in the legacy interface becoming unusable.

Acer and alienware implement balanced performance too. In the current tree.

> >> The reason for this is that if you look at power_modes_v2 there are
> >> actually 4 'possible' modes for v2 platforms.  So there is a bit of
> >> nuance involved and it's really not a 'bug fix' anymore by doing so much
> >> at once.
> >>
> >>>                break;
> >>>        default:
> >>> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> >>> index e6cf0b22dac3..a2a8511768ce 100644
> >>> --- a/drivers/platform/x86/amd/pmf/sps.c
> >>> +++ b/drivers/platform/x86/amd/pmf/sps.c
> >>> @@ -297,12 +297,15 @@ int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf)
> >>>
> >>>        switch (pmf->current_profile) {
> >>>        case PLATFORM_PROFILE_PERFORMANCE:
> >>> +     case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> >>>                mode = POWER_MODE_PERFORMANCE;
> >>>                break;
> >>>        case PLATFORM_PROFILE_BALANCED:
> >>>                mode = POWER_MODE_BALANCED_POWER;
> >>>                break;
> >>>        case PLATFORM_PROFILE_LOW_POWER:
> >>> +     case PLATFORM_PROFILE_COOL:
> >>> +     case PLATFORM_PROFILE_QUIET:
> >>>                mode = POWER_MODE_POWER_SAVER;
> >>>                break;
> >>>        default:
> >>> @@ -369,6 +372,10 @@ static int amd_pmf_profile_set(struct device *dev,
> >>>        struct amd_pmf_dev *pmf = dev_get_drvdata(dev);
> >>>        int ret = 0;
> >>>
> >>> +     /* If the profile is custom, bail without an error. */
> >>> +     if (profile == PLATFORM_PROFILE_CUSTOM)
> >>> +             return 0;
> >>> +
> >>
> >> The legacy interface doesn't support writing custom.
> >>
> >> https://github.com/torvalds/linux/blob/v6.14-rc3/drivers/acpi/platform_profile.c#L382
> >>
> >> IoW this is dead code.
> >
> > Noted.
> >
> >>>        pmf->current_profile = profile;
> >>>
> >>>        /* Notify EC about the slider position change */
> >>> @@ -400,6 +407,7 @@ static const struct platform_profile_ops amd_pmf_profile_ops = {
> >>>        .probe = amd_pmf_profile_probe,
> >>>        .profile_get = amd_pmf_profile_get,
> >>>        .profile_set = amd_pmf_profile_set,
> >>> +     .secondary = true,
> >>
> >> I really don't understand the need for declaring primary / secondary.
> >> It really doesn't matter which driver can do it.  This same problem
> >> could happen in any direction.
> >
> > No. amd-pmf is responsible here. That's why you made the multiple
> > platform profile series after all. Other WMI drivers never load
> > together. To maintain existing compatibility, those drivers need to
> > still show the same options under the legacy endpoint.
>
> My point is mostly hypothetical right now because the realistic
> combinations right now are amd-pmf + other driver.
>
> >
> >> As a different suggestion; how about a new "generic" callback for
> >> 'compatibility' profiles?
> >>
> >> Right now the .probe() callback amd_pmf_get_pprof_modes() will set bits
> >> for visible profiles.
> >>
> >> How about an optional .compat_profiles() for the hidden one(s)?  This
> >> would allow any driver to implement them.
> >
> > amd-pmf cannot obscure any settings of the primary platform, so even
> > in this case it ends up implementing all of them, and compat profiles
> > becomes equivalent to .secondary with more steps (incl. a probe).
> >
> >>>    };
> >>>
> >>>    int amd_pmf_init_sps(struct amd_pmf_dev *dev)
> >>
>

Powered by blists - more mailing lists