On Thu, 27 Feb 2025 at 18:10, Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> On 2/27/2025 11:04, Antheas Kapenekakis wrote:
> > On Thu, 27 Feb 2025 at 17:46, Mario Limonciello
> > <mario.limonciello@xxxxxxx> 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@xxxxxxxxxxx>
> >>> ---
> >>> 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)
> >>
>
