* Re: [PATCH] platform/x86/amd: pmf: Use meta + L for screen lock command
2025-04-03 1:48 ` [PATCH] platform/x86/amd: pmf: Use meta + L for screen lock command Armin Wolf
@ 2025-04-03 15:57 ` Mario Limonciello
0 siblings, 0 replies; 2+ messages in thread
From: Mario Limonciello @ 2025-04-03 15:57 UTC (permalink / raw)
To: Armin Wolf, Mario Limonciello, Shyam-sundar.S-k, hdegoede,
ilpo.jarvinen, dmitry.torokhov
Cc: platform-driver-x86, linux-input
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) {
>>>
>>
>>
^ permalink raw reply [flat|nested] 2+ messages in thread