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