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?
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?
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");
>>>
>>
>
