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

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

[Armin Wolf]

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) {
>>
>
>

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

[Mario Limonciello]

On 4/2/2025 8:48 PM, Armin Wolf wrote:
> 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

Are you proposing input subsystem to provide a helper something like this?

/**
  * report_lock_sequence - Report key combination to lock the screen
  * @dev: input device
  *
  * Key combination used in the PC industry since Windows 7 for locking 
display
  * is META + L. This is also used in GNOME and KDE by default.
  * See 
https://support.microsoft.com/en-us/windows/keyboard-shortcuts-in-windows-dcc61a57-8ff0-cffe-9796-cb9706c75eec
  */
static void report_lock_sequence(struct input_dev *dev)
{
	input_report_key(dev, KEY_LEFTMETA, 1);
	input_report_key(dev, KEY_L, 1);
	input_sync(dev);
	input_report_key(dev, KEY_L, 0);
	input_sync(dev);
	input_report_key(dev, KEY_LEFTMETA, 0);
	input_sync(dev);
}

Then PMF could just call that helper when it wants to do the sequence 
(and likewise any other driver can as well).

> 
>>>>
>>>> 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) {
>>>
>>
>>