On Mon, 13 Jan 2025, Julien ROBIN wrote:
> Hello and thank you very much for the feedback.
>
> I reworked my changes accordingly, sorry I took some time on it.
> I'm going to send a corrected [Patch v2] soon, so that the changes may
> be reviewed (as it's my first patch submission, your feedback is
> welcome).
>
> I'm going to list the changes into the [Patch v2] description, but as
> remainder, you can find my answers to your feedback and suggestions
> below.
>
> On 08/01/2025 11:08 AM, Ilpo Järvinen wrote:
>
> >> +static int victus_s_set_cpu_pl1_pl2(char pl1, char pl2)
> >
> > Is char correct type here or should you use like u8? Given you use 0xff
> > below I highly thing it's not a real character at all but generic 8-bit
> > data that should be u8.
> >
> >> +{
> >> + /* Placing 0xFF in the 2 last bytes tells we're setting PL1 and PL2 */
> >> + char buffer[4] = { pl1, pl2, 0xFF, 0xFF };
> >
> > correct type?
> >
> > I see lots of other char use elsewhere too in this patch, please check
> > those too that it's correct.
>
> You're right, usage of char variable types was just copy of what I saw
> into existing functions; but used as unsigned numeric 8-bit values, u8
> variable type is indeed more relevant.
> So I changed my "char" additions to "u8" as you suggested.
Unfortunately in the past the developers have not paid too much
attention to this difference so we're moving only (very) slowly towards
consistency.
> >> +static int hp_wmi_fan_speed_reset(void)
> >> +{
> >> + int ret;
> >> + char fan_speed[2] = { 0 }; /* Restores automatic speed */
> >
> > Instead of a comment, add a named define for the value?
>
> >> +
> >> + if (pl1 == 0xFF || pl2 == 0xFF)
> >> + return -EINVAL; /* the 0xFF value has a special meaning */
> >
> > Could that "special meaning" be labeled with a named define?
> > (I understand you might not know anything beyond it being "special" but
> > if you know a more precise meaning, a define would be much better than a
> > literal.)
>
> I dug a little more about the observed special behavior of 0 and 0xFF
> values for these HP WMI queries.
>
> It turns out 0xFF is more exactly used as "ignore / don't change"
> parameter for HPWMI_SET_POWER_LIMITS_QUERY (0x29).
> Also, 3rd and 4th parameters for HPWMI_SET_POWER_LIMITS_QUERY turned out
> to be PL4, and CPU + GPU concurrent TDP, so for these 4 bytes, I've
> changed the buffer[4] to a struct with more clearly expressed names. In
> order to be consistent, I did the same work for GPU power modes.
>
> For the named define for those values, I was about to propose the following ones:
>
> #define HP_VICTUS_ARG_VALUE_RESTORE_DEFAULT 0x00
> #define HP_VICTUS_ARG_VALUE_KEEP_UNCHANGED 0xFF
>
> However these 2 statements wouldn't always be true and could be
> deceptive for other settings, as it turns out using 0 or 0x00 as
> arguments for others WMI queries (others than fan speed and CPU power
> limits settings) tends to set 0 as actual value instead of restoring
> default one, and same goes for 0xFF.
>
> So for now I just moved the comment as function level comment, as you
> suggested below, but in case you had something else in mind, I'm of
> course open to another approach.
You could add comment before the defines to tell which queries are known
to use those values. It would still be very much preferrable to have those
literals named so the code will become self-explanatory and those comments
wouldn't be neede anymore.
> >> + if (pl2 < pl1)
> >> + return -EINVAL; /* PL2 is not supposed to be lower than PL1 */
> >
> > Please move those two comments before the if () line.
> >
> >> +
> >> + /* Note: providing 0x00 as PL1 and PL2 is restoring default values */
> >> +
> >
> > That comments sounds like something that could be a function level comment
> > instead.
>
> That's right, I moved those comments as you suggested.
>
> >> +static int platform_profile_victus_s_get(struct platform_profile_handler
> >> *pprof,
> >> + enum platform_profile_option
> >> *profile)
> >> +{
> >> + /* Same behaviour as platform_profile_omen_get */
> >> + return platform_profile_omen_get(pprof, profile);
> >> +}
> >
> > Just use platform_profile_omen_get() directly then instead?
>
> Yes, I was copying again another approach I saw; but I indeed asked
> myself the same question.
> So I removed this one and made platform_profile_omen_get() called instead.
>
>
> Finally, while investigating about 0x00 and 0xFF special effects, I also
> dug a little more about a 0x57 fixed magic 4th byte used when applying
> GPU power modes (0x57 is 87 in decimal). It turned out to be a GPU
> slowdown temperature threshold, sometimes called GPS, that didn't need
> to be hard coded: despite being unable to use 0x00 and 0xFF to deal with
> it (was applying 0°C and 255°C), I successfully added
> HPWMI_GET_GPU_THERMAL_MODES_QUERY and associated function, and now the
> current setting for this value can be retrieved prior to overwrite.
> So into the [Patch v2] this is used to retrieve and reuse the current
> value (which is confirmed to be 87 in my case after a power cycle)
> instead of using an hard coded value, before overwriting GPU power
> modes. I believe it's better that way, in case others machines are using
> another value in the future.
Great, thanks for looking into it! :-)
--
i.
