| Message ID | 20241202055031.8038-19-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
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.
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).
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?
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 --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; }