Re: [PATCH 8/9] platform/x86: thinkpad_acpi: Register a privacy-screen device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 9/15/21 10:55 PM, Lyude Paul wrote:
> On Mon, 2021-09-06 at 09:35 +0200, Hans de Goede wrote:
>> Register a privacy-screen device on laptops with a privacy-screen,
>> this exports the PrivacyGuard features to user-space using a
>> standardized vendor-agnostic sysfs interface. Note the sysfs interface
>> is read-only.
>>
>> Registering a privacy-screen device with the new privacy-screen class
>> code will also allow the GPU driver to get a handle to it and export
>> the privacy-screen setting as a property on the DRM connector object
>> for the LCD panel. This DRM connector property is news standardized
> 
> Looks like a typo here ------------------------------^

Ack I will fix this before pushing this out.

> 
>> interface which all user-space code should use to query and control
>> the privacy-screen.
>>
>> Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> Changes in v2:
>> - Make the new lcdshadow_set_sw_state, lcdshadow_get_hw_state and
>>   lcdshadow_ops symbols static
>> - Update state and call drm_privacy_screen_call_notifier_chain()
>>   when the state is changed by pressing the Fn + D hotkey combo
>> ---
>>  drivers/platform/x86/Kconfig         |  2 +
>>  drivers/platform/x86/thinkpad_acpi.c | 91 ++++++++++++++++++++--------
>>  2 files changed, 68 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index d12db6c316ea..ae00a27f9f95 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -509,7 +509,9 @@ config THINKPAD_ACPI
>>         depends on ACPI_VIDEO || ACPI_VIDEO = n
>>         depends on BACKLIGHT_CLASS_DEVICE
>>         depends on I2C
>> +       depends on DRM
>>         select ACPI_PLATFORM_PROFILE
>> +       select DRM_PRIVACY_SCREEN
>>         select HWMON
>>         select NVRAM
>>         select NEW_LEDS
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index b8f2556c4797..044b238730ba 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -73,6 +73,7 @@
>>  #include <linux/uaccess.h>
>>  #include <acpi/battery.h>
>>  #include <acpi/video.h>
>> +#include <drm/drm_privacy_screen_driver.h>
>>  #include "dual_accel_detect.h"
>>  
>>  /* ThinkPad CMOS commands */
>> @@ -157,6 +158,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_PRIVACYGUARD_TOGGLE  = 0x130f, /* Toggle priv.guard
>> on/off */
>>  
>>         /* Reasons for waking up from S3/S4 */
>>         TP_HKEY_EV_WKUP_S3_UNDOCK       = 0x2304, /* undock requested, S3 */
>> @@ -3889,6 +3891,12 @@ static bool hotkey_notify_extended_hotkey(const u32
>> hkey)
>>  {
>>         unsigned int scancode;
>>  
>> +       switch (hkey) {
>> +       case TP_HKEY_EV_PRIVACYGUARD_TOGGLE:
>> +               tpacpi_driver_event(hkey);
>> +               return true;
>> +       }
>> +
>>         /* Extended keycodes start at 0x300 and our offset into the map
>>          * TP_ACPI_HOTKEYSCAN_EXTENDED_START. The calculated scancode
>>          * will be positive, but might not be in the correct range.
>> @@ -9819,30 +9827,40 @@ static struct ibm_struct battery_driver_data = {
>>   * LCD Shadow subdriver, for the Lenovo PrivacyGuard feature
>>   */
>>  
>> +static struct drm_privacy_screen *lcdshadow_dev;
>>  static acpi_handle lcdshadow_get_handle;
>>  static acpi_handle lcdshadow_set_handle;
>> -static int lcdshadow_state;
>>  
>> -static int lcdshadow_on_off(bool state)
>> +static int lcdshadow_set_sw_state(struct drm_privacy_screen *priv,
>> +                                 enum drm_privacy_screen_status state)
>>  {
>>         int output;
>>  
>> +       if (WARN_ON(!mutex_is_locked(&priv->lock)))
>> +               return -EIO;
>> +
>>         if (!acpi_evalf(lcdshadow_set_handle, &output, NULL, "dd",
>> (int)state))
>>                 return -EIO;
>>  
>> -       lcdshadow_state = state;
>> +       priv->hw_state = priv->sw_state = state;
>>         return 0;
>>  }
>>  
>> -static int lcdshadow_set(bool on)
>> +static void lcdshadow_get_hw_state(struct drm_privacy_screen *priv)
>>  {
>> -       if (lcdshadow_state < 0)
>> -               return lcdshadow_state;
>> -       if (lcdshadow_state == on)
>> -               return 0;
>> -       return lcdshadow_on_off(on);
>> +       int output;
>> +
>> +       if (!acpi_evalf(lcdshadow_get_handle, &output, NULL, "dd", 0))
>> +               return;
>> +
>> +       priv->hw_state = priv->sw_state = output & 0x1;
>>  }
>>  
>> +static const struct drm_privacy_screen_ops lcdshadow_ops = {
>> +       .set_sw_state = lcdshadow_set_sw_state,
>> +       .get_hw_state = lcdshadow_get_hw_state,
>> +};
>> +
>>  static int tpacpi_lcdshadow_init(struct ibm_init_struct *iibm)
>>  {
>>         acpi_status status1, status2;
>> @@ -9850,36 +9868,44 @@ static int tpacpi_lcdshadow_init(struct
>> ibm_init_struct *iibm)
>>  
>>         status1 = acpi_get_handle(hkey_handle, "GSSS",
>> &lcdshadow_get_handle);
>>         status2 = acpi_get_handle(hkey_handle, "SSSS",
>> &lcdshadow_set_handle);
>> -       if (ACPI_FAILURE(status1) || ACPI_FAILURE(status2)) {
>> -               lcdshadow_state = -ENODEV;
>> +       if (ACPI_FAILURE(status1) || ACPI_FAILURE(status2))
>>                 return 0;
>> -       }
>>  
>> -       if (!acpi_evalf(lcdshadow_get_handle, &output, NULL, "dd", 0)) {
>> -               lcdshadow_state = -EIO;
>> +       if (!acpi_evalf(lcdshadow_get_handle, &output, NULL, "dd", 0))
>>                 return -EIO;
>> -       }
>> -       if (!(output & 0x10000)) {
>> -               lcdshadow_state = -ENODEV;
>> +
>> +       if (!(output & 0x10000))
>>                 return 0;
>> -       }
>> -       lcdshadow_state = output & 0x1;
>> +
>> +       lcdshadow_dev = drm_privacy_screen_register(&tpacpi_pdev->dev,
>> +                                                   &lcdshadow_ops);
>> +       if (IS_ERR(lcdshadow_dev))
>> +               return PTR_ERR(lcdshadow_dev);
>>  
>>         return 0;
>>  }
>>  
>> +static void lcdshadow_exit(void)
>> +{
>> +       drm_privacy_screen_unregister(lcdshadow_dev);
>> +}
>> +
>>  static void lcdshadow_resume(void)
>>  {
>> -       if (lcdshadow_state >= 0)
>> -               lcdshadow_on_off(lcdshadow_state);
>> +       if (!lcdshadow_dev)
>> +               return;
>> +
>> +       mutex_lock(&lcdshadow_dev->lock);
>> +       lcdshadow_set_sw_state(lcdshadow_dev, lcdshadow_dev->sw_state);
>> +       mutex_unlock(&lcdshadow_dev->lock);
>>  }
>>  
> 
> For privacy screens provided by x86 platform drivers this is -probably-
> correct, but only so long as we're confident that the privacy screen is always
> going to be controllable regardless of the power state of the actual LCD
> panel.

Right, in this case the privacy-screen control is entirely independent
of the actual LCD state. Also notice that this code does not introduce
the re-storing of the privacy-screen state, that was already there, it
merely changes it to go through the new drm_privacy_screen API.


> I'd think we would need to handle suspend/resume in the atomic commit though
> if we ever have to support systems where the two are dependent on one another,
> but, that's a simple enough change to do later if it arises that I think we
> can ignore it for now.

Ack.

Regards,

Hans



> 
>>  static int lcdshadow_read(struct seq_file *m)
>>  {
>> -       if (lcdshadow_state < 0) {
>> +       if (!lcdshadow_dev) {
>>                 seq_puts(m, "status:\t\tnot supported\n");
>>         } else {
>> -               seq_printf(m, "status:\t\t%d\n", lcdshadow_state);
>> +               seq_printf(m, "status:\t\t%d\n", lcdshadow_dev->hw_state);
>>                 seq_puts(m, "commands:\t0, 1\n");
>>         }
>>  
>> @@ -9891,7 +9917,7 @@ static int lcdshadow_write(char *buf)
>>         char *cmd;
>>         int res, state = -EINVAL;
>>  
>> -       if (lcdshadow_state < 0)
>> +       if (!lcdshadow_dev)
>>                 return -ENODEV;
>>  
>>         while ((cmd = strsep(&buf, ","))) {
>> @@ -9903,11 +9929,18 @@ static int lcdshadow_write(char *buf)
>>         if (state >= 2 || state < 0)
>>                 return -EINVAL;
>>  
>> -       return lcdshadow_set(state);
>> +       mutex_lock(&lcdshadow_dev->lock);
>> +       res = lcdshadow_set_sw_state(lcdshadow_dev, state);
>> +       mutex_unlock(&lcdshadow_dev->lock);
>> +
>> +       drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
>> +
>> +       return res;
>>  }
>>  
>>  static struct ibm_struct lcdshadow_driver_data = {
>>         .name = "lcdshadow",
>> +       .exit = lcdshadow_exit,
>>         .resume = lcdshadow_resume,
>>         .read = lcdshadow_read,
>>         .write = lcdshadow_write,
>> @@ -10717,6 +10750,14 @@ static void tpacpi_driver_event(const unsigned int
>> hkey_event)
>>                 if (!atomic_add_unless(&dytc_ignore_event, -1, 0))
>>                         dytc_profile_refresh();
>>         }
>> +
>> +       if (lcdshadow_dev && hkey_event == TP_HKEY_EV_PRIVACYGUARD_TOGGLE) {
>> +               mutex_lock(&lcdshadow_dev->lock);
>> +               lcdshadow_get_hw_state(lcdshadow_dev);
>> +               mutex_unlock(&lcdshadow_dev->lock);
>> +
>> +               drm_privacy_screen_call_notifier_chain(lcdshadow_dev);
>> +       }
>>  }
>>  
>>  static void hotkey_driver_event(const unsigned int scancode)
> 




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux