Message-ID: <CAMF+KeZ=iUQzunGUdv3-=PLC+i7JpO82bUtKp02-PsothThgug@mail.gmail.com>
Date: Sat, 18 Jan 2025 20:51:39 +0100
From: Joshua Grisham <josh@...huagrisham.com>
To: Kurt Borja <kuurtb@...il.com>
Cc: W_Armin@....de, thomas@...ch.de, ilpo.jarvinen@...ux.intel.com, 
	hdegoede@...hat.com, platform-driver-x86@...r.kernel.org, corbet@....net, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] platform/x86: samsung-galaxybook: Add
 samsung-galaxybook driver

Hi Kurt, thanks for getting back so fast on this!

Den lör 18 jan. 2025 kl 08:04 skrev Kurt Borja <kuurtb@...il.com>:
>
> Hi Joshua,
>
> I have some comments on the platform profile section. The most important
> one is the platform_profile probe one, the rest are suggetions.
>
> ...
> > +static int galaxybook_platform_profile_probe(void *drvdata, unsigned long *choices)
> > +{
> > +     struct samsung_galaxybook *galaxybook = drvdata;
> > +     int i;
> > +
> > +     for_each_set_bit(i, galaxybook->platform_profile_choices, PLATFORM_PROFILE_LAST)
> > +             set_bit(i, choices);
>
> The intended use of this callback is to "probe" for available choices
> here. You should move all galaxybook_performance_mode_profile_init()
> logic to this method. This would eliminate the need to have a copy of
> the choices bitmap.
>

Yes at first I was really thinking a lot about how to cleanly break
apart the logic as I wanted it to work so that if this feature is not
supported on the particular device (e.g. the ACPI methods return an
error code, so you can assume that this feature does not exist on this
device, as is the case for SAM0427 devices for example, possibly
others that I am not aware of..) that we would not even try to create
the platform profile device.

I think I have decided now that maybe a fair amount of this was sub
optimization and I will instead now just try to do a "get" of the
current performance_mode using the ACPI method as a sort of litmus
test if the feature is working or not (just exactly as I have done on
most of the other features) in the galaxybook_platform_profile_init;
if that fails, then I will exit gracefully without trying to create
the platform_profile (return 0), otherwise I will pass all of the rest
to the probe function and if it fails, it fails :) (because, remember,
in theory we were able to successfully get the value using ACPI so the
feature *should* work at that point, right?)

> > +static int galaxybook_performance_mode_init(struct samsung_galaxybook *galaxybook)
> > +{
> > +     enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
> > +     u8 performance_mode;
> > +     int err;
> > +     int i;
> > +
> > +     err = performance_mode_acpi_get(galaxybook, &performance_mode);
> > +     if (err)
> > +             return err;
> > +
> > +     err = get_performance_mode_profile(galaxybook, performance_mode, &profile);
> > +     if (err)
>
> If this method failed we can't safely continue. I think you should
> return here, else you may get an out of bounds access bellow.
>
> > +             dev_warn(&galaxybook->platform->dev,
> > +                      "initial startup performance mode 0x%x is not mapped\n",
> > +                      performance_mode);
> > +
> > +     for_each_set_bit(i, galaxybook->platform_profile_choices, PLATFORM_PROFILE_LAST)
> > +             dev_dbg(&galaxybook->platform->dev,
> > +                     "enabled platform profile %d using performance mode 0x%x\n",
> > +                     i, galaxybook->profile_performance_modes[i]);
>
> Maybe we can log this directly in the switch-case block inside
> galaxybook_performance_mode_profile_init() instead of having to iterate.
>

Both of these (and the entirety of how this
galaxybook_performance_mode_init() is working) I also realized are sub
optimizations .. the only thing they are really helping is the case
where someone loads removes and then loads the module again (I did not
want that their performance_mode should be updated if they had already
set a valid one previously in their session). But, yeah, that is a
super corner case and the code should not be extra complicated for
that (is what I am now thinking instead, anyway), so I think now I
will just force set the initial performance_mode on probe to what was
eventually mapped as Balanced and if someone is removing and reloading
the module in their session, they are probably capable of dealing with
the profile being reset to balanced each time :)

(Existing userspace tools like power-profiles-daemon etc are taking
care of "restoring" your session's last-used profile after startup so
even after this init sets to Balanced then userspace tools will kick
in to set it to something else later that you probably wanted/last
used anyway; this has been my experience, anyway!, so I am not worried
about setting to Balanced at Init but the reason to set anything is
because of how the ACPI Get method always responds with 0 after
startup, so I just want that it sets something that should actually be
used according to the result of the mapping -- same behavior as the
Samsung Windows apps).

> > +     /*
> > +      * Value returned in iob0 will have the number of supported performance
> > +      * modes per device. The performance mode values will then be given as a
> > +      * list after this (iob1-iobX). Loop through the supported values and
> > +      * enable their mapped platform_profile choice, overriding "legacy"
> > +      * values along the way if a non-legacy value exists.
> > +      */
> > +     for (i = 1; i <= buf.iob0; i++) {
> > +             dev_dbg(&galaxybook->platform->dev,
> > +                     "device supports performance mode 0x%x\n", buf.iob_values[i]);
> > +             err = get_performance_mode_profile(galaxybook, buf.iob_values[i], &profile);
>
> Here we pass iob_values[i] through a switch-case block inside
> get_performance_mode_profile() and then we do it again bellow. Maybe
> this all could be done here, without having to call
> get_performance_mode_profile().
>

Yes I have cleaned this up in a new draft now also and re-shaped a bit
of how the debug messages look so it hopefully makes more sense and
there are not multiple unnecessary loops.

Note: the whole point of having the messages printed out to debug was
to help with troubleshooting as new devices or new BIOS updates make
things confusing, as I have seen this problem with several different
users and devices while initially writing the logic for this driver
out of tree... so they do help a lot!

> > +             if (err)
> > +                     continue;
> > +             switch (buf.iob_values[i]) {
> > +             case GB_PERFORMANCE_MODE_OPTIMIZED:
> > +                     /* override legacy Optimized value */
> > +                     gb_pfmode(profile) = GB_PERFORMANCE_MODE_OPTIMIZED;
> > +                     break;
> > +             case GB_PERFORMANCE_MODE_PERFORMANCE:
> > +                     /* override legacy Performance value */
> > +                     gb_pfmode(profile) = GB_PERFORMANCE_MODE_PERFORMANCE;
> > +                     break;
> > +             case GB_PERFORMANCE_MODE_ULTRA:
> > +                     /*
> > +                      * if Ultra is supported, downgrade performance to
> > +                      * balanced-performance
> > +                      */
>
> I haven't been following the entire discussion, so I don't know if Armin
> changed his mind but I agree with him.
>
> I think GB_PERFORMANCE_MODE_PERFORMANCE_LEGACY should be statically
> mapped to BALANCED_PERFORMANCE and TURBO should be PERFORMANCE. This
> would simplify a lot of the logic here.
>

I was a bit worried about this, especially how it would potentially
interplay with other things such as power-profiles-daemon when
bringing in intel_pstate or anything else (it did not sit very well
for me to have a mode on my device that really felt like "High
Performance" but only call it "Balanced Performance" and worried that
it could be aggregated in the wrong way which could impact other
things etc etc .... )

BUT.. none of that matters now, as I have some news on this front: I
did a bit more digging and a bit more homework to really think through
this again, including going back through all of my notes and feedback
plus various log files given by multiple different users, for multiple
different devices in
https://github.com/joshuagrisham/samsung-galaxybook-extras/issues/31,
and then did some detective work looking up how the Samsung Settings
app looks for Ultra devices (looking where reviewers even posted
screenshots of the settings application for these devices in Windows,
etc...).

What I came to realize is that the Ultra devices do not use BOTH
"Performance" and "Ultra", but instead it actually just re-maps the
value for "Ultra" to use AS "Performance". So, they only have 3 modes
in Windows: "Quiet", "Optimized", and "High Performance" (they do not
have a fourth option for "Ultra"). The right solution then is to
re-map so that when a user selects PLATFORM_PROFILE_PERFORMANCE on an
Ultra device, it should send the "Ultra" Galaxy Book performance mode
to the ACPI method.

This should then support a lot more "hard coding" of the mapping and I
have cleaned it up significantly due to this.

Also, I have now tweaked the names of all of the internal symbols to
more closely match the internal names of these modes used by Samsung's
driver and services in Windows so that it will be easier to
troubleshoot and make sense of in the future, as well (e.g. what I had
called "Silent" they actually call "FanOff" etc..). I think it will
help a lot to just re-use their existing names instead of having yet
another "mapping layer" to sort through while troubleshooting.

> > +                     if (test_bit(PLATFORM_PROFILE_PERFORMANCE,
> > +                                  galaxybook->platform_profile_choices)) {
> > +                             gb_pfmode(PLATFORM_PROFILE_BALANCED_PERFORMANCE) =
> > +                                     gb_pfmode(PLATFORM_PROFILE_PERFORMANCE);
> > +                             set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE,
> > +                                     galaxybook->platform_profile_choices);
> > +                     }
> > +                     /* override performance profile to use Ultra's value */
> > +                     gb_pfmode(profile) = GB_PERFORMANCE_MODE_ULTRA;
> > +                     break;
> > +             default:
> > +                     break;
> > +             }
> > +             set_bit(profile, galaxybook->platform_profile_choices);
> > +     }
> > +
> > +     err = galaxybook_performance_mode_init(galaxybook);
>
> If the main goal of this method is to set an initial profile maybe we
> can just directly set it after finding GB_PERFORMANCE_MODE_OPTIMIZED?
>
> This would eliminate a bit of indirection.
>
> Do you know if all devices support OPTIMIZED? either legacy or
> non-legacy.
>

Yes as mentioned above I have decided to rip out all of the sub
optimization here and do as you say: just set the value mapped to
PLATFORM_PROFILE_BALANCED after all of the mapping values are updated
(as long as it was set it in `choices`) and move on from there. I have
not seen any device so far that does not support at least one of the
Optmized modes, but by checking if PLATFORM_PROFILE_BALANCED is set
before using it then the worst that can happen is that the startup
value is something new not actually currently mapped in
get_performance_mode_profile() and it will be "fixed" the next time a
platform_profile_cycle() runs and sets everything back on track.

Does that sound ok or do you think it is better to try and leave in
some of this conditional / detection logic like was here in v7 that I
am now removing?

> > +static int galaxybook_platform_profile_init(struct samsung_galaxybook *galaxybook)
> > +{
> > +     struct device *ppdev;
> > +     int err;
> > +
> > +     err = galaxybook_performance_mode_profile_init(galaxybook);
> > +     if (err)
> > +             return 0;
> > +
> > +     galaxybook->has_performance_mode = true;
> > +
> > +     ppdev = devm_platform_profile_register(&galaxybook->platform->dev, DRIVER_NAME,
> > +                                            galaxybook, &galaxybook_platform_profile_ops);
> > +     if (IS_ERR(ppdev))
> > +             return PTR_ERR(ppdev);
> > +
> > +     platform_profile_notify(ppdev);
>
> No need to notify after registering. You can directly return
> PTR_ERR_OR_ZERO().
>
> ~ Kurt
>
> > <snip>

I think the reason I was thinking to notify was because we are
changing this performance mode during init but I see now that it is
already done in platform_profile_register() so yes I will clean this
up as well :)

v8 coming shortly, hopefully we are getting there!

Thanks again and regards,
Joshua

Powered by blists - more mailing lists