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

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: Fri, 29 Jul 2022 10:43:33 -0500	[thread overview]
Message-ID: <bab60716-a19c-ffaf-81f5-d26bc8f2f6f3@amd.com> (raw)
In-Reply-To: <b75f24fc-3ac4-35b0-3df6-870f4151dc43@redhat.com>

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.

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

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

It is up to the firmware (and thinkpad_acpi) to decide when to send
the CQL events.

Shyam - any comments here?