Re: [PATCH v5] platform/x86: dell-laptop: Implement platform_profile

[Limonciello, Mario]

On 5/11/2024 10:05 AM, Shen, Yijun wrote:
> Internal Use - Confidential
> > -----Original Message-----
> > From: Mario Limonciello <mario.limonciello@xxxxxxx>
> > Sent: Wednesday, May 8, 2024 11:53 PM
> > To: Shen, Yijun <Yijun_Shen@xxxxxxxx>; Lyndon Sanche
> > <lsanche@xxxxxxxxxx>
> > Cc: pali@xxxxxxxxxx; W_Armin@xxxxxx;
> > srinivas.pandruvada@xxxxxxxxxxxxxxx; ilpo.jarvinen@xxxxxxxxxxxxxxx;
> > lkp@xxxxxxxxx; Hans de Goede <hdegoede@xxxxxxxxxx>; Matthew Garrett
> > <mjg59@xxxxxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; Heiner Kallweit
> > <hkallweit1@xxxxxxxxx>; Vegard Nossum <vegard.nossum@xxxxxxxxxx>;
> > platform-driver-x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Dell Client
> > Kernel <Dell.Client.Kernel@xxxxxxxx>
> > Subject: Re: RE: [PATCH v5] platform/x86: dell-laptop: Implement
> > platform_profile
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On 5/8/2024 09:24, Shen, Yijun wrote:
> > > Hi Lyndon,
> > >
> > >    Thanks for working on this patch.
> > >
> > > >
> > >    Dell side has an initial testing with this patch on some laptops, it looks
> > good. While changing the platform profile:
> > > 1. The corresponding USTT option in BIOS will be changed.
> > > 2. thermald will not be impacted. The related PSVT and ITMT will be loaded.
> > >    Some Dell DTs does not have the USTT, Dell'll have a check if nothing is
> > broken.
> >
> > Hi Alex!
> >
> > Have you had a check both on both your AMD laptops and workstations too,
> > or just the Intel ones?  I think it would be good to make sure it's getting the
> > correct experience in both cases.
> Hi Mario,
>
>   I've a check for this, for both laptop and workstation, the dell_laptop module will not be loaded. So, AMD platform will not be impacted by this patch series.
> Follow is one example output with workstation.
>   #lsmod | grep dell
>     dell_wmi               28672  0
>     dell_smbios            32768  1 dell_wmi
>     dcdbas                 20480  1 dell_smbios
>     dell_wmi_descriptor    20480  2 dell_wmi,dell_smbios
>     sparse_keymap          12288  1 dell_wmi
>     ledtrig_audio          12288  3 snd_ctl_led,snd_hda_codec_generic,dell_wmi
>     video                  73728  2 dell_wmi,nvidia_modeset
>     wmi                    40960  5 video,dell_wmi,wmi_bmof,dell_smbios,dell_wmi_descriptor

Ah; right that makes sense.  In that case, is dell-laptop even the right 
place for this patch series?  I would think the same policies for the 
platform profile should be able to apply to desktop/workstation.

The v6 of this series would block smbios-thermal-ctl from working on a 
workstation too.

> > >     Additional, with this patch, follow behavior is found:
> > >    1. For example, the platform profile is quiet.
> > >    2. Reboot the system and change the USTT to performance.
> > >    3. Boot to desktop, the platform profile is "quiet", the USTT will be
> > changed back to "quiet".
> > >    This looks like not a proper user experience. The platform profile should
> > honor the BIOS setting, aka, the platform profile should be switched to
> > "performance".
> > >
> >
> > I agree, this sounds like the initial profile needs to be read from the BIOS
> > settings too.
> >
> > Furthermore I wanted to ask is there also a WMI setting that corresponds to
> > this that dell-wmi-sysman offers?
>   Yes, Mario, you're right. This thermal setting could also be toggled by dell-wmi-sysman.
> But, for the Dell consumer AMD laptops, like Alienware, the BIOS is another variant which is different with the workstation one.
> With this variant BIOS, there is no USTT and also no dell_wmi/dell-wmi-sysman.
>
> > I'm wondering if both should be probed in case the SMBIOS one goes away one day.
>   Yep, I think this is a good suggestion.

Great! Although something I wonder is if the policy when changed with 
dell-wmi-sysman is immediate or requires a reboot.  A lot of the 
settings in there aren't effective until after a reboot.

If this is one of them then it might not be a good idea to make it work 
for both.