diff mbox series

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

Message ID 20241202055031.8038-19-mario.limonciello@amd.com (mailing list archive)
State Superseded, archived
Headers show
Series Add support for binding ACPI platform profile to multiple drivers | expand

Commit Message

Mario Limonciello Dec. 2, 2024, 5:50 a.m. UTC 
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(-)

Comments

Ilpo Järvinen Dec. 5, 2024, 2:22 p.m. UTC | #1 
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.
Mario Limonciello Dec. 5, 2024, 2:46 p.m. UTC | #2 
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.

If this has been done they're "off the beating path" of a predfined
profile because they're picking and choosing individual knobs.

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).
Ilpo Järvinen Dec. 5, 2024, 3:22 p.m. UTC | #3 
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?

> 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?
Mario Limonciello Dec. 5, 2024, 3:35 p.m. UTC | #4 
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.
diff mbox series

Patch

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);
+
+		next = find_next_bit_wrap(choices,
+					  PLATFORM_PROFILE_LAST,
 					  profile + 1);
 
-		if (WARN_ON(next == PLATFORM_PROFILE_LAST))
-			return -EINVAL;
+		err = class_for_each_device(&platform_profile_class, NULL, &next,
+					    _store_and_notify);
 
-		err = cur_profile->profile_set(cur_profile, next);
 		if (err)
 			return err;
 	}