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 >
