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'. > >> >> 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? Yes, I agree with Mario here. Thanks, Shyam
