diff mbox series

platform/x86/amd: pmf: Use meta + L for screen lock command

Message ID 20250321193052.2973537-1-superm1@kernel.org (mailing list archive)
State New
Headers show
Series platform/x86/amd: pmf: Use meta + L for screen lock command | expand

Commit Message

Mario Limonciello March 21, 2025, 7:30 p.m. UTC 
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.

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

Comments

Armin Wolf March 21, 2025, 9:16 p.m. UTC | #1 
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.

Also please add a comment explaining what meta + L is supposed to achieve.

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) {
Mario Limonciello March 21, 2025, 10:25 p.m. UTC | #2 
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.

> 
> 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) {
diff mbox series

Patch

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