Re: [PATCH v1 13/15] platform/x86/amd/pmf: Handle AMT and CQL events for Auto mode

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 7/28/2022 13:17, Hans de Goede wrote:
> Hi,
> 
> On 7/28/22 20:06, Limonciello, Mario wrote:
>> On 7/28/2022 12:46, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 7/28/22 16:38, Limonciello, Mario wrote:
>>>>
>>>>>>> 1. If I understand things right, then on ThinkPads /sys/firmware/apci/platform_profile
>>>>>>>        will be registered by thinkpad_acpi. But in version 1 of this patchset nothing is
>>>>>>>        stopping the amd-pmf code from registering /sys/firmware/apci/platform_profile if
>>>>>>>        the amd-pmf module gets loaded first. So if the intend is for it to always be owned
>>>>>>>        by thinkpad_acpi then the amd-pmf code must check for this and not even try to
>>>>>>>        register its platform_profile support. We cannot rely on module ordering ensuring
>>>>>>>        that thinkpad_acpi registers first and then amd-pmf will get an -EBUSY error,
>>>>>>>        since there are no module load ordering guarantees.
>>>>>>
>>>>>> This was my thought initially too while this was being developed, but actually there is some nuance here that is non-obvious.  The platform profile registering code in amd-pmf will examine bits set in the BIOS to decide whether or not to export platform profile support.  In Lenovo platforms that support thinkpad_acpi these bits are not set.  So platform profile support ONLY comes from thinkpad-acpi in those platforms.
>>>>>
>>>>> Right, Shyam mentioned this in another part of the thread. As I
>>>>> mentioned there IHMO it would still be good to check this in the driver
>>>>> though. To catch cases where a BIOS for some reasons advertises an
>>>>> unexpected combination of features.
>>>>>
>>>>>>> 2. So when the thinkpad_acpi platform_profile is set to balanced, then it will
>>>>>>>        enable AMT and then the periodically run workqueue function from amd-pmf
>>>>>>>        will do its AMT thing. But what when the thinkpad_acpi platform_profile is
>>>>>>>        set to low-power or performance. Should the amd-pmf code then apply the static
>>>>>>>        slider settings for low-power/performance which it has read from the ACPI
>>>>>>>        tables?  Or will the ACPI/EC code on thinkpads take care of this themselves ?
>>>>>>>
>>>>>>
>>>>>> When thinkpad_acpi changes platform profile then a BIOS event goes through and amd-pmf receives that and will run based on the event.
>>>>>
>>>>> Hmm, I don't remember seeing anything for this in the patches. Actually this
>>>>> reminds me that the code should probably reschedule (using mod_delayed_work)
>>>>> the work to run immediately after a BIOS event, rather then waiting for
>>>>> the next normally scheduled run.
>>>>>
>>>>> But even then I don't remember seeing any code related to catching
>>>>> platform-profile changes done outside amd-pmf... ?
>>>>
>>>> It's not a platform profile change - it's an ACPI event.
>>>>
>>>> When a user changes a platform profile then thinkpad_acpi will see whether it's balanced or not.  When changing to/from balanced thinkpad_acpi sends an AMT event.  amd-pmf reacts to said AMT event.
>>>>
>>>> This is the code you're looking for (in this specific patch):
>>>>
>>>> +static void apmf_event_handler(acpi_handle handle, u32 event, void *data)
>>>> +{
>>>> +    struct amd_pmf_dev *pmf_dev = data;
>>>> +    struct apmf_if *apmf_if = pmf_dev->apmf_if;
>>>> +    int ret;
>>>> +
>>>> +    if (apmf_if->func.sbios_requests) {
>>>> +        struct apmf_sbios_req req;
>>>> +
>>>> +        ret = apmf_get_sbios_requests(apmf_if, &req);
>>>> +        if (ret) {
>>>> +            dev_err(pmf_dev->dev, "Failed to get SBIOS requests:%d\n", ret);
>>>> +            return;
>>>> +        }
>>>> +        if (req.pending_req & BIT(APMF_AMT_NOTIFICATION)) {
>>>> +            pr_debug("PMF: AMT is supported and notifications %s\n",
>>>> +                 req.amt_event ? "Enabled" : "Disabled");
>>>> +            if (req.amt_event)
>>>> +                pmf_dev->is_amt_event = true;
>>>> +            else
>>>> +                pmf_dev->is_amt_event = !!req.amt_event;
>>>> +        }
>>>> +
>>>> +        if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
>>>> +            pr_debug("PMF: CQL is supported and notifications %s\n",
>>>> +                 req.cql_event ? "Enabled" : "Disabled");
>>>> +            if (req.cql_event)
>>>> +                pmf_dev->is_cql_event = true;
>>>> +            else
>>>> +                pmf_dev->is_cql_event = !!req.cql_event;
>>>> +
>>>> +            /* update the target mode information */
>>>> +            amd_pmf_update_2_cql(pmf_dev);
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>
>>> Right this is the AMT on/off path that bit I understand.
>>> This happens when switching to / away from balanced mode.
>>>
>>> My question is what does the equivalent of these lines:
>>>
>>> +        amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false,
>>> +                 config_store.prop[src][idx].sppt_apu_only, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
>>> +                 config_store.prop[src][idx].stt_min, NULL);
>>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
>>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
>>> +        amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
>>> +                 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
>>>
>>> When the profile is switched (by userspace, or through the hotkeys on
>>> the laptop) to low-power or to performance mode ?
>>
>> Lenovo's firmware will handle the equivalent of changing relevant values for their platform through a BIOS interface in this case when they change ACPI platform profiles.  You will see in their driver something call "PSC" mode, and this is exactly that type of stuff.
> 
> Ok I see, thank you for clarifying this.
> 
> So as for the AMT mode, since that is Lenovo only, I guess that means
> that there is no need to do call amd_pmf_update_slider() when AMT
> is being disabled since at this point the firmware will have
> already set the values.

Yeah, Shyam made this modification for v2 to make sure that code path 
isn't called unless static slider was set in the BIOS.

> 
> Actually this seems to mean that we must ensure that the AMD-PMF
> code stops touching these settings as soon as the event is received.
> 
> Which would imply killing the periodic work when an AMT off event
> is received from within the event handling and then restating it
> when AMT is on (and making sure the work being queued or not state
> matches the AMT on/off state at driver probe time) ?
> 

At first glance this seems plausible, but actually I think it should 
stay as is because CQL thermals can be set at any time (that's like a 
lap mode sensor event from thinkpad_acpi).  Even when AMT is turned off, 
you may want the CQL thermal profile set accordingly.