| Message ID | 20250425162949.2021325-1-superm1@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] Input: Add a Kconfig to emulate KEY_SCREENLOCK with META + L | expand |
On Fri 2025-04-25 11:29:48, Mario Limonciello wrote: > From: Mario Limonciello <mario.limonciello@amd.com> > > In the PC industry KEY_SCREENLOCK isn't used as frequently as it used > to be. Modern versions of Windows [1], GNOME and KDE support "META" + "L" > to lock the screen. Modern hardware [2] also sends this sequence of > events for keys with a silkscreen for screen lock. > > Introduced a new Kconfig option that will change KEY_SCREENLOCK when > emitted by driver to META + L. Fix gnome and kde, do not break kernel... Pavel > > +config INPUT_SCREENLOCK_EMULATION > + bool "Pass KEY_SCREENLOCK as META + L" > + help > + Say Y here if you want KEY_SCREENLOCK to be passed to userspace as > + META + L. > + > + If unsure, say Y. > + > comment "Input Device Drivers" > > source "drivers/input/keyboard/Kconfig" > diff --git a/drivers/input/input.c b/drivers/input/input.c > index dfeace85c4710..983e3c0f88e5f 100644 > --- a/drivers/input/input.c > +++ b/drivers/input/input.c > @@ -370,6 +370,13 @@ void input_handle_event(struct input_dev *dev, > } > } > > +static void handle_screenlock_as_meta_l(struct input_dev *dev, unsigned int type, > + int value) > +{ > + input_handle_event(dev, type, KEY_LEFTMETA, value); > + input_handle_event(dev, type, KEY_L, value); > +} > + > /** > * input_event() - report new input event > * @dev: device that generated the event > @@ -392,6 +399,12 @@ void input_event(struct input_dev *dev, > { > if (is_event_supported(type, dev->evbit, EV_MAX)) { > guard(spinlock_irqsave)(&dev->event_lock); > +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION > + if (code == KEY_SCREENLOCK) { > + handle_screenlock_as_meta_l(dev, type, value); > + return; > + } > +#endif > input_handle_event(dev, type, code, value); > } > } > @@ -2134,6 +2147,13 @@ void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int > > switch (type) { > case EV_KEY: > +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION > + if (code == KEY_SCREENLOCK) { > + __set_bit(KEY_L, dev->keybit); > + __set_bit(KEY_LEFTMETA, dev->keybit); > + break; > + } > +#endif > __set_bit(code, dev->keybit); > break; >
On 4/26/25 03:15, Pavel Machek wrote: > On Fri 2025-04-25 11:29:48, Mario Limonciello wrote: >> From: Mario Limonciello <mario.limonciello@amd.com> >> >> In the PC industry KEY_SCREENLOCK isn't used as frequently as it used >> to be. Modern versions of Windows [1], GNOME and KDE support "META" + "L" >> to lock the screen. Modern hardware [2] also sends this sequence of >> events for keys with a silkscreen for screen lock. >> >> Introduced a new Kconfig option that will change KEY_SCREENLOCK when >> emitted by driver to META + L. > > Fix gnome and kde, do not break kernel... I'm sorry; fix them to do what exactly? Switch to KEY_SCREENLOCK? That's going to break modern hardware lockscreen keys. They've all obviously moved to META+L because that's what hardware today uses. That's also what earlier versions of my series tried to change just amd-pmf over to use, but Armin Wolf said this should be done in the input subsystem for all drivers instead. > Pavel > >> >> +config INPUT_SCREENLOCK_EMULATION >> + bool "Pass KEY_SCREENLOCK as META + L" >> + help >> + Say Y here if you want KEY_SCREENLOCK to be passed to userspace as >> + META + L. >> + >> + If unsure, say Y. >> + >> comment "Input Device Drivers" >> >> source "drivers/input/keyboard/Kconfig" >> diff --git a/drivers/input/input.c b/drivers/input/input.c >> index dfeace85c4710..983e3c0f88e5f 100644 >> --- a/drivers/input/input.c >> +++ b/drivers/input/input.c >> @@ -370,6 +370,13 @@ void input_handle_event(struct input_dev *dev, >> } >> } >> >> +static void handle_screenlock_as_meta_l(struct input_dev *dev, unsigned int type, >> + int value) >> +{ >> + input_handle_event(dev, type, KEY_LEFTMETA, value); >> + input_handle_event(dev, type, KEY_L, value); >> +} >> + >> /** >> * input_event() - report new input event >> * @dev: device that generated the event >> @@ -392,6 +399,12 @@ void input_event(struct input_dev *dev, >> { >> if (is_event_supported(type, dev->evbit, EV_MAX)) { >> guard(spinlock_irqsave)(&dev->event_lock); >> +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION >> + if (code == KEY_SCREENLOCK) { >> + handle_screenlock_as_meta_l(dev, type, value); >> + return; >> + } >> +#endif >> input_handle_event(dev, type, code, value); >> } >> } >> @@ -2134,6 +2147,13 @@ void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int >> >> switch (type) { >> case EV_KEY: >> +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION >> + if (code == KEY_SCREENLOCK) { >> + __set_bit(KEY_L, dev->keybit); >> + __set_bit(KEY_LEFTMETA, dev->keybit); >> + break; >> + } >> +#endif >> __set_bit(code, dev->keybit); >> break; >> >
On 4/26/25 13:41, Pavel Machek wrote: > Hi! > >>>> In the PC industry KEY_SCREENLOCK isn't used as frequently as it used >>>> to be. Modern versions of Windows [1], GNOME and KDE support "META" + "L" >>>> to lock the screen. Modern hardware [2] also sends this sequence of >>>> events for keys with a silkscreen for screen lock. >>>> >>>> Introduced a new Kconfig option that will change KEY_SCREENLOCK when >>>> emitted by driver to META + L. >>> >>> Fix gnome and kde, do not break kernel... >> >> I'm sorry; fix them to do what exactly? Switch to KEY_SCREENLOCK? >> >> That's going to break modern hardware lockscreen keys. They've all >> obviously moved to META+L because that's what hardware today uses. > > Gnome / KDE should accept either META+L _or_ KEY_SCREENLOCK to do the > screen locking, no? > This was actually the first path I looked down before I even started the kernel patch direction for this problem. GNOME doesn't support assigning more than one shortcut key for an action.
Hi! > > > > > In the PC industry KEY_SCREENLOCK isn't used as frequently as it used > > > > > to be. Modern versions of Windows [1], GNOME and KDE support "META" + "L" > > > > > to lock the screen. Modern hardware [2] also sends this sequence of > > > > > events for keys with a silkscreen for screen lock. > > > > > > > > > > Introduced a new Kconfig option that will change KEY_SCREENLOCK when > > > > > emitted by driver to META + L. > > > > > > > > Fix gnome and kde, do not break kernel... > > > > > > I'm sorry; fix them to do what exactly? Switch to KEY_SCREENLOCK? > > > > > > That's going to break modern hardware lockscreen keys. They've all > > > obviously moved to META+L because that's what hardware today uses. > > > > Gnome / KDE should accept either META+L _or_ KEY_SCREENLOCK to do the > > screen locking, no? > > This was actually the first path I looked down before I even started the > kernel patch direction for this problem. > > GNOME doesn't support assigning more than one shortcut key for an action. So if I want to start calculator on meta+c on internal keyboard, and have calculator button on USB keyboard, I'm out of luck? Sounds that should be fixed :-). Alternatively, you can just turn KEY_SCREENLOCK into META+L inside Gnome. BR, Pavel
Apologies for extended absence... On Sun, Apr 27, 2025 at 07:15:31AM -0500, Mario Limonciello wrote: > > > On 4/27/25 01:10, Pavel Machek wrote: > > Hi! > > > > > > > > > In the PC industry KEY_SCREENLOCK isn't used as frequently as it used > > > > > > > to be. Modern versions of Windows [1], GNOME and KDE support "META" + "L" > > > > > > > to lock the screen. Modern hardware [2] also sends this sequence of > > > > > > > events for keys with a silkscreen for screen lock. > > > > > > > > > > > > > > Introduced a new Kconfig option that will change KEY_SCREENLOCK when > > > > > > > emitted by driver to META + L. > > > > > > > > > > > > Fix gnome and kde, do not break kernel... > > > > > > > > > > I'm sorry; fix them to do what exactly? Switch to KEY_SCREENLOCK? > > > > > > > > > > That's going to break modern hardware lockscreen keys. They've all > > > > > obviously moved to META+L because that's what hardware today uses. Vendors do all kind of weird things. They want to ship their peripherals here and now and they do not care of shortcuts will change a few years down the road. FWIW there are plenty of external keyboards that use KEY_SCREENLOCK and do not emit any shortcurts. Anything that is "Woks with Chromebooks" will use KEY_SCREENLOCK. > > > > > > > > Gnome / KDE should accept either META+L _or_ KEY_SCREENLOCK to do the > > > > screen locking, no? KDE by default recognizes Meta+L combination (which used to be Alt+Ctrl+L), Screensaver key, and allows users to define their custom shortcuts. I also wonder how many other DEs beside Gnome do not recognize KEY_SCREENLOCK. > > > > > > This was actually the first path I looked down before I even started the > > > kernel patch direction for this problem. > > > > > > GNOME doesn't support assigning more than one shortcut key for an action. > > > > So if I want to start calculator on meta+c on internal keyboard, and > > have calculator button on USB keyboard, I'm out of luck? > > Yeah AFAICT that's the case. > > > > > Sounds that should be fixed :-). > > GNOME is commonly known to try to have a very simplistic UX instead of > exposing more knobs and buttons. > > Adding support for multiple key combinations in a UX means convincing the > GNOME design team to support this, followed by actual changes. So there is a simple and wrong way of fixing this (introducing a hardcoded combination for shortcut du jour in the kernel) and complicated one of making one of poplar DEs behave better and be more flexible. We will not be adding the wrong one to the kernel. If someone wants to do this kind of translation they are free to have a tiny uinput daemon. > > > > > Alternatively, you can just turn KEY_SCREENLOCK into META+L inside > > Gnome. > > > > BR, > > Pavel > > Or I can just go back to changing this locally in the PMF driver and it > works everywhere without needing to convince every userspace to make a > change to add special mappings. > > As there isn't appetite from input maintainers to have a mapping in the > input layer I think I'll go that direction for a v5. I think this would be a mistake. I'll mention that on the other posting. Thanks.
Hi! > > > Sounds that should be fixed :-). > > > > GNOME is commonly known to try to have a very simplistic UX instead of > > exposing more knobs and buttons. > > > > Adding support for multiple key combinations in a UX means convincing the > > GNOME design team to support this, followed by actual changes. > > So there is a simple and wrong way of fixing this (introducing a > hardcoded combination for shortcut du jour in the kernel) and > complicated one of making one of poplar DEs behave better and be more > flexible. We will not be adding the wrong one to the kernel. Note that there's a way to fix that in GNOME without changing UI that is still way nicer than kernel hacks: KEY_SCREENLOCK always locks. Whatever combination user selects in config dialog (META-L by default) locks, too. Best regards, Pavel
Hi Mario, On 28-Apr-25 15:50, Mario Limonciello wrote: > On 4/28/2025 12:51 AM, Dmitry Torokhov wrote: >> On Sun, Apr 27, 2025 at 10:30:24PM -0700, Dmitry Torokhov wrote: >>> Apologies for extended absence... >>> >>> On Sun, Apr 27, 2025 at 07:15:31AM -0500, Mario Limonciello wrote: >>>> >>>> >>>> On 4/27/25 01:10, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>>>>> In the PC industry KEY_SCREENLOCK isn't used as frequently as it used >>>>>>>>>> to be. Modern versions of Windows [1], GNOME and KDE support "META" + "L" >>>>>>>>>> to lock the screen. Modern hardware [2] also sends this sequence of >>>>>>>>>> events for keys with a silkscreen for screen lock. >>>>>>>>>> >>>>>>>>>> Introduced a new Kconfig option that will change KEY_SCREENLOCK when >>>>>>>>>> emitted by driver to META + L. >>>>>>>>> >>>>>>>>> Fix gnome and kde, do not break kernel... >>>>>>>> >>>>>>>> I'm sorry; fix them to do what exactly? Switch to KEY_SCREENLOCK? >>>>>>>> >>>>>>>> That's going to break modern hardware lockscreen keys. They've all >>>>>>>> obviously moved to META+L because that's what hardware today uses. >>> >>> Vendors do all kind of weird things. They want to ship their >>> peripherals here and now and they do not care of shortcuts will change a >>> few years down the road. >>> >>> FWIW there are plenty of external keyboards that use KEY_SCREENLOCK and >>> do not emit any shortcurts. Anything that is "Woks with Chromebooks" >>> will use KEY_SCREENLOCK. >>> >>> >>>>>>> >>>>>>> Gnome / KDE should accept either META+L _or_ KEY_SCREENLOCK to do the >>>>>>> screen locking, no? >>> >>> KDE by default recognizes Meta+L combination (which used to be >>> Alt+Ctrl+L), Screensaver key, and allows users to define their custom >>> shortcuts. >>> >>> I also wonder how many other DEs beside Gnome do not recognize >>> KEY_SCREENLOCK. >> >> So I poked around Gnome a bit. According to the gnome-settings-daemon >> source code KEY_SCREENLOCK should be recognized. It is set up as >> "screensaver-static" key which is hidden and shoudl not be changed by >> user: >> >> https://github.com/GNOME/gnome-settings-daemon/blob/master/data/org.gnome.settings-daemon.plugins.media-keys.gschema.xml.in#L504 >> >> <key name="screensaver-static" type="as"> >> <default>['XF86ScreenSaver']</default> >> <summary>Lock screen</summary> >> <description>Static binding to lock the screen.</description> >> </key> >> >> >> >>> >>>>>> >>>>>> This was actually the first path I looked down before I even started the >>>>>> kernel patch direction for this problem. >>>>>> >>>>>> GNOME doesn't support assigning more than one shortcut key for an action. >> >> It sure does even if it is not shown in UI. Poke around with >> dconf-editor and look in /org/gnome/settings-daemon/plugins/media-keys/ >> and you will see plenty of "*-static" keys with multiple >> keycodes/shortcuts assigned. >> >> "touchpad-toggle-static" - ['XF86TouchpadToggle', '<Ctrl><Super>XF86TouchpadToggle'] >> "rotate-video-lock-static" - ['<Super>o', 'XF86RotationLockToggle'] >> >> and so on... >> >> Maybe Gnome broke screen lock key in recent release? >> >> Thanks. >> > > Thanks for your feedback and looking into what GNOME is doing. It sure /sounds/ like this should have worked with no kernel changes and GNOME has a bug with the lock screen key. My guess is that maybe at some point the lock-key (combo) handling has moved from gnome-settings-daemon into mutter and maybe things broke at that point ? > I'll abandon the kernel series. Agreed, the GNOME bug needs to be fixed regardless of AMD PMF use. Regards, Hans
diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig index 88ecdf5218ee9..ffb4163fe315f 100644 --- a/drivers/input/Kconfig +++ b/drivers/input/Kconfig @@ -174,6 +174,14 @@ config INPUT_APMPOWER To compile this driver as a module, choose M here: the module will be called apm-power. +config INPUT_SCREENLOCK_EMULATION + bool "Pass KEY_SCREENLOCK as META + L" + help + Say Y here if you want KEY_SCREENLOCK to be passed to userspace as + META + L. + + If unsure, say Y. + comment "Input Device Drivers" source "drivers/input/keyboard/Kconfig" diff --git a/drivers/input/input.c b/drivers/input/input.c index dfeace85c4710..983e3c0f88e5f 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -370,6 +370,13 @@ void input_handle_event(struct input_dev *dev, } } +static void handle_screenlock_as_meta_l(struct input_dev *dev, unsigned int type, + int value) +{ + input_handle_event(dev, type, KEY_LEFTMETA, value); + input_handle_event(dev, type, KEY_L, value); +} + /** * input_event() - report new input event * @dev: device that generated the event @@ -392,6 +399,12 @@ void input_event(struct input_dev *dev, { if (is_event_supported(type, dev->evbit, EV_MAX)) { guard(spinlock_irqsave)(&dev->event_lock); +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION + if (code == KEY_SCREENLOCK) { + handle_screenlock_as_meta_l(dev, type, value); + return; + } +#endif input_handle_event(dev, type, code, value); } } @@ -2134,6 +2147,13 @@ void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int switch (type) { case EV_KEY: +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION + if (code == KEY_SCREENLOCK) { + __set_bit(KEY_L, dev->keybit); + __set_bit(KEY_LEFTMETA, dev->keybit); + break; + } +#endif __set_bit(code, dev->keybit); break;