Re: [PATCH v9 18/22] ACPI: platform_profile: Check all profile handler to calculate next

Mario Limonciello
2024-12-05T15:36:01Z

On 12/5/2024 09:22, Ilpo Järvinen wrote:
> On Thu, 5 Dec 2024, Mario Limonciello wrote:
>
>> On 12/5/2024 08:22, Ilpo Järvinen wrote:
>>> On Sun, 1 Dec 2024, Mario Limonciello wrote:
>>>
>>>> As multiple platform profile handlers might not all support the same
>>>> profile, cycling to the next profile could have a different result
>>>> depending on what handler are registered.
>>>>
>>>> Check what is active and supported by all handlers to decide what
>>>> to do.
>>>>
>>>> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
>>>> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>> drivers/acpi/platform_profile.c | 30 +++++++++++++++++++++---------
>>>> 1 file changed, 21 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/platform_profile.c
>>>> b/drivers/acpi/platform_profile.c
>>>> index d5f0679d59d50..16746d9b9aa7c 100644
>>>> --- a/drivers/acpi/platform_profile.c
>>>> +++ b/drivers/acpi/platform_profile.c
>>>> @@ -407,25 +407,37 @@ EXPORT_SYMBOL_GPL(platform_profile_notify);
>>>> int platform_profile_cycle(void)
>>>> {
>>>> - enum platform_profile_option profile;
>>>> - enum platform_profile_option next;
>>>> + enum platform_profile_option next = PLATFORM_PROFILE_LAST;
>>>> + enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>>>> + unsigned long choices[BITS_TO_LONGS(PLATFORM_PROFILE_LAST)];
>>>> int err;
>>>> + set_bit(PLATFORM_PROFILE_LAST, choices);
>>>> scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &profile_lock) {
>>>> - if (!cur_profile)
>>>> - return -ENODEV;
>>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>>> + &profile, _aggregate_profiles);
>>>> + if (err)
>>>> + return err;
>>>> - err = cur_profile->profile_get(cur_profile, &profile);
>>>> + if (profile == PLATFORM_PROFILE_CUSTOM ||
>>>> + profile == PLATFORM_PROFILE_LAST)
>>>> + return -EINVAL;
>>>> +
>>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>>> + choices, _aggregate_choices);
>>>> if (err)
>>>> return err;
>>>> - next = find_next_bit_wrap(cur_profile->choices,
>>>> PLATFORM_PROFILE_LAST,
>>>> + /* never iterate into a custom if all drivers supported it */
>>>> + clear_bit(PLATFORM_PROFILE_CUSTOM, choices);
>>>
>>> I'm confused by the comment. I was under impression the custom "profile"
>>> is just a framework construct when the _framework_ couldn't find a
>>> consistent profile? How can a driver decide to "support it"? It sounds
>>> like a driver overstepping its intended domain of operation.
>>>
>>> If the intention really is for the driver to "support" or "not support"
>>> custom profile, then you should adjust the commit message of the patch
>>> which introduced it.
>>>
>>
>> Yes I had envisioned that a driver could potentially set custom as well.
>>
>> This idea was introduced by my RFC series that precluded doing the
>> multiple driver handlers.
>>
>> The basic idea is that some drivers (for example asus-wmi and asus-armoury)
>> have the ability for the user to change a sysfs file that represents sPPT or
>> fPPT directly.
>
> I recall that series.
>
>> If this has been done they're "off the beating path" of a predfined
>> profile because they're picking and choosing individual knobs.
>
> The user would still not set it to "custom" nor driver "support" it,
> right? But it's a consequence of tuning those other knobs? Or do you mean
> user would first have to set "custom" and tuning the knobs is blocked
> otherwise?

I think the driver would have to "support" it. But in terms of a user
having to set "custom" and blocking the knobs until they do I think we
can go back and forth on. I don't feel strongly on how the semantics
would work.

>
>> So if a user touches those a driver could set profile as "custom" and if a
>> user chooses the platform profile the driver will override all of those and
>> report a pre-defined profile.
>>
>> So, yes I had that all in my mind but as you point out I definitely forgot to
>> mention it in the commit messages.
>>
>> Do you agree with it? If so, I'll amend the next version where applicable
>> (probably the patch that introduces custom and the documentation patch).
>
> I'm a little worried about overloading the meaning from mere profile
> disagreement to truly off the charted waters travel. Albeit, I suppose
> that overloading is just between global "custom" vs per-driver "custom",
> the latter would never be "custom" in case of mere profile disagreement,
> if I've understood everything correctly?
>

I personally see both as the same. I think we're in agreement on
multi-driver handler and why custom makes sense.

But think about the common case of "one driver handler". For the
purpose of this conversation let's say it's a system that supports
asus-armory and not amd-pmf and that asus-armory supports "custom".

If the user enabled custom ('either' directly or by writing a file that
set it) I think it's best that the "global" platform profile advertises
it too.

Specifically I think about how it translates over into the power slider
in GNOME/KDE. I don't think it's right this slider should show
power-saver if someone manually tuned sPPT up to a giant value.

However if the global platform profile advertises "custom", then the
slider behavior could show an overlay string for "custom", "undefined",
a "!" or something like that.