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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 8/1/22 12:29, Shyam Sundar S K wrote:
> Hi Hans,
> 
> On 7/29/2022 11:29 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 7/29/22 19:40, Shyam Sundar S K wrote:
>>> Hi Mario,
>>>
>>> On 7/29/2022 9:13 PM, Limonciello, Mario wrote:
>>>> On 7/29/2022 06:03, Hans de Goede wrote:
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> But this code path is only hit when AMT / auto mode is available and
>>>>> when that is true then the static slider should never be set in the BIOS
>>>>> so the whole amd_pmf_update_slider() call on AMT disable can simply
>>>>> be dropped AFAICT.
>>>>
>>>> The reason to leave it in place but guarded like this is for validation
>>>> of the feature behaves properly from AMD internal systems AMD test BIOS.
>>>>  It can be used to prove out something works properly without needing to
>>>> include extra drivers and software.
>>>
>>> Yes. We will need this path to check on the internal CRB system to
>>> validate the 'auto mode'. Whenever the amd-pmf driver gets the AMT
>>> disable event we shall disable the power-settings w.r.t to 'auto mode'.
>>>
>>> I moved the handling to amd_pmf_reset_amt() based on Hans review
>>> remarks, and its guarded with a if() check, so that we accidentally
>>> don't land up in updating the static slider.
>>>
>>> Also left a note on the same function, so that it provides some
>>> information on why the logic is being done in that way.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> So the CQL code is to handle lapmode when AMT is active. But I would
>>>>> expect the firmware to update the power-limits, etc. for lapmode itself
>>>>> when in performance mode. >
>>>>> The amd_pmf_update_2_cql() function only does things when
>>>>> config_store.current_mode == AUTO_PERFORMANCE (or
>>>>> AUTO_PERFORMANCE_ON_LAP)
>>>>>
>>>>> And that reflects the last mode selected by the auto/AMT mode code, not
>>>>> the mode actual set by thinkpad_acpi so if the last auto selected mode
>>>>> was balanced and then AMT gets disabled because thinkpad_acpi switches
>>>>> to performance mode, then on CQL events after the switch
>>>>> amd_pmf_update_2_cql()
>>>>> will not do anything.
>>>>>
>>>>> To me it seems that when AMT is off the AMD-PMF code should not touch
>>>>> the power-limits, etc. at all and thus it should also always ignore
>>>>> CQL events when AMT is off.
>>>>>
>>>>> This assumes that the firmware takes care of udating the limits for
>>>>> on lap / off lap when thinkpad_acpi's profile is set to performance.
>>>>
>>>> Where does this assumption come from?  I guess that's how it's done on
>>>> Lenovo's Intel systems?
>>>>
>>>> AMT and CQL is a new feature on Lenovo AMD systems, this is the way that
>>>> it's supposed to be done here.
>>>
>>> Yes, this was newly designed for Lenovo AMD systems. The behavior is
>>> same on windows too (atleast on the RMB laptops today) .
>>>
>>> When the system is running in 'auto-mode performance' and the user keeps
>>> the system on his lap, amd-pmf driver receives a 'CQL' event from Lenovo
>>> BIOS. In this case, the amd-pmf driver shall apply thermal limits w.r.t
>>> to 'auto-mode performance-on-lap' and not 'auto-mode performance'.
>>
>> The question here is not about the 'auto-mode performance' mode
>> but what to do when AMT / 'auto-mode performance' is disabled.
>>
>> What should the behavior of the AMD-PMf code be when it receives
>> a CQL event when AMT is disabled ?
> 
> When:
> 1. AMT is disabled and we get a CQL event, it becomes a no-op to the
> amd-pmf driver.

But that is not what happens in the current (v2) code:

1. The apmf_event_handler() is always registered as long as the driver is bound
   (which it must be to catch AMT being re-enabled)

2. apmf_event_handler() does:

+		if (req.pending_req & BIT(APMF_CQL_NOTIFICATION)) {
+			pr_debug("PMF: CQL is supported and notifications %s\n",
+				 req.cql_event ? "Enabled" : "Disabled");
+			pmf_dev->is_cql_event = !!req.cql_event;
+
+			/* update the target mode information */
+			amd_pmf_update_2_cql(pmf_dev);
+		}

3. amd_pmf_update_2_cql() does:

+void amd_pmf_update_2_cql(struct amd_pmf_dev *dev)
+{
+	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
+			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;
+	if ((config_store.current_mode == AUTO_PERFORMANCE ||
+	     config_store.current_mode == AUTO_PERFORMANCE_ON_LAP) &&
+	    config_store.current_mode !=
+	    config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode) {
+		config_store.current_mode =
+				config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode;
+		amd_pmf_handle_automode(dev, SLIDER_OP_SET, config_store.current_mode, NULL);
+	}
+	dev_dbg(dev->dev, "updated CQL thermals\n");
+}

Note this does not check dev->is_amt_event at all so CQL events
may lead to amd_pmf_handle_automode() getting called, which sets
the limits even when AMT is disabled.

 
> 2. AMT is enabled:
>   - Avg. SoC power is higher than a selected measure, the amd-pmf driver
> tries to move to 'auto-mode performance' and apply the thermals set in
> the BIOS for 'auto-mode peformance' but in this scenario, when we are in
> 'auto-mode performance' and user moves the laptop from desk to lap, we
> receive a 'on-lap' event. In this case we apply thermals w.r.t to
> 'auto-mode performance-on-lap' and not 'auto-mode performance'.
> 
> That is what is being done in amd_pmf_update_2_cql() with a check:
> 	config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].target_mode =
> 			dev->is_cql_event ? AUTO_PERFORMANCE_ON_LAP : AUTO_PERFORMANCE;

Right this much I understand.

> Update of CQL happens only when AMT is active.

So you mean the SBIOS is expected to only send CQL events when it has
set AMT to be enabled ?  AFAIK the CQL events are basically on-lap /
not-on-lap events, these can easily also happen when not in AMT mode,
so the SBIOS would need to explicitly not send these events when
lap-mode changes when AMT is disabled.

Since firmware bugs do happen and can sometimes be hard to fix /
have long times to get published IMHO it would be best if 
amd_pmf_update_2_cql() would check dev->is_amt_event and return
early from the function if that is not set.

Regards,

Hans