Re: [PATCH] platform/surface: Add platform profile driver
From: Maximilian Luz
Date: Thu Feb 11 2021 - 12:35:43 EST
On 2/11/21 5:31 PM, Hans de Goede wrote:
Hi,
On 2/11/21 5:17 PM, Maximilian Luz wrote:
On 2/11/21 4:56 PM, Hans de Goede wrote:
Hi,
On 2/8/21 10:38 PM, Maximilian Luz wrote:
On 2/8/21 9:27 PM, Hans de Goede wrote:
<snip>
+static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
+{
+ switch (p) {
+ case SSAM_TMP_PROFILE_NORMAL:
+ return PLATFORM_PROFILE_QUIET;
+
+ case SSAM_TMP_PROFILE_BATTERY_SAVER:
+ return PLATFORM_PROFILE_LOW_POWER;
+
+ case SSAM_TMP_PROFILE_BETTER_PERFORMANCE:
+ return PLATFORM_PROFILE_BALANCED;
+
+ case SSAM_TMP_PROFILE_BEST_PERFORMANCE:
+ return PLATFORM_PROFILE_PERFORMANCE;
+
+ default:
+ dev_err(&sdev->dev, "invalid performance profile: %d", p);
+ return -EINVAL;
+ }
+}
I'm not sure about the mapping which you have chosen here. I know that at least for
gnome there are plans to make this stuff available in the UI:
https://gitlab.gnome.org/Teams/Design/settings-mockups/-/blob/master/power/power.png
http://www.hadess.net/2020/09/power-profiles-daemon-new-project.html
Thanks for those links!
Notice there are only 3 levels in the UI, which will primarily be mapped to:
PLATFORM_PROFILE_LOW_POWER
PLATFORM_PROFILE_BALANCED
PLATFORM_PROFILE_PERFORMANCE
(with fallbacks to say QUIET for LOW_POWER of there is no LOW_POWER, but that
mostly is something for userspace to worry about).
Interesting, I wasn't aware of that. I was aware of Bastien's work
towards implementing user-space support for this but I hadn't yet looked
at it in detail (e.g. the "fallback to quiet" is new to me).
Note that the fallback stuff would not apply here, since you do provide
all 3 of low-power, balanced and performance. But the current way gnome
will handle this means that it will be impossible to select "normal" from
the GNOME ui which feels wrong.
And the power-profile-daemon will likely restore the last used setting on boot,
meaning with your mapping that it will always switch the profile away from
SSAM_TMP_PROFILE_NORMAL, which I assume is the default profile picked at boot ?
Pretty much, yeah. AFAICT booting doesn't reset it, but hard-resetting
the EC does. Same difference though.
So ideally we would map PLATFORM_PROFILE_BALANCED (which will be the default
GNOME / power-profile-daemon setting) to SSAM_TMP_PROFILE_NORMAL.
I know the ABI docs say that drivers should try to use existing values, but
this seems like a good case to add a new value or 2 to the PLATFORM_PROFILE enum.
During the discussion the following 2 options were given because some devices
may have more then one balanced profile:
PLATFORM_PROFILE_BALANCED_LOW_POWER:
balanced-low-power: Balances between low power consumption
and performance with a slight bias
towards low power
PLATFORM_PROFILE_BALANCED_PERFORMANCE:
balanced-performance: Balances between performance and low
power consumption with a slight bias
towards performance
I think it would be better to add 1 or both of these, if we add both
we could e.g. do the following mappings:
SSAM_TMP_PROFILE_BATTERY_SAVER -> PLATFORM_PROFILE_LOW_POWER
SSAM_TMP_PROFILE_NORMAL -> PLATFORM_PROFILE_BALANCED_LOW_POWER
SSAM_TMP_PROFILE_BETTER_PERFORMANCE -> PLATFORM_PROFILE_BALANCED_PERFORMANCE
SSAM_TMP_PROFILE_BEST_PERFORMANCE -> PLATFORM_PROFILE_PERFORMANCE
or we could do:
SSAM_TMP_PROFILE_BATTERY_SAVER -> PLATFORM_PROFILE_LOW_POWER
SSAM_TMP_PROFILE_NORMAL -> PLATFORM_PROFILE_BALANCED
SSAM_TMP_PROFILE_BETTER_PERFORMANCE -> PLATFORM_PROFILE_BALANCED_PERFORMANCE
SSAM_TMP_PROFILE_BEST_PERFORMANCE -> PLATFORM_PROFILE_PERFORMANCE
I'm not sure which is best, I hope you have a better idea of that then me.
I might even be wrong here and NORMAL might really be more about being QUIET
then it really being BALANCED ? In which case the mapping is fine as is.
I can only really speak on the behavior of my Surface Book 2. On that
device, the CPU is passively cooled, but the discrete GPU is actively
cooled, so I can actually only really talk about active cooling behavior
for the dGPU.
On that, at least, the normal (Windows calls this 'recommended') profile
feels like it targets quiet operation. Using the dGPU with that profile
pretty much ensures that the dGPU will be limited in performance by a
thermal limiter (around 75°C to 80°C; at least it feels that way), while
the fan is somewhat audible but definitely not at maximum speed.
Changing the profile to any higher profile (Windows calls those 'better
performance' and 'best performance'), the fan becomes significantly more
audible. I'm not entirely sure if the performance increase can solely be
attributed to cooling though.
As far as I've heard, that behavior seems to be similar on other devices
with fans for CPU cooling, but I can try to get some more feedback on
that.
Based on all of this, I thought that this would most resemble a 'quiet'
profile. But I'd also be fine with your second suggestion. Calling the
last two options 'balanced performance' and 'performance' might be a bit
closer to the Windows naming scheme. It doesn't seem like the normal
profile does much power limiting in terms of actually capping the power
limit of the dGPU, so I think calling this 'balanced' would also make
sense to me, especially in light of Gnome's defaults.
Ack.
So that means that this is going to need to have a preparation patch
adding the 2 balanced variants which I mention above. Can you take care
of that in the next version?
Sure. Already prepared a patch for the 'balanced-performance' one over at [1].
Just needs some squashing and I can send in an updated series. Do you also want
me to add the 'balanced-low-power' version? I'd have chosen 'balanced' and
'balanced-performance' in the new mapping, so there wouldn't be any driver
right now using that.
I see at [1] that for now you've just added 'balanced-performance' that is probably
best, since as you say atm there are no users for 'balanced-low-power' .
Perfect.
And since that prep. patch needs to go through Rafael's PM tree anyways,
maybe also throw in a patch to make ACPI_PLATFORM_PROFILE not user selectable
and use select on it in the thinkpad_acpi and ideapad_laptop drivers?
There's also already one at [1] for that just waiting to be sent :)
Nice, thank you!
[1]: https://github.com/linux-surface/kernel/commits/s/surface-platform-profile/next
The platform-profile bits which you have here all look good to me.
Thanks, I'll try to send that and an updated registry series in later today.
Regards,
Hans
+
+static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
+{
+ switch (p) {
+ case PLATFORM_PROFILE_LOW_POWER:
+ return SSAM_TMP_PROFILE_BATTERY_SAVER;
+
+ case PLATFORM_PROFILE_QUIET:
+ return SSAM_TMP_PROFILE_NORMAL;
+
+ case PLATFORM_PROFILE_BALANCED:
+ return SSAM_TMP_PROFILE_BETTER_PERFORMANCE;
+
+ case PLATFORM_PROFILE_PERFORMANCE:
+ return SSAM_TMP_PROFILE_BEST_PERFORMANCE;
+
+ default:
+ /* This should have already been caught by platform_profile_store(). */
+ WARN(true, "unsupported platform profile");
+ return -EOPNOTSUPP;
+ }
+}
+
+static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
+ enum platform_profile_option *profile)
+{
+ struct ssam_tmp_profile_device *tpd;
+ enum ssam_tmp_profile tp;
+ int status;
+
+ tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
+
+ status = ssam_tmp_profile_get(tpd->sdev, &tp);
+ if (status)
+ return status;
+
+ status = convert_ssam_to_profile(tpd->sdev, tp);
+ if (status < 0)
+ return status;
+
+ *profile = status;
+ return 0;
+}
+
+static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
+ enum platform_profile_option profile)
+{
+ struct ssam_tmp_profile_device *tpd;
+ int tp;
+
+ tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
+
+ tp = convert_profile_to_ssam(tpd->sdev, profile);
+ if (tp < 0)
+ return tp;
+
+ return ssam_tmp_profile_set(tpd->sdev, tp);
+}
+
+static int surface_platform_profile_probe(struct ssam_device *sdev)
+{
+ struct ssam_tmp_profile_device *tpd;
+
+ tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
+ if (!tpd)
+ return -ENOMEM;
+
+ tpd->sdev = sdev;
+
+ tpd->handler.profile_get = ssam_platform_profile_get;
+ tpd->handler.profile_set = ssam_platform_profile_set;
+
+ set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
+ set_bit(PLATFORM_PROFILE_QUIET, tpd->handler.choices);
+ set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
+ set_bit(PLATFORM_PROFILE_PERFORMANCE, tpd->handler.choices);
+
+ platform_profile_register(&tpd->handler);
+ return 0;
+}
+
+static void surface_platform_profile_remove(struct ssam_device *sdev)
+{
+ platform_profile_remove();
+}
+
+static const struct ssam_device_id ssam_platform_profile_match[] = {
+ { SSAM_SDEV(TMP, 0x01, 0x00, 0x01) },
+ { },
+};
+MODULE_DEVICE_TABLE(ssam, ssam_platform_profile_match);
+
+static struct ssam_device_driver surface_platform_profile = {
+ .probe = surface_platform_profile_probe,
+ .remove = surface_platform_profile_remove,
+ .match_table = ssam_platform_profile_match,
+ .driver = {
+ .name = "surface_platform_profile",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+};
+module_ssam_device_driver(surface_platform_profile);
+
+MODULE_AUTHOR("Maximilian Luz <luzmaximilian@xxxxxxxxx>");
+MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
+MODULE_LICENSE("GPL");