Hi all,
More emails came in since I wrote this....but I'm going to send anyway
and catch up with those after. I need to write faster :)
On 9/17/2020 7:50 AM, Bastien Nocera wrote:
Hey,
On Thu, 2020-09-17 at 13:22 +0200, Hans de Goede wrote:
Hi Elia, Mark, et al.
Elia, Mark I'm mailing you both because both of you have pdx86
patches pending to add a vendor
specific sysfs-attribute for selecting performance-profiles for resp.
HP and Lenovo Thinkpad laptops.
I think that this shows that we might need to start thinking
about a generic kernel API for this, otherwise we will
end up with slight different options per vendor ...
Some comments below based on possible use in power-profiles-daemon:
https://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
So it seems we may need something like:
/sys/class/system_performance_profile
Where we would then get e.g.:
/sys/class/system_performance_profile/thinkpad_acpi/performance_profi
le
And then we need to standardize on the names/values which
performance_profile can show / accept when written too.
The big question is what do we do if there are more then 3 profiles?
The Intel P-State driver in the kernel supports 4 separate ones (plus
default):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/cpufreq/intel_pstate.c#n591
which we crammed into 3 profiles:
https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/blob/master/src/ppd-driver-intel-pstate.c#L209-226
One option would be something like the following:
cat
/sys/class/system_performance_profile/thinkpad_acpi/performance_profi
le
low-power [balanced] performance
Are the square brackets to show the currently selected profile? I'd
rather it was a separate sysfs attribute. I would expect to only ever
read the list of supported profiles once, and then monitor an "active
profile" attribute.
(a bit like the intel_pstate kernel driver does, but then all the
devices that support Intel P-State support all the profiles, so it's
not a good example ;)
cat
/sys/class/system_performance_profile/thinkpad_acpi/extra_performance
_profiles
extra-low-power balanced-performance-mix
So we add an optional extra_performance_profiles sysfs attribute and
we ask all
drivers implemeting this class to implement at least the 3 standard
profiles
(by mapping 3 of their options to these) and optional they can offer
extra
profiles (with free form names) in the extra_performance_profiles
sysfs attribute under the class-device.
The idea behind putting the extra profiles in a separate sysfs-
attribute
is that reading the main performance_profile attribute will always
show
one selected, even if one of the extra profiles is actually in use,
then the driver should also show the closest standardized profile as
being active.
I think it's fine having more than 3 profiles. Something like power-
profiles-daemon would likely trying to match them all to one of the 3
profiles it uses as an interface, or forcing the use of those 3
profiles, depending on what that profile behaves.
This will allow userspace code to always rely on the standard
interface
both for getting a representation of the currently active profile as
well
as for setting the active profile.
Elia, Mark, I assume that both of you want to get your patches for
this
upstream sooner, rather then later. But I think we should put them on
hold until we have an agreement on a shared userspace API for this.
Compared to the WIP lenovo-dytc "perfmode" driver, we're missing
something to advertise the unavailability of a profile, and the reason
for that unavailability.
In all honesty I was slightly dreading this email :) I know the similar
issue killed our ePrivacy patch...but I fully appreciate that is part of
open source contribution
So yes - I agree that having a common interface would be a good idea and
making it common between the vendors makes sense. Let me know how to
contribute and make that happen.
From Lenovo's firmware point of view - our three settings should map on
to this quite closely with the exception that we have one setting that
covers balance_power and power (I never understood why the FW team did
that - as they have the four states in Windows - I wasn't able to get a
satisfactory answer to that question)
Cheers
I would like to think that the above proposal is a good start,
if we can quickly (*) decide on an userspace API here
Yes and understood. Let me know what is the best place to make this
happen - from my point of view the main aim is to get this to our users
to make the whole performance mode implemetnation more usable and
obvious. Without my proposed patch it's really hard to tell what mode
you are in on our platforms (and leads to lots of support questions).
I'm particularly aware of the eprivacy patch where that got rejected for
a generic solution that was under development - but the person working
on the generic solution stopped part way through to work on other
things. We didn't have the knowledge or experience of the graphics
driver to be able to really go and contribute effectively so for now
that feature is dead even though our initial patch was fairly simple. It
is still disappointing that our users don't get useful functionality
(and I also have to argue with marketing as to whether we can sell Linux
systems with ePrivacy screens which is no fun - I spend way too much of
my life doing Lenovo internal paperwork).
I figure on this item it's less complicated (not tied into the graphics
drivers details) so I hope I can contribute more directly - let me know
if I'm being naive.
One question - the main reason for a common interface is for user space
to not deal with a mess of APIs. Is it worth me doing a simplified
version of my patch (maybe using debugfs?) so I can expose the modes to
users whilst we work on the common solution? I'm assuming there is no
mileage in getting my patch (with the fix I owe Benjamin) in and then
changing it in the future once the generic solution is available as that
potentially messes up userspace too much? Something as a stopgap measure
that won't annoy the kernel community but is good for Linux users as I'm
guessing the generic solution is likely to be months away
Let me know what you recommend as the next steps.
Regards,
Hans
p.s.
I guess we should also add an optional lap_mode sysfs attribute
to the class-device, to have all the info for the Thinkpads in
one place.
I'm good with this too - but the lapmode patch is accepted and there is
the palm sensor patch too which I'm hoping is accepted soon. Whilst I'm
happy to make them part of this implementation (if they fit) I'd
appreciate if they didn't get removed or held up as they're needed for
our WWAN implementation which is already overdue.
The main consumer there will be our WWAN enablement utility and we can
change that to support different API if needs be :)
*) but not too quickly, it is important we get this right
