Re: [PATCH 2/2] platform/x86: hp-wmi: Add thermal profile support for 8BAD boards

[Alexis Belmonte <alexbelm48@gmail.com>]

On Thu, Jan 04, 2024 at 08:00:15PM +0200, Ilpo Järvinen wrote:
> On Sun, 31 Dec 2023, Alexis Belmonte wrote:
> 
> > Add 8BAD to the list of boards which have thermal profile selection
> > available. This allows the CPU to draw more power than the default TDP
> > barrier defined by the 'balanced' thermal profile (around 50W), hence
> > allowing it to perform better without being throttled by the embedded
> > controller (around 130W).
> > 
> > We first need to set the HP_OMEN_EC_THERMAL_PROFILE_TIMER_OFFSET to zero.
> > This prevents the timer countdown from reaching zero, making the embedded
> > controller "force-switch" the system's thermal profile back to 'balanced'
> > automatically.
> > 
> > We also need to put a number of specific flags in
> > HP_OMEN_EC_THERMAL_PROFILE_FLAGS_OFFSET when we're switching to another
> > thermal profile:
> > 
> >    - for 'performance', we need to set both HP_OMEN_EC_FLAGS_TURBO and
> >      HP_OMEN_EC_FLAGS_NOTIMER;
> > 
> >    - for 'balanced' and 'powersave', we clear out the register to notify
> >      the system that we want to lower the TDP barrier as soon as possible.
> > 
> > The third flag defined in the hp_thermal_profile_omen_flags enum,
> > HP_OMEN_EC_FLAGS_JUSTSET, is present for completeness.
> 
> I don't understand this. Based on the name of the flag, I'd have expected 
> it has to be set (but its purpose is not known), but the code below does 
> not do that?

It does not indeed, that's what I meant by 'completeness'. I defined it as a way
to show that I acknowledged the existence of that bit in the register, but I do
not use it.

I can comment it (or even remove it) if necessary.

On Thu, Jan 04, 2024 at 08:00:15PM +0200, Ilpo Järvinen wrote:
> > To prevent potential behaviour breakage with other Omen models, a
> > separate omen_timed_thermal_profile_boards array has been added to list
> > which boards expose this behaviour.
> > 
> > Performance benchmarking was done with the help of silver.urih.com and
> > Google Chrome 120.0.6099.129, on Gnome 45.2, with the 'performance'
> > thermal profile set:
> > 
> > |                  | Performance |     Stress |   TDP |
> > |------------------|-------------|------------|-------|
> > |    with my patch |      P84549 |    S0.1891 |  131W | 
> > | without my patch |      P44084 |    S0.1359 |   47W |
> > 
> > The TDP measurements were done with the help of the s-tui utility,
> > during the load.
> > 
> > There is still work to be done:
> > 
> >    - tune the CPU and GPU fans to better cool down and enhance
> >      performance at the right time; right now, it seems that the fans are
> >      not properly reacting to thermal/performance events, which in turn
> >      either causes thermal throttling OR makes the fans spin way too long,
> >      even though the temperatures have lowered down
> > 
> >    - expose the CPU and GPU fan curves to user-land so that they can be
> >      controlled just like what the Omen Gaming Hub utility proposes to
> >      its users;
> > 
> > Signed-off-by: Alexis Belmonte <alexbelm48@gmail.com>
> > ---
> >  drivers/platform/x86/hp/hp-wmi.c | 63 +++++++++++++++++++++++++++++++-
> >  1 file changed, 61 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> > index 95282c3a02ed..79caf5d79e05 100644
> > --- a/drivers/platform/x86/hp/hp-wmi.c
> > +++ b/drivers/platform/x86/hp/hp-wmi.c
> > @@ -38,6 +38,8 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45E9-BE91-3D44E2C707E4");
> >  #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
> >  #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> >  
> > +#define HP_OMEN_EC_THERMAL_PROFILE_FLAGS_OFFSET 0x62
> > +#define HP_OMEN_EC_THERMAL_PROFILE_TIMER_OFFSET 0x63
> >  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> >  
> >  #define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use when zero insize is required
> > @@ -57,7 +59,7 @@ static const char * const omen_thermal_profile_boards[] = {
> >  	"874A", "8603", "8604", "8748", "886B", "886C", "878A", "878B", "878C",
> >  	"88C8", "88CB", "8786", "8787", "8788", "88D1", "88D2", "88F4", "88FD",
> >  	"88F5", "88F6", "88F7", "88FE", "88FF", "8900", "8901", "8902", "8912",
> > -	"8917", "8918", "8949", "894A", "89EB"
> > +	"8917", "8918", "8949", "894A", "89EB", "8BAD"
> >  };
> >  
> >  /* DMI Board names of Omen laptops that are specifically set to be thermal
> > @@ -68,6 +71,14 @@ static const char * const omen_thermal_profile_force_v0_boards[] = {
> >  	"8607", "8746", "8747", "8749", "874A", "8748"
> >  };
> >  
> > +/* DMI board names of Omen laptops that have a thermal profile timer which will
> > + * cause the embedded controller to set the thermal profile back to
> > + * "balanced" when reaching zero.
> > + */
> > +static const char * const omen_timed_thermal_profile_boards[] = {
> > +	"8BAD"
> > +};
> > +
> >  /* DMI Board names of Victus laptops */
> >  static const char * const victus_thermal_profile_boards[] = {
> >  	"8A25"
> > @@ -184,6 +194,12 @@ enum hp_thermal_profile_omen_v1 {
> >  	HP_OMEN_V1_THERMAL_PROFILE_COOL		= 0x50,
> >  };
> >  
> > +enum hp_thermal_profile_omen_flags {
> > +	HP_OMEN_EC_FLAGS_TURBO		= 0x04,
> > +	HP_OMEN_EC_FLAGS_NOTIMER	= 0x02,
> > +	HP_OMEN_EC_FLAGS_JUSTSET	= 0x01,
> > +};
> > +
> >  enum hp_thermal_profile_victus {
> >  	HP_VICTUS_THERMAL_PROFILE_DEFAULT		= 0x00,
> >  	HP_VICTUS_THERMAL_PROFILE_PERFORMANCE		= 0x01,
> > @@ -451,7 +467,11 @@ static int hp_wmi_get_tablet_mode(void)
> >  
> >  static int omen_thermal_profile_set(int mode)
> >  {
> > -	char buffer[2] = {0, mode};
> > +	/* The Omen Control Center actively sets the first byte of the buffer to
> > +	 * 255, so let's mimic this behaviour to be as close as possible to
> > +	 * the original software.
> > +	 */
> > +	char buffer[2] = {-1, mode};
> >  	int ret;
> >  
> >  	ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
> > @@ -1203,6 +1221,28 @@ static int platform_profile_omen_get(struct platform_profile_handler *pprof,
> >  	return 0;
> >  }
> >  
> > +static bool has_omen_thermal_profile_ec_timer(void)
> > +{
> > +	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> > +
> > +	if (!board_name)
> > +		return false;
> > +
> > +	return match_string(omen_timed_thermal_profile_boards,
> > +			    ARRAY_SIZE(omen_timed_thermal_profile_boards),
> > +			    board_name) >= 0;
> > +}
> > +
> > +inline int omen_thermal_profile_ec_flags_set(enum hp_thermal_profile_omen_flags flags)
> > +{
> > +	return ec_write(HP_OMEN_EC_THERMAL_PROFILE_FLAGS_OFFSET, flags);
> > +}
> > +
> > +inline int omen_thermal_profile_ec_timer_set(char value)
> 
> u8 ?
> 
> I know this driver uses char in many place but if this is binary data, u8 
> is the correct type (and the other char instances should be converted 
> to u8 too eventually).
>

Alright, I did not know about this. I am a first timer to this driver
and the whole Linux kernel source code, so I'm happy to learn things
from this experience! :]

> > +{
> > +	return ec_write(HP_OMEN_EC_THERMAL_PROFILE_TIMER_OFFSET, value);
> > +}
> > +
> >  static int platform_profile_omen_set(struct platform_profile_handler *pprof,
> >  				     enum platform_profile_option profile)
> >  {
> > @@ -1240,6 +1280,24 @@ static int platform_profile_omen_set(struct platform_profile_handler *pprof,
> >  	if (err < 0)
> >  		return err;
> >  
> > +	if (has_omen_thermal_profile_ec_timer()) {
> > +		err = omen_thermal_profile_ec_timer_set(0);
> > +		if (err < 0)
> > +			return err;
> > +
> > +		enum hp_thermal_profile_omen_flags flags;
> 
> Please put this to the usual place (beginning of the block or function).
> I know it's now possible to place them anywhere because of what cleanup.h 
> requires but lets not start to spread them all around just because we can,
> it makes finding the declaration harder, you could also initialize to 0 
> so you don't need the else branch below.
> 
> > +
> > +		if (profile == PLATFORM_PROFILE_PERFORMANCE)
> > +			flags = HP_OMEN_EC_FLAGS_NOTIMER |
> > +				HP_OMEN_EC_FLAGS_TURBO;
> > +		else
> > +			flags = 0;
> > +
> > +		err = omen_thermal_profile_ec_flags_set(flags);
> > +		if (err < 0)
> > +			return err;
> > +	}
> > +
> >  	return 0;
> >  }
> 
OK, this should be fixed now.
> 
> -- 
>  i.
> 

Thank you for reviewing my 2nd patch -- I've also noticed that I've missed
another detail in my first patch, so I'll send both of them right after this
e-mail so as to separate all of this from my answers to your questionings and
remarks.

I might be slightly less reactive for one or two weeks, as I've started my 2nd
year at my IT school; but I won't go away, as the laptop that I'm implementing
support for is also the same laptop that I daily drive, so I'm pretty much
obligated to finish this, soon or later! :]

Alexis