Re: [PATCH] platform/x86/amd: pmf: Use meta + L for screen lock command

[Armin Wolf <W_Armin@gmx.de>]

Am 31.03.25 um 19:49 schrieb Mario Limonciello:

> On 3/21/2025 5:25 PM, Mario Limonciello wrote:
>>
>>
>> On 3/21/25 16:16, Armin Wolf wrote:
>>> Am 21.03.25 um 20:30 schrieb Mario Limonciello:
>>>
>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>
>>>> In practice userspace software doesn't react to KEY_SCREENLOCK by
>>>> default.  So any time that the PMF policies would suggest to lock
>>>> the screen (for example from an HPD sensor event) userspace isn't
>>>> configured to do it properly.
>>>>
>>>> However userspace is configured for meta + L as this is the default
>>>> in the ecosystem. Adjust the PMF driver to send meta + L.
>>>
>>> Hi,
>>>
>>> KEY_SCREENLOCK is used by other drivers too, so it would make sense
>>> to instead add support for KEY_SCREENLOCK to the userspace software
>>> instead of having this workaround inside the driver.
>>
>> Right; that's actually that's the first thing I looked at when I came
>> to this issue.
>>
>> I had "expected" GNOME for example to work with KEY_SCREENLOCK, but
>> even when you program it to do so it doesn't work.
>>
>> https://gitlab.gnome.org/GNOME/mutter/-/issues/3990
>>
>> The ecosystem has moved to META + L.  My last employer (Dell) I
>> remember there was a FN + F key that would issue a screen lock. It
>> had a silkscreen of a lock symbol.
>> How did it work?  Not KEY_SCREENLOCK - it emulated META + L.
>>
>> This is what works in Windows, GNOME and KDE.  So I am of the opinion
>> that KEY_SCREENLOCK is likely a dinosaur that doesn't really exist
>> anymore.
>>
>
> FWIW, I found an aftermarket keyboard (Logitech Ergo K860 [1]) that
> has a "lock" key.
>
> It also emits a KEY_LEFTMETA combination when this key is pressed and
> works by default in GNOME as well with no changes.
>
> -event11  DEVICE_ADDED            Logitech ERGO K860 seat0 default
> group7  cap:kp left scroll-nat scroll-button
>
> -event11  KEYBOARD_KEY            +4.191s       KEY_LEFTMETA (125)
> pressed
>  event11  KEYBOARD_KEY            +4.231s       *** (-1) pressed
>  event11  KEYBOARD_KEY            +4.374s       *** (-1) released
>  event11  KEYBOARD_KEY            +4.412s       KEY_LEFTMETA (125)
> released
>
> [1] https://www.logitech.com/en-us/shop/p/k860-split-ergonomic.920-009166
>
Interesting, i CCed the input maintainer so that he can decide whether to keep KEY_SCREENLOCK or replace it
with meta + L. Maybe the input subsystem could provide a generic meta + L emulation for KEY_SCREENLOCK?

Thanks,
Armin Wolf

>>>
>>> Also please add a comment explaining what meta + L is supposed to
>>> achieve.
>>>
>>
>> Sure if we can align on doing this I will spin a V2 with a comment
>> better explaining the situation.
>>
>>> Thanks,
>>> Armin Wolf
>>>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>   drivers/platform/x86/amd/pmf/tee-if.c | 11 +++++++++--
>>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/
>>>> platform/ x86/amd/pmf/tee-if.c
>>>> index 8c88769ea1d87..2c00f2baeec7b 100644
>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> @@ -151,7 +151,13 @@ static void amd_pmf_apply_policies(struct
>>>> amd_pmf_dev *dev, struct ta_pmf_enact_
>>>>                   amd_pmf_update_uevents(dev, KEY_SUSPEND);
>>>>                   break;
>>>>               case 2:
>>>> -                amd_pmf_update_uevents(dev, KEY_SCREENLOCK);
>>>> +                input_report_key(dev->pmf_idev, KEY_LEFTMETA, 1);
>>>> +                input_report_key(dev->pmf_idev, KEY_L, 1);
>>>> +                input_sync(dev->pmf_idev);
>>>> +                input_report_key(dev->pmf_idev, KEY_L, 0);
>>>> +                input_sync(dev->pmf_idev);
>>>> +                input_report_key(dev->pmf_idev, KEY_LEFTMETA, 0);
>>>> +                input_sync(dev->pmf_idev);
>>>>                   break;
>>>>               default:
>>>>                   dev_err(dev->dev, "Invalid PMF policy system
>>>> state: %d\n", val);
>>>> @@ -422,8 +428,9 @@ static int amd_pmf_register_input_device(struct
>>>> amd_pmf_dev *dev)
>>>>       dev->pmf_idev->phys = "amd-pmf/input0";
>>>>
>>>>       input_set_capability(dev->pmf_idev, EV_KEY, KEY_SLEEP);
>>>> -    input_set_capability(dev->pmf_idev, EV_KEY, KEY_SCREENLOCK);
>>>>       input_set_capability(dev->pmf_idev, EV_KEY, KEY_SUSPEND);
>>>> +    input_set_capability(dev->pmf_idev, EV_KEY, KEY_L);
>>>> +    input_set_capability(dev->pmf_idev, EV_KEY, KEY_LEFTMETA);
>>>>
>>>>       err = input_register_device(dev->pmf_idev);
>>>>       if (err) {
>>
>
>