Date:   Thu, 11 Feb 2021 17:34:26 +0100
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Bastien Nocera <hadess@...ess.net>
Cc:     Mark Gross <mgross@...ux.intel.com>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/surface: Add platform profile driver



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@...il.com>");
>>>>>> +MODULE_DESCRIPTION("Platform Profile Support for Surface System Aggregator Module");
>>>>>> +MODULE_LICENSE("GPL");
>>>>>>
>>>>>
>>>>
>>>
>>
> 

Powered by blists - more mailing lists