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);
+ }
+ }
+}
+
There is this bit:
if (current_profile == PLATFORM_PROFILE_BALANCED) {
- /* Apply the Auto Mode transition */
- amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
+ if (dev->is_amt_event) {
+ /* Apply the Auto Mode transition */
+ amd_pmf_trans_automode(dev, socket_power, time_elapsed_ms);
+ } else if (!dev->is_amt_event && dev->amt_running) {
+ pr_debug("resetting AMT thermals\n");
+ mode = amd_pmf_get_pprof_modes(dev);
+ amd_pmf_update_slider(dev, SLIDER_OP_SET, mode, NULL);
+ dev->amt_running = false;
+ }
+ } else {
+ dev->amt_running = false;
}
But the new code here only applies the static slider settings on
is_amt_event edges (going from 1->0) and if the static slider support
bits are supposed to not be set then amd_pmf_load_defaults_sps() will
not have run because
is_apmf_func_supported(APMF_FUNC_STATIC_SLIDER_GRANULAR) will return
false.
So the values being set by amd_pmf_update_slider() will not have been
initialized and it will be setting everything to 0.
Also amd_pmf_get_pprof_modes() will always return POWER_MODE_POWER_SAVER
since pmf->current_profile is left at its 0 value (from kzalloc) in
this case.
So it seems that the code path where AMT is being disabled here is
buggy and it still is not clear to me where the limits get set
when thinkpad_acpi's platform_profile gets set to low-power
or performance.
I think you're right an extra check should end up in
amd_pmf_update_slider that only runs code when the static slider returns
true.
