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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

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

Update of CQL happens only when AMT is active.

Thanks,
Shyam

> 
>>>> If thinkpad_acpi does not do this then the AMD-PMF code should
>>>> check what mode has been selected by the thinkpad_acpi code in
>>>> amd_pmf_update_2_cql() when AMT is off.
> 
> Regards,
> 
> Hans
>