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 ?
Regards,
Hans
