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 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.

> 
> Regards,
> 
> Hans
>