| Message ID | 20250227153603.131046-3-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: platform_profile: fix legacy sysfs with multiple handlers | expand |
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. 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. > 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. 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. > }; > > int amd_pmf_init_sps(struct amd_pmf_dev *dev)
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. > 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. > 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) >
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. >> 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) >>
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. 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. > >> 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) > >> >
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) >>>> >>
On Thu, 27 Feb 2025 at 18:24, Mario Limonciello <mario.limonciello@amd.com> wrote: > > 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. Sure, how do we do that? You want to make amd-pmf accept both just for 6.14? I would be ok with that. > > > > 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. From a quick google search, Acer Swift Edge 16 - 8840U. But we do not have a lot of acer users I'd say. > > > snip > >> >
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; 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; + 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, }; int amd_pmf_init_sps(struct amd_pmf_dev *dev)
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(+)