Re: [PATCH v2 2/2] ACPI: platform_profile: make amd-pmf a secondary handler

[Mario Limonciello <mario.limonciello@amd.com>]

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

I think just covering the QUIET == LOW_POWER is the important one for now.

> 
> 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.

But do Acer and Alienware have designs that amd-pmf will bind at the 
same time?

I'm not so sure.

> 
>>>> 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)
>>>>
>>