On Thu, Feb 20, 2020 at 9:48 AM Nitin Joshi nitjoshi@gmail.com 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_Se0hrziw2oTNS3ENNoeHYhv...
-- With Best Regards, Andy Shevchenko
Hi Andy
-----Original Message----- From: Andy Shevchenko andy.shevchenko@gmail.com Sent: Thursday, February 20, 2020 5:43 AM
On Thu, Feb 20, 2020 at 9:48 AM Nitin Joshi nitjoshi@gmail.com 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.
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 mpearson@lenovo.com wrote:
Hi Andy
-----Original Message----- From: Andy Shevchenko andy.shevchenko@gmail.com Sent: Thursday, February 20, 2020 5:43 AM
On Thu, Feb 20, 2020 at 9:48 AM Nitin Joshi nitjoshi@gmail.com 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.
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 rajatja@google.com 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 mpearson@lenovo.com wrote:
Hi Rajat,
-----Original Message----- From: Rajat Jain rajatja@google.com 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 rajatja@google.com wrote:
Hi Mark,
On Thu, Feb 20, 2020 at 11:03 AM Mark Pearson mpearson@lenovo.com wrote:
Hi Rajat,
-----Original Message----- From: Rajat Jain rajatja@google.com 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
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 Thu, 20 Feb 2020, Rajat Jain rajatja@google.com 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.
On Fri, Feb 21, 2020 at 4:29 AM Jani Nikula jani.nikula@linux.intel.com wrote:
On Thu, 20 Feb 2020, Rajat Jain rajatja@google.com 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
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
Benjamin Berg -
Jani Nikula -
Mark Pearson -
Rajat Jain