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

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

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.