Hi Joshua,
On Fri, Jan 10, 2025 at 04:59:16PM +0100, Joshua Grisham wrote:
> <snip>
> > > + /* if startup performance_mode fails to match a profile, try to set init mode */
> > > + err = get_performance_mode_profile(galaxybook, performance_mode, NULL);
> > > + if (err) {
> > > + if (init_performance_mode == GB_PERFORMANCE_MODE_UNKNOWN) {
> > > + dev_err(&galaxybook->platform->dev, "missing initial performance mode\n");
> > > + return -ENODATA;
> > > + }
> > > + err = performance_mode_acpi_set(galaxybook, init_performance_mode);
> > > + if (err) {
> > > + dev_err(&galaxybook->platform->dev,
> > > + "failed to set initial performance mode 0x%x\n",
> > > + init_performance_mode);
> > > + return err;
> >
> > ...why these two cases then result in failing everything vs. just platform
> > profile feature? Not an end of the world but it feels inconsistent to me.
> >
>
> I am glad you bring this up, as it forces me think through this a few
> more rounds... basically what happens is that the device will always
> come up from a fresh boot with the value of 0x0 as the "current
> performance mode" as response from the ACPI method, even though for
> most devices value 0x0 is the "legacy" optimized value that should not
> be used.
Is this 0x0 mode real or just a placeholder value? i.e. the device always
starts with legacy low-power?
>
> In Windows, the Samsung background apps take care of this by storing
> last-used value from previous session and then re-applying it after
> startup (and the same happens with various userspace services on
> platform profile from what I have seen, actually).
>
> But since the driver does not map 0x0 to any valid profile unless the
> device only has the "legacy" optimized mode, then my function
> get_performance_mode_profile() will return -ENODATA in this initial
> startup state of 0x0. What I noticed is that the first time after this
> that you run platform_profile_cycle() after this, there is a little
> "hiccup" and an error "Failed to get profile for handler .." is
> printed in the kernel log from platform_profile.c (without pr_fmt by
> the way), but then it just works anyway and starts to pick up from the
> first enabled profile and then can continue to cycle.
>
> My bit of code here was to basically try to "force" to set the profile
> to whatever was successfully mapped as "balanced" upon a fresh startup
> so that when platform_profile_cycle() first runs there would not be
> any condition where get_performance_mode_profile() would return
> -ENODATA. Then the userspace tools would take over anyway and restore
> your last session's latest used profile so it would not matter so
> much. In the end, really the aim I guess is to avoid this error
> message in the kernel log, but otherwise everything works even though
> there is an error message printed, but this is maybe a bit overkill?
> And by the way, that, as you say, we should probably not fail the
> entire feature just because of this, but let the error happen anyway
> and let everything work after that.
>
> Possibly more "simple" alternatives I can think of off the top of my head:
>
> 1. Let get_performance_mode_profile() return 0 instead of -ENODATA if
> nothing is matched , this way a non-mapped performance mode would
> always just set platform_profile_cycle() to the start of the cycle I
> guess/would hope? and/or add a special case in
> get_performance_mode_profile() for performance_mode=0 to just return 0
> to get the same effect ? (though what happens if we have not enabled
> PLATFORM_PROFILE_LOW_POWER in the choices? even though the function
> returned 0, will platform_profile see that 0 is not supported, and
> just keep moving on until it gets to the first one that is? If so then
> this will work, but I have not yet tested or fully checked the code to
> ensure that this will actually be the behavior...)
I don't know if this has been discussed before. If it was you should
follow their advice instead.
The platform_profile module doesn't enforce selected choices when
getting the current profile. Choices are only enforced when setting it.
I suggest that galaxybook_platform_profile_get() should map all known
values to valid platform profile options. Something like:
switch() {
case GB_PERFORMANCE_MODE_SILENT:
*profile = PLATFORM_PROFILE_LOW_POWER;
break;
case GB_PERFORMANCE_MODE_QUIET:
*profile = PLATFORM_PROFILE_QUIET;
break;
case GB_PERFORMANCE_MODE_OPTIMIZED:
case GB_PERFORMANCE_MODE_OPTIMIZED_LEGACY:
*profile = PLATFORM_PROFILE_BALANCED;
break;
case GB_PERFORMANCE_MODE_PERFORMANCE:
case GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY:
if (ultra)
*profile = PLATFORM_PROFILE_BALANCED_PERFORMANCE;
else
*profile = PLATFORM_PROFILE_PERFORMANCE;
break;
case GB_PERFORMANCE_MODE_ULTRA:
*profile = PLAFORM_PROFILE_PERFORMANCE;
break;
default:
return -ENODATA;
}
Also a quick question. Why isn't silent mapped to
PLATFORM_PROFILE_QUIET. Are there devices that support both
GB_PERFORMANCE_MODE_SILENT and GB_PERFORMANCE_MODE_QUIET? Are this modes
different in nature?
>
> 2. Try to do the logic which I did with this init_performance_mode,
> but in case init_profile_mode is not set, just skip it and let the
> error come from platform_profile_cycle() anyway and it should start to
> work. In this case I think it would be good if the user is maybe
> flagged somehow and create a bug for this, because I would want to be
> able to work with them to see what modes are reported by their device
> and see if the mapping needs to be updated in some way.
>
> 3. Do neither of these, remove everything with init_performance_mode,
> and just let platform_profile_cycle() fail upon startup and print the
> error message and then it should just start working after anyway.
>
> 4. Map 0x0 performance_mode to PLATFORM_PROFILE_CUSTOM in case the
I don't think CUSTOM should be used as a placeholder value.
> "legacy" mode with this value is not mapped, then the hotkey would not
> work to cycle the profile at first as the user would be forced to set
> the profile to a value via either a GUI or the sysfs interface before
> they can begin to use the hotkey to cycle the profile. Once they do
> this the very first time, then the userspace tools should kick in
> after this (upon every restart for example) to set the profile to the
> prior session's last used profile and then the hotkey will start to
> work to cycle the profile anyway in that session without any
> intervention from the user at all (so it is the very first time that
> they start their environment up, assuming that the prior value does
> not get cleared somehow due to some combo like the module being
> removed/blacklisted and then they restart, then add it back, etc, or
> some other corner-case situation?)
>
> I do think that something should change, maybe the most
> straight-forward are either 1 or 2 in this list, but not sure if there
> are any opinions or preferences on these or if there are other better
> options I did not think of here?
>
> > > +static ssize_t current_value_store(struct kobject *kobj, struct kobj_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct galaxybook_fw_attr *fw_attr =
> > > + container_of(attr, struct galaxybook_fw_attr, current_value);
> > > + struct samsung_galaxybook *galaxybook = fw_attr->galaxybook;
> > > +
> > > + bool value;
> >
> > Remove the extra empty line from within variable declarations.
> >
>
> Yes sorry this was just a miss when so much got moved around due to
> the big changes between v4 and v5; will fix this and the other small
> straight-forward issues for v6 :)
>
> > --
> > i.
>
> Thanks again!
> Joshua
