This feature is supported on some Thinkpad products like T490s, Thinkpad
X1 yoga 4th Gen etc . The lcdshadow feature can be enabled and disabled
when user press "Fn" + "D" key. Currently, no user feedback is given for
this action. Adding as sysfs entry allows userspace to show an On Screen
Display whenever the setting changes.
Summary of changes is mentioned below :
- Added TP_HKEY_EV_LCDSHADOW_CHANGED for consistency inside the driver
- Added unmapped LCDSHADOW to keymap
- Added lcdshadow_get function to read value using ACPI
- Added lcdshadow_refresh function to re-read value and send notification
- Added sysfs group creation to tpaci_lcdshadow_init
- Added lcdshadow_exit to remove sysfs group again
- Implemented lcdshadow_enable_show/lcdshadow_enable_store
- Added handler to tpacpi_driver_event to update refresh lcdshadow
- Explicitly call tpacpi_driver_event for extended keyset
Patch is tested on kernel 5.5 on Thinkpad X1 Yoga 4th Gen.
Co-developed-by: Benjamin Berg <[email protected]>
Signed-off-by: Benjamin Berg <[email protected]>
Reviewed-by: Mark Pearson <[email protected]>
Signed-off-by: Nitin Joshi <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 117 +++++++++++++++++++++++----
1 file changed, 102 insertions(+), 15 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index da794dcfdd92..bd137cc7baee 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -155,6 +155,7 @@ enum tpacpi_hkey_event_t {
TP_HKEY_EV_VOL_UP = 0x1015, /* Volume up or unmute */
TP_HKEY_EV_VOL_DOWN = 0x1016, /* Volume down or unmute */
TP_HKEY_EV_VOL_MUTE = 0x1017, /* Mixer output mute */
+ TP_HKEY_EV_LCDSHADOW_CHANGED = 0x130f, /* Eprivacy status changed */
/* Reasons for waking up from S3/S4 */
TP_HKEY_EV_WKUP_S3_UNDOCK = 0x2304, /* undock requested, S3 */
@@ -1925,6 +1926,7 @@ enum { /* hot key scan codes (derived from ACPI DSDT) */
/* Lenovo extended keymap, starting at 0x1300 */
TP_ACPI_HOTKEYSCAN_EXTENDED_START,
+ TP_ACPI_HOTKEYSCAN_LCDSHADOW = 67,
/* first new observed key (star, favorites) is 0x1311 */
TP_ACPI_HOTKEYSCAN_STAR = 69,
TP_ACPI_HOTKEYSCAN_CLIPPING_TOOL2,
@@ -3342,7 +3344,7 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
- KEY_UNKNOWN, KEY_UNKNOWN
+ KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
},
@@ -3444,7 +3446,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
- KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
+ KEY_UNKNOWN, KEY_UNKNOWN, KEY_UNKNOWN,
+ KEY_RESERVED, /* LCD Shadow/ePrivacy */
KEY_UNKNOWN,
KEY_BOOKMARKS, /* Favorite app, 0x311 */
@@ -3921,6 +3924,7 @@ static bool hotkey_notify_hotkey(const u32 hkey,
scancode -= (0x300 - TP_ACPI_HOTKEYSCAN_EXTENDED_START);
if (scancode >= TP_ACPI_HOTKEYSCAN_EXTENDED_START &&
scancode < TPACPI_HOTKEY_MAP_LEN) {
+ tpacpi_driver_event(hkey);
tpacpi_input_send_key(scancode);
return true;
}
@@ -9717,6 +9721,12 @@ static struct ibm_struct battery_driver_data = {
static int lcdshadow_state;
+static void lcdshadow_notify_change(void)
+{
+ sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
+ "lcdshadow_enable");
+}
+
static int lcdshadow_on_off(bool state)
{
acpi_handle set_shadow_handle;
@@ -9731,6 +9741,7 @@ static int lcdshadow_on_off(bool state)
return -EIO;
lcdshadow_state = state;
+ lcdshadow_notify_change();
return 0;
}
@@ -9743,27 +9754,90 @@ static int lcdshadow_set(bool on)
return lcdshadow_on_off(on);
}
-static int tpacpi_lcdshadow_init(struct ibm_init_struct *iibm)
+static int lcdshadow_get(void)
{
acpi_handle get_shadow_handle;
int output;
- if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSSS", &get_shadow_handle))) {
- lcdshadow_state = -ENODEV;
- return 0;
- }
+ if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "GSSS",
+ &get_shadow_handle)))
+ return -ENODEV;
- if (!acpi_evalf(get_shadow_handle, &output, NULL, "dd", 0)) {
- lcdshadow_state = -EIO;
+ if (!acpi_evalf(get_shadow_handle, &output, NULL, "dd", 0))
return -EIO;
+
+ if (!(output & 0x10000))
+ return -ENODEV;
+
+ return output & 0x1;
+}
+
+static void lcdshadow_refresh(void)
+{
+ int new_state;
+
+ new_state = lcdshadow_get();
+
+ if (lcdshadow_state != new_state) {
+ lcdshadow_state = new_state;
+ lcdshadow_notify_change();
}
- if (!(output & 0x10000)) {
- lcdshadow_state = -ENODEV;
- return 0;
- }
- lcdshadow_state = output & 0x1;
+}
- return 0;
+
+/* sysfs lcdshadow entry */
+static ssize_t lcdshadow_enable_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ if (lcdshadow_state < 0)
+ return lcdshadow_state;
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", lcdshadow_state);
+}
+
+static ssize_t lcdshadow_enable_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ unsigned long t;
+ int res;
+
+ if (parse_strtoul(buf, 1, &t))
+ return -EINVAL;
+
+ tpacpi_disclose_usertask(attr->attr.name, "set to %ld\n", t);
+
+ res = lcdshadow_set(!!t);
+
+ return (res < 0) ? res : count;
+}
+
+static DEVICE_ATTR_RW(lcdshadow_enable);
+
+static struct attribute *lcdshadow_attributes[] = {
+ &dev_attr_lcdshadow_enable.attr,
+ NULL
+};
+
+static const struct attribute_group lcdshadow_attr_group = {
+ .attrs = lcdshadow_attributes,
+};
+
+
+static int tpacpi_lcdshadow_init(struct ibm_init_struct *iibm)
+{
+ int res;
+
+ lcdshadow_state = lcdshadow_get();
+
+ if (lcdshadow_state < 0 && lcdshadow_state != -ENODEV)
+ return lcdshadow_state;
+
+ res = sysfs_create_group(&tpacpi_pdev->dev.kobj,
+ &lcdshadow_attr_group);
+
+ return res;
}
static void lcdshadow_resume(void)
@@ -9805,11 +9879,18 @@ static int lcdshadow_write(char *buf)
return lcdshadow_set(state);
}
+static void lcdshadow_exit(void)
+{
+ sysfs_remove_group(&tpacpi_pdev->dev.kobj,
+ &lcdshadow_attr_group);
+}
+
static struct ibm_struct lcdshadow_driver_data = {
.name = "lcdshadow",
.resume = lcdshadow_resume,
.read = lcdshadow_read,
.write = lcdshadow_write,
+ .exit = lcdshadow_exit,
};
/****************************************************************************
@@ -9859,6 +9940,12 @@ static void tpacpi_driver_event(const unsigned int hkey_event)
mutex_unlock(&kbdlight_mutex);
}
+ if (lcdshadow_state >= 0) {
+ switch (hkey_event) {
+ case TP_HKEY_EV_LCDSHADOW_CHANGED:
+ lcdshadow_refresh();
+ }
+ }
}
static void hotkey_driver_event(const unsigned int scancode)
--
2.17.1
On Thu, Feb 20, 2020 at 9:48 AM Nitin Joshi <[email protected]> wrote:
>
> This feature is supported on some Thinkpad products like T490s, Thinkpad
> X1 yoga 4th Gen etc . The lcdshadow feature can be enabled and disabled
> when user press "Fn" + "D" key. Currently, no user feedback is given for
> this action. Adding as sysfs entry allows userspace to show an On Screen
> Display whenever the setting changes.
>
> Summary of changes is mentioned below :
>
> - Added TP_HKEY_EV_LCDSHADOW_CHANGED for consistency inside the driver
> - Added unmapped LCDSHADOW to keymap
> - Added lcdshadow_get function to read value using ACPI
> - Added lcdshadow_refresh function to re-read value and send notification
> - Added sysfs group creation to tpaci_lcdshadow_init
> - Added lcdshadow_exit to remove sysfs group again
> - Implemented lcdshadow_enable_show/lcdshadow_enable_store
> - Added handler to tpacpi_driver_event to update refresh lcdshadow
> - Explicitly call tpacpi_driver_event for extended keyset
Adding custom PrivacyGuard support to this driver was my mistake,
There is a discussion [1] how to do this in generic way to cover other
possible users.
I Cc this to people from that discussion.
[1]: https://lore.kernel.org/dri-devel/CAL_quvRknSSVvXN3q_Se0hrziw2oTNS3ENNoeHYhvciCRq9Yww@mail.gmail.com/
--
With Best Regards,
Andy Shevchenko
Hi Andy
> -----Original Message-----
> From: Andy Shevchenko <[email protected]>
> Sent: Thursday, February 20, 2020 5:43 AM
>
> On Thu, Feb 20, 2020 at 9:48 AM Nitin Joshi <[email protected]> wrote:
> >
> > This feature is supported on some Thinkpad products like T490s, Thinkpad
> > X1 yoga 4th Gen etc . The lcdshadow feature can be enabled and disabled
> > when user press "Fn" + "D" key. Currently, no user feedback is given for
> > this action. Adding as sysfs entry allows userspace to show an On Screen
> > Display whenever the setting changes.
> >
> > Summary of changes is mentioned below :
> >
> > - Added TP_HKEY_EV_LCDSHADOW_CHANGED for consistency inside the
> driver
> > - Added unmapped LCDSHADOW to keymap
> > - Added lcdshadow_get function to read value using ACPI
> > - Added lcdshadow_refresh function to re-read value and send notification
> > - Added sysfs group creation to tpaci_lcdshadow_init
> > - Added lcdshadow_exit to remove sysfs group again
> > - Implemented lcdshadow_enable_show/lcdshadow_enable_store
> > - Added handler to tpacpi_driver_event to update refresh lcdshadow
> > - Explicitly call tpacpi_driver_event for extended keyset
>
> Adding custom PrivacyGuard support to this driver was my mistake,
> There is a discussion [1] how to do this in generic way to cover other
> possible users.
> I Cc this to people from that discussion.
>
> [1]: https://lore.kernel.org/dri-
> devel/CAL_quvRknSSVvXN3q_Se0hrziw2oTNS3ENNoeHYhvciCRq9Yww@mail
> .gmail.com/
>
Thanks for the pointer to that thread - really useful and interesting, we weren't aware there was an ongoing exercise to do this.
I work with Nitin as part of the Linux team at Lenovo. We're trying to get more directly and actively involved in the open source community to improve the Linux experience on Lenovo devices and of course want to make sure we contribute the right way. We're all still pretty new so pointers and help are very much appreciated (we've been getting some great support from the distros to get us started).
For this particular issue what is the best way to contribute and get involved? We'd like to make it so ePrivacy can be used more easily from Linux. I agree a more generic way of controlling it would be good.
I looked at the proposed patch from Rajat (https://lkml.org/lkml/2019/10/22/967) - it seems like a good solution to me. We can help with testing that on our platforms if that would be useful.
I need to understand how we connect that implementation with the ACPI controls we have (as I believe what we have are thinkpad specific and not to a drm spec; we need to confirm that). We also have the ACPI events that notify if ePrivacy was changed by the hotkeys and that seems like something that should be done in thinkpad_acpi.c and not the drm code. Not sure if the two need to be connected somehow (or if handling the event is actually not important and polling is acceptable)?
As a note Nitin has been working with the Red Hat folk and is looking at the user space aspect of this (in particularl gnome settings) as well.
Thanks
Mark Pearson
Hi Mark,
On Thu, Feb 20, 2020 at 7:14 AM Mark Pearson <[email protected]> wrote:
>
> Hi Andy
>
> > -----Original Message-----
> > From: Andy Shevchenko <[email protected]>
> > Sent: Thursday, February 20, 2020 5:43 AM
> >
> > On Thu, Feb 20, 2020 at 9:48 AM Nitin Joshi <[email protected]> wrote:
> > >
> > > This feature is supported on some Thinkpad products like T490s, Thinkpad
> > > X1 yoga 4th Gen etc . The lcdshadow feature can be enabled and disabled
> > > when user press "Fn" + "D" key. Currently, no user feedback is given for
> > > this action. Adding as sysfs entry allows userspace to show an On Screen
> > > Display whenever the setting changes.
> > >
> > > Summary of changes is mentioned below :
> > >
> > > - Added TP_HKEY_EV_LCDSHADOW_CHANGED for consistency inside the
> > driver
> > > - Added unmapped LCDSHADOW to keymap
> > > - Added lcdshadow_get function to read value using ACPI
> > > - Added lcdshadow_refresh function to re-read value and send notification
> > > - Added sysfs group creation to tpaci_lcdshadow_init
> > > - Added lcdshadow_exit to remove sysfs group again
> > > - Implemented lcdshadow_enable_show/lcdshadow_enable_store
> > > - Added handler to tpacpi_driver_event to update refresh lcdshadow
> > > - Explicitly call tpacpi_driver_event for extended keyset
> >
> > Adding custom PrivacyGuard support to this driver was my mistake,
> > There is a discussion [1] how to do this in generic way to cover other
> > possible users.
> > I Cc this to people from that discussion.
> >
> > [1]: https://lore.kernel.org/dri-
> > devel/CAL_quvRknSSVvXN3q_Se0hrziw2oTNS3ENNoeHYhvciCRq9Yww@mail
> > .gmail.com/
> >
> Thanks for the pointer to that thread - really useful and interesting, we weren't aware there was an ongoing exercise to do this.
>
> I work with Nitin as part of the Linux team at Lenovo. We're trying to get more directly and actively involved in the open source community to improve the Linux experience on Lenovo devices and of course want to make sure we contribute the right way. We're all still pretty new so pointers and help are very much appreciated (we've been getting some great support from the distros to get us started).
>
> For this particular issue what is the best way to contribute and get involved? We'd like to make it so ePrivacy can be used more easily from Linux. I agree a more generic way of controlling it would be good.
> I looked at the proposed patch from Rajat (https://lkml.org/lkml/2019/10/22/967) - it seems like a good solution to me. We can help with testing that on our platforms if that would be useful.
Thanks you, just so that you know, the latest patchset is at:
https://lkml.org/lkml/2019/12/20/794
It would be great to get some additional testing if possible. I can
send a sample ACPI (for our platform) in case it helps.
>
> I need to understand how we connect that implementation with the ACPI controls we have (as I believe what we have are thinkpad specific and not to a drm spec; we need to confirm that). We also have the ACPI events that notify if ePrivacy was changed by the hotkeys and that seems like something that should be done in thinkpad_acpi.c and not the drm code.
Not sure if the two need to be connected somehow (or if handling the
event is actually not important and polling is acceptable)?
So there was some brief discussion about this on my patches - but
atleast on the platforms I have seen, there was no way to change the
privacy screen out of software / kernel control. Essentially, if there
are hotkeys, they would send an input event to the kernel, who'd send
them to userspace, who'd use the DRM method to toggle the privacy
screen. Thus the current version of the patch only supports
controlling the privacy screen via set() method. The get() method just
returns the cached value.I hope that works for you.
Jani, I'm waiting on your inputs here
https://lkml.org/lkml/2020/1/24/1932 in order to send the next
iteration of my patch. Can you please let me know if you have any
comments.
Thanks & Best Regards,
Rajat
>
> As a note Nitin has been working with the Red Hat folk and is looking at the user space aspect of this (in particularl gnome settings) as well.
>
> Thanks
> Mark Pearson
Hi Rajat,
> -----Original Message-----
> From: Rajat Jain <[email protected]>
> Sent: Thursday, February 20, 2020 1:39 PM
> >
> > For this particular issue what is the best way to contribute and get
> > involved? We'd like to make it so ePrivacy can be used more easily from
> > Linux. I agree a more generic way of controlling it would be good.
> > I looked at the proposed patch from Rajat
> > (https://lkml.org/lkml/2019/10/22/967) - it seems like a good solution to me.
> > We can help with testing that on our platforms if that would be useful.
>
> Thanks you, just so that you know, the latest patchset is at:
> https://lkml.org/lkml/2019/12/20/794
>
> It would be great to get some additional testing if possible. I can
> send a sample ACPI (for our platform) in case it helps.
>
Sounds good - we'll definitely try this out and see how it goes. I suspect we'll have some questions once we try it out and get more familiar.
> >
> > I need to understand how we connect that implementation with the ACPI
> > controls we have (as I believe what we have are thinkpad specific and not to
> > a drm spec; we need to confirm that). We also have the ACPI events that
> > notify if ePrivacy was changed by the hotkeys and that seems like something
> > that should be done in thinkpad_acpi.c and not the drm code.
> >
> > Not sure if the two need to be connected somehow (or if handling the
> > event is actually not important and polling is acceptable)?
>
> So there was some brief discussion about this on my patches - but
> atleast on the platforms I have seen, there was no way to change the
> privacy screen out of software / kernel control. Essentially, if there
> are hotkeys, they would send an input event to the kernel, who'd send
> them to userspace, who'd use the DRM method to toggle the privacy
> screen. Thus the current version of the patch only supports
> controlling the privacy screen via set() method. The get() method just
> returns the cached value.I hope that works for you.
>
OK - on the thinkpads we have function+D as a 'hotkey' to control the feature...and my understanding is that bypasses everything and goes straight to the firmware.
The changes Nitin had been working on in thinkpad_acpi.c was to make this more Linux and friendly - provide a sysfs hook for user space to connect to with the aim of allowing it to be configured from user space and have on screen display when it was triggered etc.
I'm personally not sure yet how this ties up with the DRM method - more digging required. I'm intrigued to see if it works on our systems (sadly I don't have anything with that feature available on my desk right now...I need to get my hands on one)
Thanks
Mark
Hi Mark,
On Thu, Feb 20, 2020 at 11:03 AM Mark Pearson <[email protected]> wrote:
>
> Hi Rajat,
>
> > -----Original Message-----
> > From: Rajat Jain <[email protected]>
> > Sent: Thursday, February 20, 2020 1:39 PM
> > >
> > > For this particular issue what is the best way to contribute and get
> > > involved? We'd like to make it so ePrivacy can be used more easily from
> > > Linux. I agree a more generic way of controlling it would be good.
> > > I looked at the proposed patch from Rajat
> > > (https://lkml.org/lkml/2019/10/22/967) - it seems like a good solution to me.
> > > We can help with testing that on our platforms if that would be useful.
> >
> > Thanks you, just so that you know, the latest patchset is at:
> > https://lkml.org/lkml/2019/12/20/794
> >
> > It would be great to get some additional testing if possible. I can
> > send a sample ACPI (for our platform) in case it helps.
> >
> Sounds good - we'll definitely try this out and see how it goes. I suspect we'll have some questions once we try it out and get more familiar.
>
> > >
> > > I need to understand how we connect that implementation with the ACPI
> > > controls we have (as I believe what we have are thinkpad specific and not to
> > > a drm spec; we need to confirm that). We also have the ACPI events that
> > > notify if ePrivacy was changed by the hotkeys and that seems like something
> > > that should be done in thinkpad_acpi.c and not the drm code.
> > >
> > > Not sure if the two need to be connected somehow (or if handling the
> > > event is actually not important and polling is acceptable)?
> >
> > So there was some brief discussion about this on my patches - but
> > atleast on the platforms I have seen, there was no way to change the
> > privacy screen out of software / kernel control. Essentially, if there
> > are hotkeys, they would send an input event to the kernel, who'd send
> > them to userspace, who'd use the DRM method to toggle the privacy
> > screen. Thus the current version of the patch only supports
> > controlling the privacy screen via set() method. The get() method just
> > returns the cached value.I hope that works for you.
> >
> OK - on the thinkpads we have function+D as a 'hotkey' to control the feature...and my understanding is that bypasses everything and goes straight to the firmware.
>
> The changes Nitin had been working on in thinkpad_acpi.c was to make this more Linux and friendly - provide a sysfs hook for user space to connect to with the aim of allowing it to be configured from user space and have on screen display when it was triggered etc.
>
> I'm personally not sure yet how this ties up with the DRM method - more digging required. I'm intrigued to see if it works on our systems (sadly I don't have anything with that feature available on my desk right now...I need to get my hands on one)
Just FYI, Here is the brief discussion we had about an interrupt
mechanism to support a (hardware based) "kill switch" for the privacy
screen.
https://lkml.org/lkml/2019/10/25/992
Thanks,
Rajat
>
> Thanks
> Mark
On Thu, 20 Feb 2020, Rajat Jain <[email protected]> wrote:
> Hi Mark,
>
>
> On Thu, Feb 20, 2020 at 11:03 AM Mark Pearson <[email protected]> wrote:
>>
>> Hi Rajat,
>>
>> > -----Original Message-----
>> > From: Rajat Jain <[email protected]>
>> > Sent: Thursday, February 20, 2020 1:39 PM
>> > >
>> > > For this particular issue what is the best way to contribute and get
>> > > involved? We'd like to make it so ePrivacy can be used more easily from
>> > > Linux. I agree a more generic way of controlling it would be good.
>> > > I looked at the proposed patch from Rajat
>> > > (https://lkml.org/lkml/2019/10/22/967) - it seems like a good solution to me.
>> > > We can help with testing that on our platforms if that would be useful.
>> >
>> > Thanks you, just so that you know, the latest patchset is at:
>> > https://lkml.org/lkml/2019/12/20/794
>> >
>> > It would be great to get some additional testing if possible. I can
>> > send a sample ACPI (for our platform) in case it helps.
>> >
>> Sounds good - we'll definitely try this out and see how it goes. I
>> suspect we'll have some questions once we try it out and get more
>> familiar.
>>
>> > >
>> > > I need to understand how we connect that implementation with the ACPI
>> > > controls we have (as I believe what we have are thinkpad specific and not to
>> > > a drm spec; we need to confirm that). We also have the ACPI events that
>> > > notify if ePrivacy was changed by the hotkeys and that seems like something
>> > > that should be done in thinkpad_acpi.c and not the drm code.
>> > >
>> > > Not sure if the two need to be connected somehow (or if handling the
>> > > event is actually not important and polling is acceptable)?
>> >
>> > So there was some brief discussion about this on my patches - but
>> > atleast on the platforms I have seen, there was no way to change the
>> > privacy screen out of software / kernel control. Essentially, if there
>> > are hotkeys, they would send an input event to the kernel, who'd send
>> > them to userspace, who'd use the DRM method to toggle the privacy
>> > screen. Thus the current version of the patch only supports
>> > controlling the privacy screen via set() method. The get() method just
>> > returns the cached value.I hope that works for you.
>> >
>> OK - on the thinkpads we have function+D as a 'hotkey' to control the
>> feature...and my understanding is that bypasses everything and goes
>> straight to the firmware.
In general I think it's preferrable if the hotkey sends the key event to
userspace that then makes the policy decision of what, if anything, to
do with it. Everything is much easier if the policy is in userspace
control. For example, you could define content based policies for
enabling privacy screen, something that is definitely not possible with
firmware.
I emphatize with the desire to just bypass everything at the
hardware/firmware level, because that is totally in your control (as an
OEM), and requires no interaction with the operating system
initially. Exposing the read-only state of the privacy screen is
helpful, but prevents the OS from building more advanced features on
top, failing to reach the full potential of the nice hardware feature.
That said, we obviously do need to take such hardware/firmware
implementations into account as well.
>> The changes Nitin had been working on in thinkpad_acpi.c was to make
>> this more Linux and friendly - provide a sysfs hook for user space to
>> connect to with the aim of allowing it to be configured from user
>> space and have on screen display when it was triggered etc.
IMO one of the problems with using sysfs for this is that it's not
connected with the graphics subsystem. The userspace has to go out of
its way to make the connection between the privacy screen and the
display. It shouldn't have to. It's a property of the display, not some
unrelated device (although, technically, I presume in hardware it might
be ;).
We've made the mistake with backlight before, and we still somewhat
struggle with it. Please let's not repeat that.
>> I'm personally not sure yet how this ties up with the DRM method -
>> more digging required. I'm intrigued to see if it works on our
>> systems (sadly I don't have anything with that feature available on
>> my desk right now...I need to get my hands on one)
>
> Just FYI, Here is the brief discussion we had about an interrupt
> mechanism to support a (hardware based) "kill switch" for the privacy
> screen.
> https://lkml.org/lkml/2019/10/25/992
I agree with Pekka's mail [1] in that thread.
BR,
Jani.
[1] https://lkml.org/lkml/2019/10/28/94
--
Jani Nikula, Intel Open Source Graphics Center
On Thu, 20 Feb 2020, Rajat Jain <[email protected]> wrote:
> Jani, I'm waiting on your inputs here
> https://lkml.org/lkml/2020/1/24/1932 in order to send the next
> iteration of my patch. Can you please let me know if you have any
> comments.
Yikes, sorry, I didn't realize you were still waiting for my input. :(
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Hi,
On Fri, 2020-02-21 at 14:28 +0200, Jani Nikula wrote:
> In general I think it's preferrable if the hotkey sends the key event to
> userspace that then makes the policy decision of what, if anything, to
> do with it. Everything is much easier if the policy is in userspace
> control. For example, you could define content based policies for
> enabling privacy screen, something that is definitely not possible with
> firmware.
>
> I emphatize with the desire to just bypass everything at the
> hardware/firmware level, because that is totally in your control (as an
> OEM), and requires no interaction with the operating system
> initially. Exposing the read-only state of the privacy screen is
> helpful, but prevents the OS from building more advanced features on
> top, failing to reach the full potential of the nice hardware feature.
There seems to be a slight misunderstanding here. On the Lenovo laptops
the feature is automatically adjusted by the Firmware. However, the
setting itself is read/write and it can also be controlled from
userspace.
In principle, I agree that it makes sense to control these things from
software and have a toggle key event that is send around. It has the
unfortunate disadvantage though that it requires updating the entire
userspace. Including the ugly side effect that we continue to have
trouble to support these things on X11 due protocol restrictions with
"high" key codes (>= 248).
Benjamin
On Fri, Feb 21, 2020 at 4:29 AM Jani Nikula <[email protected]> wrote:
>
> On Thu, 20 Feb 2020, Rajat Jain <[email protected]> wrote:
> > Jani, I'm waiting on your inputs here
> > https://lkml.org/lkml/2020/1/24/1932 in order to send the next
> > iteration of my patch. Can you please let me know if you have any
> > comments.
>
> Yikes, sorry, I didn't realize you were still waiting for my input. :(
Hi Jani,
I have posted a new iteration of my patchset at:
https://patchwork.freedesktop.org/series/74299/
I'd appreciate if you could please take a look and provide any comments.
Thanks & Best Regards,
Rajat
>
> BR,
> Jani.
>
> --
> Jani Nikula, Intel Open Source Graphics Center