Re: [PATCH] platform/x86: hp-wmi: add support for quiet thermal profile

[Elia Devito <eliadevito@gmail.com>]

Hi Hans,

Il giorno mar, 27/09/2022 alle 12.24 +0200, Hans de Goede ha scritto:
> Hi Elia,
> 
> On 9/25/22 22:42, Elia Devito wrote:
> > The quiet profile is available only on models without dGPU,
> > so we enable it when the device has only one GPU onboard.
> > 
> > Signed-off-by: Elia Devito <eliadevito@gmail.com>
> > Tested-by: Philippe Rouquier <bonfire-app@wanadoo.fr>
> 
> Thank you for your patch.
> 
> I have just landed a very similar patch form Jorge from HP:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next&id=00b1829294b7c88ecba92c661fbe6fe347b364d2
> 

Sorry for the duplicate I hadn’t seen Jorge’s patch.

> The first 3 hunks of the patch are the same, only the conditions
> under which the:
> 
>         set_bit(PLATFORM_PROFILE_QUIET,
> platform_profile_handler.choices);
> 
> call is made are different, Jorge's patch does this essentially like
> this:
>         
>         if (!is_omen_thermal_profile())
>                 set_bit(PLATFORM_PROFILE_QUIET,
> platform_profile_handler.choices);
> 
> which seems a bit cleaner to me and hopefully works just as well
> or maybe even better.
> 
> Regards,
> 
> Hans
> 

The only thing I want to point out is that in this way the patch
enables a quiet profile on my non-omen notebook (HP Spectre x360 15-
df0006nl) that doesn't support it or at least the HP Command Center app
on windows doesn't offer it as choice.

Maybe this is a bug in windows implementation, if Jorge can kindly give
a feedback on this.

Regards,
Elia
> 
> 
> > ---
> >  drivers/platform/x86/hp-wmi.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/hp-wmi.c
> > b/drivers/platform/x86/hp-wmi.c
> > index bc7020e9df9e..3e0e67be8001 100644
> > --- a/drivers/platform/x86/hp-wmi.c
> > +++ b/drivers/platform/x86/hp-wmi.c
> > @@ -177,7 +177,8 @@ enum hp_thermal_profile_omen_v1 {
> >  enum hp_thermal_profile {
> >         HP_THERMAL_PROFILE_PERFORMANCE  = 0x00,
> >         HP_THERMAL_PROFILE_DEFAULT              = 0x01,
> > -       HP_THERMAL_PROFILE_COOL                 = 0x02
> > +       HP_THERMAL_PROFILE_COOL                 = 0x02,
> > +       HP_THERMAL_PROFILE_QUIET                        = 0x03,
> >  };
> >  
> >  #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) !=
> > HPWMI_POWER_FW_OR_HW)
> > @@ -1194,6 +1195,9 @@ static int hp_wmi_platform_profile_get(struct
> > platform_profile_handler *pprof,
> >         case HP_THERMAL_PROFILE_COOL:
> >                 *profile =  PLATFORM_PROFILE_COOL;
> >                 break;
> > +       case HP_THERMAL_PROFILE_QUIET:
> > +               *profile =  PLATFORM_PROFILE_QUIET;
> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > @@ -1216,6 +1220,9 @@ static int hp_wmi_platform_profile_set(struct
> > platform_profile_handler *pprof,
> >         case PLATFORM_PROFILE_COOL:
> >                 tp =  HP_THERMAL_PROFILE_COOL;
> >                 break;
> > +       case PLATFORM_PROFILE_QUIET:
> > +               tp =  HP_THERMAL_PROFILE_QUIET;
> > +               break;
> >         default:
> >                 return -EOPNOTSUPP;
> >         }
> > @@ -1230,6 +1237,8 @@ static int hp_wmi_platform_profile_set(struct
> > platform_profile_handler *pprof,
> >  static int thermal_profile_setup(void)
> >  {
> >         int err, tp;
> > +       unsigned int n_gpu = 0;
> > +       const struct dmi_device *dev = NULL;
> >  
> >         if (is_omen_thermal_profile()) {
> >                 tp = omen_thermal_profile_get();
> > @@ -1263,6 +1272,16 @@ static int thermal_profile_setup(void)
> >  
> >                 platform_profile_handler.profile_get =
> > hp_wmi_platform_profile_get;
> >                 platform_profile_handler.profile_set =
> > hp_wmi_platform_profile_set;
> > +
> > +               /*
> > +                * The quiet profile is available only on models
> > without dGPU,
> > +                * so we enable it when the device has only one GPU
> > onboard.
> > +                */
> > +               while ((dev = dmi_find_device(DMI_DEV_TYPE_VIDEO,
> > NULL, dev)))
> > +                       n_gpu++;
> > +
> > +               if (n_gpu == 1)
> > +                       set_bit(PLATFORM_PROFILE_QUIET,
> > platform_profile_handler.choices);
> >         }
> >  
> >         set_bit(PLATFORM_PROFILE_COOL,
> > platform_profile_handler.choices);
>