Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Andy Shevchenko]

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_Se0hrziw2oTNS3ENNoeHYhvciCRq9Yww@mail.gmail.com/


--
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Mark Pearson]

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.
> 
> [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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Rajat Jain]

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.
> >
> > [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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Rajat Jain]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Jani Nikula]

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.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Benjamin Berg]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [External] Re: [PATCH] thinkpad_acpi: Add sysfs entry for lcdshadow feature

[Rajat Jain]

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel