From: "Limonciello, Mario" <mario.limonciello@amd.com>
To: Hans de Goede <hdegoede@redhat.com>,
Shyam Sundar S K <Shyam-sundar.S-k@amd.com>,
markgross@kernel.org
Cc: platform-driver-x86@vger.kernel.org, Patil.Reddy@amd.com,
Mark Pearson <markpearson@lenovo.com>
Subject: Re: [PATCH v1 13/15] platform/x86/amd/pmf: Handle AMT and CQL events for Auto mode
Date: Thu, 28 Jul 2022 09:38:13 -0500 [thread overview]
Message-ID: <f351a921-3b7f-d8c9-10e5-816f83f22775@amd.com> (raw)
In-Reply-To: <ce75f2cc-0129-84f5-bbab-dae5ed9a7ea9@redhat.com>
>>> 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.
next prev parent reply other threads:[~2022-07-28 14:40 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-12 14:58 [PATCH v1 00/15] platform/x86/amd/pmf: Introduce AMD PMF Driver Shyam Sundar S K
2022-07-12 14:58 ` [PATCH v1 01/15] ACPI: platform_profile: Add support for notification chains Shyam Sundar S K
2022-07-12 15:03 ` Limonciello, Mario
2022-07-27 13:24 ` Hans de Goede
2022-07-27 20:38 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 02/15] platform/x86/amd/pmf: Add support for PMF core layer Shyam Sundar S K
2022-07-27 13:57 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 03/15] platform/x86/amd/pmf: Add support for PMF APCI layer Shyam Sundar S K
2022-07-27 13:57 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 04/15] platform/x86/amd/pmf: Add support SPS PMF feature Shyam Sundar S K
2022-07-27 19:29 ` Hans de Goede
2022-07-27 20:26 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 05/15] platform/x86/amd/pmf: Add debugfs information Shyam Sundar S K
2022-07-27 19:50 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 06/15] platform/x86/amd/pmf: Add heartbeat signal support Shyam Sundar S K
2022-07-27 19:53 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 07/15] platform/x86/amd/pmf: Add fan control support Shyam Sundar S K
2022-07-27 20:11 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 08/15] platform/x86/amd/pmf: Get performance metrics from PMFW Shyam Sundar S K
2022-07-27 20:36 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 09/15] platform/x86/amd/pmf: Add support for CnQF Shyam Sundar S K
2022-07-27 20:51 ` Hans de Goede
2022-07-27 21:00 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 10/15] platform/x86/amd/pmf: Add sysfs to toggle CnQF Shyam Sundar S K
2022-07-27 20:52 ` Hans de Goede
2022-07-27 21:12 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 11/15] Documentation/ABI/testing/sysfs-amd-pmf: Add ABI doc for AMD PMF Shyam Sundar S K
2022-07-27 20:52 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 12/15] platform/x86/amd/pmf: Add support for Auto mode feature Shyam Sundar S K
2022-07-27 21:22 ` Hans de Goede
2022-07-28 12:57 ` Shyam Sundar S K
2022-07-28 13:15 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 13/15] platform/x86/amd/pmf: Handle AMT and CQL events for Auto mode Shyam Sundar S K
2022-07-27 21:33 ` Hans de Goede
2022-07-27 21:44 ` Hans de Goede
2022-07-27 21:46 ` Hans de Goede
2022-07-27 23:52 ` Limonciello, Mario
2022-07-28 13:03 ` Hans de Goede
2022-07-28 13:43 ` Limonciello, Mario
2022-07-28 14:09 ` Hans de Goede
2022-07-28 14:38 ` Limonciello, Mario [this message]
2022-07-28 17:46 ` Hans de Goede
2022-07-28 18:06 ` Limonciello, Mario
2022-07-28 18:17 ` Hans de Goede
2022-07-28 21:01 ` Limonciello, Mario
2022-07-29 11:03 ` Hans de Goede
2022-07-29 15:43 ` Limonciello, Mario
2022-07-29 17:40 ` Shyam Sundar S K
2022-07-29 17:59 ` Hans de Goede
2022-08-01 10:29 ` Shyam Sundar S K
2022-08-01 11:08 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 14/15] platform/x86/amd/pmf: Force load driver on older supported platforms Shyam Sundar S K
2022-07-27 21:40 ` Hans de Goede
2022-07-12 14:58 ` [PATCH v1 15/15] MAINTAINERS: Add AMD PMF driver entry Shyam Sundar S K
2022-07-27 21:41 ` Hans de Goede
2022-07-28 17:44 ` Shyam Sundar S K
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f351a921-3b7f-d8c9-10e5-816f83f22775@amd.com \
--to=mario.limonciello@amd.com \
--cc=Patil.Reddy@amd.com \
--cc=Shyam-sundar.S-k@amd.com \
--cc=hdegoede@redhat.com \
--cc=markgross@kernel.org \
--cc=markpearson@lenovo.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).