Certain laptops now come with panels that have integrated privacy screens on them. This patch adds support for such panels by adding a privacy-screen property to the intel_connector for the panel, that the userspace can then use to control and check the status.
Identifying the presence of privacy screen, and controlling it, is done via ACPI _DSM methods.
Currently, this is done only for the Intel display ports. But in future, this can be done for any other ports if the hardware becomes available (e.g. external monitors supporting integrated privacy screens?).
Signed-off-by: Rajat Jain rajatja@google.com --- v6: Always initialize prop in intel_attach_privacy_screen_property() v5: fix a cosmetic checkpatch warning v4: Fix a typo in intel_privacy_screen.h v3: * Change license to GPL-2.0 OR MIT * Move privacy screen enum from UAPI to intel_display_types.h * Rename parameter name and some other minor changes. v2: Formed by splitting the original patch into multiple patches. - All code has been moved into i915 now. - Privacy screen is a i915 property - Have a local state variable to store the prvacy screen. Don't read it from hardware.
drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/display/intel_atomic.c | 13 +++- .../gpu/drm/i915/display/intel_connector.c | 35 +++++++++ .../gpu/drm/i915/display/intel_connector.h | 1 + .../drm/i915/display/intel_display_types.h | 18 +++++ drivers/gpu/drm/i915/display/intel_dp.c | 6 ++ .../drm/i915/display/intel_privacy_screen.c | 72 +++++++++++++++++++ .../drm/i915/display/intel_privacy_screen.h | 27 +++++++ 8 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.c create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 991a8c537d123..825951b4cd006 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -208,7 +208,8 @@ i915-y += \ display/intel_vga.o i915-$(CONFIG_ACPI) += \ display/intel_acpi.o \ - display/intel_opregion.o + display/intel_opregion.o \ + display/intel_privacy_screen.o i915-$(CONFIG_DRM_FBDEV_EMULATION) += \ display/intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index d043057d2fa03..4ed537c877777 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -40,6 +40,7 @@ #include "intel_global_state.h" #include "intel_hdcp.h" #include "intel_psr.h" +#include "intel_privacy_screen.h" #include "intel_sprite.h"
/** @@ -60,11 +61,14 @@ int intel_digital_connector_atomic_get_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state); + struct intel_connector *intel_connector = to_intel_connector(connector);
if (property == dev_priv->force_audio_property) *val = intel_conn_state->force_audio; else if (property == dev_priv->broadcast_rgb_property) *val = intel_conn_state->broadcast_rgb; + else if (property == intel_connector->privacy_screen_property) + *val = intel_conn_state->privacy_screen_status; else { drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n", @@ -93,15 +97,18 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state); + struct intel_connector *intel_connector = to_intel_connector(connector);
if (property == dev_priv->force_audio_property) { intel_conn_state->force_audio = val; return 0; - } - - if (property == dev_priv->broadcast_rgb_property) { + } else if (property == dev_priv->broadcast_rgb_property) { intel_conn_state->broadcast_rgb = val; return 0; + } else if (property == intel_connector->privacy_screen_property) { + intel_privacy_screen_set_val(intel_connector, val); + intel_conn_state->privacy_screen_status = val; + return 0; }
drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n", diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 903e49659f561..55f80219cb269 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -297,3 +297,38 @@ intel_attach_colorspace_property(struct drm_connector *connector) drm_object_attach_property(&connector->base, connector->colorspace_property, 0); } + +static const struct drm_prop_enum_list privacy_screen_enum[] = { + { PRIVACY_SCREEN_DISABLED, "Disabled" }, + { PRIVACY_SCREEN_ENABLED, "Enabled" }, +}; + +/** + * intel_attach_privacy_screen_property - + * create and attach the connecter's privacy-screen property. * + * @connector: connector for which to init the privacy-screen property + * + * This function creates and attaches the "privacy-screen" property to the + * connector. Initial state of privacy-screen is set to disabled. + */ +void +intel_attach_privacy_screen_property(struct drm_connector *connector) +{ + struct intel_connector *intel_connector = to_intel_connector(connector); + struct drm_property *prop = intel_connector->privacy_screen_property; + + if (!prop) { + prop = drm_property_create_enum(connector->dev, + DRM_MODE_PROP_ENUM, + "privacy-screen", + privacy_screen_enum, + ARRAY_SIZE(privacy_screen_enum)); + if (!prop) + return; + + intel_connector->privacy_screen_property = prop; + } + + drm_object_attach_property(&connector->base, prop, + PRIVACY_SCREEN_DISABLED); +} diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h index 93a7375c8196d..61005f37a3385 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.h +++ b/drivers/gpu/drm/i915/display/intel_connector.h @@ -31,5 +31,6 @@ void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); void intel_attach_colorspace_property(struct drm_connector *connector); +void intel_attach_privacy_screen_property(struct drm_connector *connector);
#endif /* __INTEL_CONNECTOR_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index d70612cc1ba2a..de20effb3e073 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -442,6 +442,23 @@ struct intel_connector { struct work_struct modeset_retry_work;
struct intel_hdcp hdcp; + + /* Optional "privacy-screen" property for the connector panel */ + struct drm_property *privacy_screen_property; +}; + +/** + * enum intel_privacy_screen_status - privacy_screen status + * + * This enum is used to track and control the state of the integrated privacy + * screen present on some display panels, via the "privacy-screen" property. + * + * @PRIVACY_SCREEN_DISABLED: The privacy-screen on the panel is disabled + * @PRIVACY_SCREEN_ENABLED: The privacy-screen on the panel is enabled + **/ +enum intel_privacy_screen_status { + PRIVACY_SCREEN_DISABLED = 0, + PRIVACY_SCREEN_ENABLED = 1, };
struct intel_digital_connector_state { @@ -449,6 +466,7 @@ struct intel_digital_connector_state {
enum hdmi_force_audio force_audio; int broadcast_rgb; + enum intel_privacy_screen_status privacy_screen_status; };
#define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base) diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 171821113d362..ff76c799364d0 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -62,6 +62,7 @@ #include "intel_lspcon.h" #include "intel_lvds.h" #include "intel_panel.h" +#include "intel_privacy_screen.h" #include "intel_psr.h" #include "intel_sideband.h" #include "intel_tc.h" @@ -6841,6 +6842,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect { struct drm_i915_private *dev_priv = to_i915(connector->dev); enum port port = dp_to_dig_port(intel_dp)->base.port; + struct intel_connector *intel_connector = to_intel_connector(connector);
if (!IS_G4X(dev_priv) && port != PORT_A) intel_attach_force_audio_property(connector); @@ -6871,6 +6873,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
/* Lookup the ACPI node corresponding to the connector */ intel_acpi_device_id_update(dev_priv); + + /* Check for integrated Privacy screen support */ + if (intel_privacy_screen_present(intel_connector)) + intel_attach_privacy_screen_property(connector); } }
diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.c b/drivers/gpu/drm/i915/display/intel_privacy_screen.c new file mode 100644 index 0000000000000..c8a5b64f94fb7 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/* + * Intel ACPI privacy screen code + * + * Copyright © 2019 Google Inc. + */ + +#include <linux/acpi.h> + +#include "intel_privacy_screen.h" + +#define CONNECTOR_DSM_REVID 1 + +#define CONNECTOR_DSM_FN_PRIVACY_ENABLE 2 +#define CONNECTOR_DSM_FN_PRIVACY_DISABLE 3 + +static const guid_t drm_conn_dsm_guid = + GUID_INIT(0xC7033113, 0x8720, 0x4CEB, + 0x90, 0x90, 0x9D, 0x52, 0xB3, 0xE5, 0x2D, 0x73); + +/* Makes _DSM call to set privacy screen status */ +static void acpi_privacy_screen_call_dsm(acpi_handle conn_handle, u64 func) +{ + union acpi_object *obj; + + obj = acpi_evaluate_dsm(conn_handle, &drm_conn_dsm_guid, + CONNECTOR_DSM_REVID, func, NULL); + if (!obj) { + DRM_DEBUG_DRIVER("failed to evaluate _DSM for fn %llx\n", func); + return; + } + + ACPI_FREE(obj); +} + +void intel_privacy_screen_set_val(struct intel_connector *connector, + enum intel_privacy_screen_status val) +{ + acpi_handle acpi_handle = connector->acpi_handle; + + if (!acpi_handle) + return; + + if (val == PRIVACY_SCREEN_DISABLED) + acpi_privacy_screen_call_dsm(acpi_handle, + CONNECTOR_DSM_FN_PRIVACY_DISABLE); + else if (val == PRIVACY_SCREEN_ENABLED) + acpi_privacy_screen_call_dsm(acpi_handle, + CONNECTOR_DSM_FN_PRIVACY_ENABLE); + else + DRM_WARN("%s: Cannot set privacy screen to invalid val %u\n", + dev_name(connector->base.dev->dev), val); +} + +bool intel_privacy_screen_present(struct intel_connector *connector) +{ + acpi_handle handle = connector->acpi_handle; + + if (!handle) + return false; + + if (!acpi_check_dsm(handle, &drm_conn_dsm_guid, + CONNECTOR_DSM_REVID, + 1 << CONNECTOR_DSM_FN_PRIVACY_ENABLE | + 1 << CONNECTOR_DSM_FN_PRIVACY_DISABLE)) { + DRM_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n", + dev_name(connector->base.dev->dev)); + return false; + } + DRM_DEV_INFO(connector->base.dev->dev, "supports privacy screen\n"); + return true; +} diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.h b/drivers/gpu/drm/i915/display/intel_privacy_screen.h new file mode 100644 index 0000000000000..74013a7885c70 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/* + * Copyright © 2019 Google Inc. + */ + +#ifndef __DRM_PRIVACY_SCREEN_H__ +#define __DRM_PRIVACY_SCREEN_H__ + +#include "intel_display_types.h" + +#ifdef CONFIG_ACPI +bool intel_privacy_screen_present(struct intel_connector *connector); +void intel_privacy_screen_set_val(struct intel_connector *connector, + enum intel_privacy_screen_status val); +#else +static bool intel_privacy_screen_present(struct intel_connector *connector) +{ + return false; +} + +static void +intel_privacy_screen_set_val(struct intel_connector *connector, + enum intel_privacy_screen_status val) +{ } +#endif /* CONFIG_ACPI */ + +#endif /* __DRM_PRIVACY_SCREEN_H__ */
On Wed, 04 Mar 2020, Rajat Jain rajatja@google.com wrote:
Certain laptops now come with panels that have integrated privacy screens on them. This patch adds support for such panels by adding a privacy-screen property to the intel_connector for the panel, that the userspace can then use to control and check the status.
Identifying the presence of privacy screen, and controlling it, is done via ACPI _DSM methods.
Currently, this is done only for the Intel display ports. But in future, this can be done for any other ports if the hardware becomes available (e.g. external monitors supporting integrated privacy screens?).
I think you should add the property at the drm core level in drm_connector.c, not in i915, to ensure we have the same property across drivers. Even if, for now, you leave the acpi implementation part in i915.
More comments inline.
Signed-off-by: Rajat Jain rajatja@google.com
v6: Always initialize prop in intel_attach_privacy_screen_property() v5: fix a cosmetic checkpatch warning v4: Fix a typo in intel_privacy_screen.h v3: * Change license to GPL-2.0 OR MIT * Move privacy screen enum from UAPI to intel_display_types.h * Rename parameter name and some other minor changes. v2: Formed by splitting the original patch into multiple patches. - All code has been moved into i915 now. - Privacy screen is a i915 property - Have a local state variable to store the prvacy screen. Don't read it from hardware.
drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/display/intel_atomic.c | 13 +++- .../gpu/drm/i915/display/intel_connector.c | 35 +++++++++ .../gpu/drm/i915/display/intel_connector.h | 1 + .../drm/i915/display/intel_display_types.h | 18 +++++ drivers/gpu/drm/i915/display/intel_dp.c | 6 ++ .../drm/i915/display/intel_privacy_screen.c | 72 +++++++++++++++++++ .../drm/i915/display/intel_privacy_screen.h | 27 +++++++ 8 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.c create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 991a8c537d123..825951b4cd006 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -208,7 +208,8 @@ i915-y += \ display/intel_vga.o i915-$(CONFIG_ACPI) += \ display/intel_acpi.o \
- display/intel_opregion.o
- display/intel_opregion.o \
- display/intel_privacy_screen.o
i915-$(CONFIG_DRM_FBDEV_EMULATION) += \ display/intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index d043057d2fa03..4ed537c877777 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -40,6 +40,7 @@ #include "intel_global_state.h" #include "intel_hdcp.h" #include "intel_psr.h" +#include "intel_privacy_screen.h" #include "intel_sprite.h"
/** @@ -60,11 +61,14 @@ int intel_digital_connector_atomic_get_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state);
struct intel_connector *intel_connector = to_intel_connector(connector);
if (property == dev_priv->force_audio_property) *val = intel_conn_state->force_audio; else if (property == dev_priv->broadcast_rgb_property) *val = intel_conn_state->broadcast_rgb;
else if (property == intel_connector->privacy_screen_property)
*val = intel_conn_state->privacy_screen_status;else { drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n",
@@ -93,15 +97,18 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state);
struct intel_connector *intel_connector = to_intel_connector(connector);
if (property == dev_priv->force_audio_property) { intel_conn_state->force_audio = val; return 0;
- }
- if (property == dev_priv->broadcast_rgb_property) {
- } else if (property == dev_priv->broadcast_rgb_property) { intel_conn_state->broadcast_rgb = val; return 0;
- } else if (property == intel_connector->privacy_screen_property) {
intel_privacy_screen_set_val(intel_connector, val);
I think this part should only change the connector state. The driver would then do the magic at commit stage according to the property value.
intel_conn_state->privacy_screen_status = val;return 0;}
drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n",
diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 903e49659f561..55f80219cb269 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -297,3 +297,38 @@ intel_attach_colorspace_property(struct drm_connector *connector) drm_object_attach_property(&connector->base, connector->colorspace_property, 0); }
+static const struct drm_prop_enum_list privacy_screen_enum[] = {
- { PRIVACY_SCREEN_DISABLED, "Disabled" },
- { PRIVACY_SCREEN_ENABLED, "Enabled" },
+};
+/**
- intel_attach_privacy_screen_property -
create and attach the connecter's privacy-screen property. *
- @connector: connector for which to init the privacy-screen property
- This function creates and attaches the "privacy-screen" property to the
- connector. Initial state of privacy-screen is set to disabled.
- */
+void +intel_attach_privacy_screen_property(struct drm_connector *connector) +{
- struct intel_connector *intel_connector = to_intel_connector(connector);
- struct drm_property *prop = intel_connector->privacy_screen_property;
- if (!prop) {
prop = drm_property_create_enum(connector->dev,DRM_MODE_PROP_ENUM,"privacy-screen",privacy_screen_enum,ARRAY_SIZE(privacy_screen_enum));if (!prop)return;intel_connector->privacy_screen_property = prop;- }
- drm_object_attach_property(&connector->base, prop,
PRIVACY_SCREEN_DISABLED);+}
This looks about right, except IMO should be part of drm_connector and moved to drm_connector.c.
diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h index 93a7375c8196d..61005f37a3385 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.h +++ b/drivers/gpu/drm/i915/display/intel_connector.h @@ -31,5 +31,6 @@ void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); void intel_attach_colorspace_property(struct drm_connector *connector); +void intel_attach_privacy_screen_property(struct drm_connector *connector);
#endif /* __INTEL_CONNECTOR_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index d70612cc1ba2a..de20effb3e073 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -442,6 +442,23 @@ struct intel_connector { struct work_struct modeset_retry_work;
struct intel_hdcp hdcp;
- /* Optional "privacy-screen" property for the connector panel */
- struct drm_property *privacy_screen_property;
+};
+/**
- enum intel_privacy_screen_status - privacy_screen status
- This enum is used to track and control the state of the integrated privacy
- screen present on some display panels, via the "privacy-screen" property.
- @PRIVACY_SCREEN_DISABLED: The privacy-screen on the panel is disabled
- @PRIVACY_SCREEN_ENABLED: The privacy-screen on the panel is enabled
- **/
+enum intel_privacy_screen_status {
- PRIVACY_SCREEN_DISABLED = 0,
- PRIVACY_SCREEN_ENABLED = 1,
};
struct intel_digital_connector_state { @@ -449,6 +466,7 @@ struct intel_digital_connector_state {
enum hdmi_force_audio force_audio; int broadcast_rgb;
- enum intel_privacy_screen_status privacy_screen_status;
};
#define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base) diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 171821113d362..ff76c799364d0 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -62,6 +62,7 @@ #include "intel_lspcon.h" #include "intel_lvds.h" #include "intel_panel.h" +#include "intel_privacy_screen.h" #include "intel_psr.h" #include "intel_sideband.h" #include "intel_tc.h" @@ -6841,6 +6842,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect { struct drm_i915_private *dev_priv = to_i915(connector->dev); enum port port = dp_to_dig_port(intel_dp)->base.port;
struct intel_connector *intel_connector = to_intel_connector(connector);
if (!IS_G4X(dev_priv) && port != PORT_A) intel_attach_force_audio_property(connector);
@@ -6871,6 +6873,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
/* Lookup the ACPI node corresponding to the connector */ intel_acpi_device_id_update(dev_priv);
/* Check for integrated Privacy screen support */if (intel_privacy_screen_present(intel_connector))intel_attach_privacy_screen_property(connector);
As said in reply to patch 2, we need to figure this part out.
} }
diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.c b/drivers/gpu/drm/i915/display/intel_privacy_screen.c new file mode 100644 index 0000000000000..c8a5b64f94fb7 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/*
- Intel ACPI privacy screen code
- Copyright © 2019 Google Inc.
- */
+#include <linux/acpi.h>
+#include "intel_privacy_screen.h"
+#define CONNECTOR_DSM_REVID 1
+#define CONNECTOR_DSM_FN_PRIVACY_ENABLE 2 +#define CONNECTOR_DSM_FN_PRIVACY_DISABLE 3
+static const guid_t drm_conn_dsm_guid =
- GUID_INIT(0xC7033113, 0x8720, 0x4CEB,
0x90, 0x90, 0x9D, 0x52, 0xB3, 0xE5, 0x2D, 0x73);+/* Makes _DSM call to set privacy screen status */ +static void acpi_privacy_screen_call_dsm(acpi_handle conn_handle, u64 func) +{
- union acpi_object *obj;
- obj = acpi_evaluate_dsm(conn_handle, &drm_conn_dsm_guid,
CONNECTOR_DSM_REVID, func, NULL);- if (!obj) {
DRM_DEBUG_DRIVER("failed to evaluate _DSM for fn %llx\n", func);return;- }
- ACPI_FREE(obj);
+}
+void intel_privacy_screen_set_val(struct intel_connector *connector,
enum intel_privacy_screen_status val)+{
- acpi_handle acpi_handle = connector->acpi_handle;
- if (!acpi_handle)
return;- if (val == PRIVACY_SCREEN_DISABLED)
acpi_privacy_screen_call_dsm(acpi_handle,CONNECTOR_DSM_FN_PRIVACY_DISABLE);- else if (val == PRIVACY_SCREEN_ENABLED)
acpi_privacy_screen_call_dsm(acpi_handle,CONNECTOR_DSM_FN_PRIVACY_ENABLE);- else
DRM_WARN("%s: Cannot set privacy screen to invalid val %u\n",dev_name(connector->base.dev->dev), val);+}
+bool intel_privacy_screen_present(struct intel_connector *connector) +{
- acpi_handle handle = connector->acpi_handle;
- if (!handle)
return false;- if (!acpi_check_dsm(handle, &drm_conn_dsm_guid,
CONNECTOR_DSM_REVID,1 << CONNECTOR_DSM_FN_PRIVACY_ENABLE |1 << CONNECTOR_DSM_FN_PRIVACY_DISABLE)) {DRM_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",dev_name(connector->base.dev->dev));return false;- }
- DRM_DEV_INFO(connector->base.dev->dev, "supports privacy screen\n");
- return true;
+}
I don't have much to say about the ACPI parts, except please use the new drm_dbg_kms and drm_info helpers for logging.
BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.h b/drivers/gpu/drm/i915/display/intel_privacy_screen.h new file mode 100644 index 0000000000000..74013a7885c70 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/*
- Copyright © 2019 Google Inc.
- */
+#ifndef __DRM_PRIVACY_SCREEN_H__ +#define __DRM_PRIVACY_SCREEN_H__
+#include "intel_display_types.h"
+#ifdef CONFIG_ACPI +bool intel_privacy_screen_present(struct intel_connector *connector); +void intel_privacy_screen_set_val(struct intel_connector *connector,
enum intel_privacy_screen_status val);+#else +static bool intel_privacy_screen_present(struct intel_connector *connector) +{
- return false;
+}
+static void +intel_privacy_screen_set_val(struct intel_connector *connector,
enum intel_privacy_screen_status val)+{ } +#endif /* CONFIG_ACPI */
+#endif /* __DRM_PRIVACY_SCREEN_H__ */
Hi Jani,
Thank you for the comments. Please see my responses inline.
On Thu, Mar 5, 2020 at 2:02 AM Jani Nikula jani.nikula@linux.intel.com wrote:
On Wed, 04 Mar 2020, Rajat Jain rajatja@google.com wrote:
Certain laptops now come with panels that have integrated privacy screens on them. This patch adds support for such panels by adding a privacy-screen property to the intel_connector for the panel, that the userspace can then use to control and check the status.
Identifying the presence of privacy screen, and controlling it, is done via ACPI _DSM methods.
Currently, this is done only for the Intel display ports. But in future, this can be done for any other ports if the hardware becomes available (e.g. external monitors supporting integrated privacy screens?).
I think you should add the property at the drm core level in drm_connector.c, not in i915, to ensure we have the same property across drivers. Even if, for now, you leave the acpi implementation part in i915.
OK, will do. In order to do that I may need to introduce driver level hooks that i915 driver can populate and drm core can call (or may be some functions to add privacy screen property that drm core exports and i915 driver will call).
More comments inline.
Signed-off-by: Rajat Jain rajatja@google.com
v6: Always initialize prop in intel_attach_privacy_screen_property() v5: fix a cosmetic checkpatch warning v4: Fix a typo in intel_privacy_screen.h v3: * Change license to GPL-2.0 OR MIT * Move privacy screen enum from UAPI to intel_display_types.h * Rename parameter name and some other minor changes. v2: Formed by splitting the original patch into multiple patches. - All code has been moved into i915 now. - Privacy screen is a i915 property - Have a local state variable to store the prvacy screen. Don't read it from hardware.
drivers/gpu/drm/i915/Makefile | 3 +- drivers/gpu/drm/i915/display/intel_atomic.c | 13 +++- .../gpu/drm/i915/display/intel_connector.c | 35 +++++++++ .../gpu/drm/i915/display/intel_connector.h | 1 + .../drm/i915/display/intel_display_types.h | 18 +++++ drivers/gpu/drm/i915/display/intel_dp.c | 6 ++ .../drm/i915/display/intel_privacy_screen.c | 72 +++++++++++++++++++ .../drm/i915/display/intel_privacy_screen.h | 27 +++++++ 8 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.c create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 991a8c537d123..825951b4cd006 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -208,7 +208,8 @@ i915-y += \ display/intel_vga.o i915-$(CONFIG_ACPI) += \ display/intel_acpi.o \
display/intel_opregion.o
display/intel_opregion.o \display/intel_privacy_screen.oi915-$(CONFIG_DRM_FBDEV_EMULATION) += \ display/intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index d043057d2fa03..4ed537c877777 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -40,6 +40,7 @@ #include "intel_global_state.h" #include "intel_hdcp.h" #include "intel_psr.h" +#include "intel_privacy_screen.h" #include "intel_sprite.h"
/** @@ -60,11 +61,14 @@ int intel_digital_connector_atomic_get_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state);
struct intel_connector *intel_connector = to_intel_connector(connector); if (property == dev_priv->force_audio_property) *val = intel_conn_state->force_audio; else if (property == dev_priv->broadcast_rgb_property) *val = intel_conn_state->broadcast_rgb;else if (property == intel_connector->privacy_screen_property)*val = intel_conn_state->privacy_screen_status; else { drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n",@@ -93,15 +97,18 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state);
struct intel_connector *intel_connector = to_intel_connector(connector); if (property == dev_priv->force_audio_property) { intel_conn_state->force_audio = val; return 0;
}if (property == dev_priv->broadcast_rgb_property) {
} else if (property == dev_priv->broadcast_rgb_property) { intel_conn_state->broadcast_rgb = val; return 0;} else if (property == intel_connector->privacy_screen_property) {intel_privacy_screen_set_val(intel_connector, val);I think this part should only change the connector state. The driver would then do the magic at commit stage according to the property value.
Can you please point me to some code reference as to where in code does the "commit stage" apply the changes?
intel_conn_state->privacy_screen_status = val;return 0; } drm_dbg_atomic(&dev_priv->drm, "Unknown property [PROP:%d:%s]\n",diff --git a/drivers/gpu/drm/i915/display/intel_connector.c b/drivers/gpu/drm/i915/display/intel_connector.c index 903e49659f561..55f80219cb269 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.c +++ b/drivers/gpu/drm/i915/display/intel_connector.c @@ -297,3 +297,38 @@ intel_attach_colorspace_property(struct drm_connector *connector) drm_object_attach_property(&connector->base, connector->colorspace_property, 0); }
+static const struct drm_prop_enum_list privacy_screen_enum[] = {
{ PRIVACY_SCREEN_DISABLED, "Disabled" },{ PRIVACY_SCREEN_ENABLED, "Enabled" },+};
+/**
- intel_attach_privacy_screen_property -
create and attach the connecter's privacy-screen property. *
- @connector: connector for which to init the privacy-screen property
- This function creates and attaches the "privacy-screen" property to the
- connector. Initial state of privacy-screen is set to disabled.
- */
+void +intel_attach_privacy_screen_property(struct drm_connector *connector) +{
struct intel_connector *intel_connector = to_intel_connector(connector);struct drm_property *prop = intel_connector->privacy_screen_property;if (!prop) {prop = drm_property_create_enum(connector->dev,DRM_MODE_PROP_ENUM,"privacy-screen",privacy_screen_enum,ARRAY_SIZE(privacy_screen_enum));if (!prop)return;intel_connector->privacy_screen_property = prop;}drm_object_attach_property(&connector->base, prop,PRIVACY_SCREEN_DISABLED);+}
This looks about right, except IMO should be part of drm_connector and moved to drm_connector.c.
Will do.
diff --git a/drivers/gpu/drm/i915/display/intel_connector.h b/drivers/gpu/drm/i915/display/intel_connector.h index 93a7375c8196d..61005f37a3385 100644 --- a/drivers/gpu/drm/i915/display/intel_connector.h +++ b/drivers/gpu/drm/i915/display/intel_connector.h @@ -31,5 +31,6 @@ void intel_attach_force_audio_property(struct drm_connector *connector); void intel_attach_broadcast_rgb_property(struct drm_connector *connector); void intel_attach_aspect_ratio_property(struct drm_connector *connector); void intel_attach_colorspace_property(struct drm_connector *connector); +void intel_attach_privacy_screen_property(struct drm_connector *connector);
#endif /* __INTEL_CONNECTOR_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index d70612cc1ba2a..de20effb3e073 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -442,6 +442,23 @@ struct intel_connector { struct work_struct modeset_retry_work;
struct intel_hdcp hdcp;
/* Optional "privacy-screen" property for the connector panel */struct drm_property *privacy_screen_property;+};
+/**
- enum intel_privacy_screen_status - privacy_screen status
- This enum is used to track and control the state of the integrated privacy
- screen present on some display panels, via the "privacy-screen" property.
- @PRIVACY_SCREEN_DISABLED: The privacy-screen on the panel is disabled
- @PRIVACY_SCREEN_ENABLED: The privacy-screen on the panel is enabled
- **/
+enum intel_privacy_screen_status {
PRIVACY_SCREEN_DISABLED = 0,PRIVACY_SCREEN_ENABLED = 1,};
struct intel_digital_connector_state { @@ -449,6 +466,7 @@ struct intel_digital_connector_state {
enum hdmi_force_audio force_audio; int broadcast_rgb;
enum intel_privacy_screen_status privacy_screen_status;};
#define to_intel_digital_connector_state(x) container_of(x, struct intel_digital_connector_state, base) diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 171821113d362..ff76c799364d0 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -62,6 +62,7 @@ #include "intel_lspcon.h" #include "intel_lvds.h" #include "intel_panel.h" +#include "intel_privacy_screen.h" #include "intel_psr.h" #include "intel_sideband.h" #include "intel_tc.h" @@ -6841,6 +6842,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect { struct drm_i915_private *dev_priv = to_i915(connector->dev); enum port port = dp_to_dig_port(intel_dp)->base.port;
struct intel_connector *intel_connector = to_intel_connector(connector); if (!IS_G4X(dev_priv) && port != PORT_A) intel_attach_force_audio_property(connector);@@ -6871,6 +6873,10 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
/* Lookup the ACPI node corresponding to the connector */ intel_acpi_device_id_update(dev_priv);
/* Check for integrated Privacy screen support */if (intel_privacy_screen_present(intel_connector))intel_attach_privacy_screen_property(connector);As said in reply to patch 2, we need to figure this part out.
Yup, responded in patch 2 reply.
}}
diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.c b/drivers/gpu/drm/i915/display/intel_privacy_screen.c new file mode 100644 index 0000000000000..c8a5b64f94fb7 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT +/*
- Intel ACPI privacy screen code
- Copyright © 2019 Google Inc.
- */
+#include <linux/acpi.h>
+#include "intel_privacy_screen.h"
+#define CONNECTOR_DSM_REVID 1
+#define CONNECTOR_DSM_FN_PRIVACY_ENABLE 2 +#define CONNECTOR_DSM_FN_PRIVACY_DISABLE 3
+static const guid_t drm_conn_dsm_guid =
GUID_INIT(0xC7033113, 0x8720, 0x4CEB,0x90, 0x90, 0x9D, 0x52, 0xB3, 0xE5, 0x2D, 0x73);+/* Makes _DSM call to set privacy screen status */ +static void acpi_privacy_screen_call_dsm(acpi_handle conn_handle, u64 func) +{
union acpi_object *obj;obj = acpi_evaluate_dsm(conn_handle, &drm_conn_dsm_guid,CONNECTOR_DSM_REVID, func, NULL);if (!obj) {DRM_DEBUG_DRIVER("failed to evaluate _DSM for fn %llx\n", func);return;}ACPI_FREE(obj);+}
+void intel_privacy_screen_set_val(struct intel_connector *connector,
enum intel_privacy_screen_status val)+{
acpi_handle acpi_handle = connector->acpi_handle;if (!acpi_handle)return;if (val == PRIVACY_SCREEN_DISABLED)acpi_privacy_screen_call_dsm(acpi_handle,CONNECTOR_DSM_FN_PRIVACY_DISABLE);else if (val == PRIVACY_SCREEN_ENABLED)acpi_privacy_screen_call_dsm(acpi_handle,CONNECTOR_DSM_FN_PRIVACY_ENABLE);elseDRM_WARN("%s: Cannot set privacy screen to invalid val %u\n",dev_name(connector->base.dev->dev), val);+}
+bool intel_privacy_screen_present(struct intel_connector *connector) +{
acpi_handle handle = connector->acpi_handle;if (!handle)return false;if (!acpi_check_dsm(handle, &drm_conn_dsm_guid,CONNECTOR_DSM_REVID,1 << CONNECTOR_DSM_FN_PRIVACY_ENABLE |1 << CONNECTOR_DSM_FN_PRIVACY_DISABLE)) {DRM_WARN("%s: Odd, connector ACPI node but no privacy scrn?\n",dev_name(connector->base.dev->dev));return false;}DRM_DEV_INFO(connector->base.dev->dev, "supports privacy screen\n");return true;+}
I don't have much to say about the ACPI parts, except please use the new drm_dbg_kms and drm_info helpers for logging.
Will do.
Thanks,
Rajat
BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.h b/drivers/gpu/drm/i915/display/intel_privacy_screen.h new file mode 100644 index 0000000000000..74013a7885c70 --- /dev/null +++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ +/*
- Copyright © 2019 Google Inc.
- */
+#ifndef __DRM_PRIVACY_SCREEN_H__ +#define __DRM_PRIVACY_SCREEN_H__
+#include "intel_display_types.h"
+#ifdef CONFIG_ACPI +bool intel_privacy_screen_present(struct intel_connector *connector); +void intel_privacy_screen_set_val(struct intel_connector *connector,
enum intel_privacy_screen_status val);+#else +static bool intel_privacy_screen_present(struct intel_connector *connector) +{
return false;+}
+static void +intel_privacy_screen_set_val(struct intel_connector *connector,
enum intel_privacy_screen_status val)+{ } +#endif /* CONFIG_ACPI */
+#endif /* __DRM_PRIVACY_SCREEN_H__ */
-- Jani Nikula, Intel Open Source Graphics Center
On Thu, 05 Mar 2020, Rajat Jain rajatja@google.com wrote:
OK, will do. In order to do that I may need to introduce driver level hooks that i915 driver can populate and drm core can call (or may be some functions to add privacy screen property that drm core exports and i915 driver will call).
The latter. Look at drm_connector_attach_*() functions in drm_connector.c. i915 (or any other driver) can create and attach the property as needed. drm_atomic_connector_{get,set}_property in drm_atomic_uapi.c need to handle the properties, but *only* to get/set the value in drm_connector_state, nothing more. How that value is actually used is up to the drivers, but the userspace interface will be the same instead of being driver specific.
@@ -93,15 +97,18 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state);
struct intel_connector *intel_connector = to_intel_connector(connector); if (property == dev_priv->force_audio_property) { intel_conn_state->force_audio = val; return 0;
}if (property == dev_priv->broadcast_rgb_property) {
} else if (property == dev_priv->broadcast_rgb_property) { intel_conn_state->broadcast_rgb = val; return 0;} else if (property == intel_connector->privacy_screen_property) {intel_privacy_screen_set_val(intel_connector, val);I think this part should only change the connector state. The driver would then do the magic at commit stage according to the property value.
Also, this would be the part that's done in drm core level.
Can you please point me to some code reference as to where in code does the "commit stage" apply the changes?
Look at, say, drm_connector_attach_scaling_mode_property(). In the getter/setter code it'll just read/change state->scaling_mode. You can use the value in encoder ->enable, ->disable, and ->update_pipe hooks. Enable should enable the privacy screen if desired, disable should probably unconditionally disable the privacy screen while disabling the display, and update should just change the state according to the value. Update is called if there isn't a full modeset. (Scaling mode is a bit more indirect than that, affecting other things in the encoder ->compute_config hook, leading to similar effects.)
Ville, anything I missed?
BR, Jani.
Thanks Jani so much for the detailed explanation.
I was able to write the code for this, but I am facing one problem, see below.
On Fri, Mar 6, 2020 at 2:15 AM Jani Nikula jani.nikula@linux.intel.com wrote:
On Thu, 05 Mar 2020, Rajat Jain rajatja@google.com wrote:
OK, will do. In order to do that I may need to introduce driver level hooks that i915 driver can populate and drm core can call (or may be some functions to add privacy screen property that drm core exports and i915 driver will call).
The latter. Look at drm_connector_attach_*() functions in drm_connector.c. i915 (or any other driver) can create and attach the property as needed. drm_atomic_connector_{get,set}_property in drm_atomic_uapi.c need to handle the properties, but *only* to get/set the value in drm_connector_state, nothing more. How that value is actually used is up to the drivers, but the userspace interface will be the same instead of being driver specific.
Understood, done.
@@ -93,15 +97,18 @@ int intel_digital_connector_atomic_set_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = to_i915(dev); struct intel_digital_connector_state *intel_conn_state = to_intel_digital_connector_state(state);
struct intel_connector *intel_connector = to_intel_connector(connector); if (property == dev_priv->force_audio_property) { intel_conn_state->force_audio = val; return 0;
}if (property == dev_priv->broadcast_rgb_property) {
} else if (property == dev_priv->broadcast_rgb_property) { intel_conn_state->broadcast_rgb = val; return 0;} else if (property == intel_connector->privacy_screen_property) {intel_privacy_screen_set_val(intel_connector, val);I think this part should only change the connector state. The driver would then do the magic at commit stage according to the property value.
Also, this would be the part that's done in drm core level.
Yup.
Can you please point me to some code reference as to where in code does the "commit stage" apply the changes?
Look at, say, drm_connector_attach_scaling_mode_property(). In the getter/setter code it'll just read/change state->scaling_mode. You can use the value in encoder ->enable, ->disable, and ->update_pipe hooks. Enable should enable the privacy screen if desired, disable should probably unconditionally disable the privacy screen while disabling the display, and update should just change the state according to the value. Update is called if there isn't a full modeset. (Scaling mode is a bit more indirect than that, affecting other things in the encoder ->compute_config hook, leading to similar effects.)
For my testing purposes, I'm testing this using the proptest utility in our distribution (I think from https://github.com/CPFL/drm/blob/master/tests/proptest/proptest.c). I notice that when I change the value of the property from userspace, even though the drm_connector_state->privacy_screen_status gets updated and reflects the change, the encoder->update_pipe() is not getting called. Just wanted to ask if this is expected since you seem to imply this update_pipe() might *not* get called if there *is* a full modeset? (What is the hook that gets called for a full modeset where i915 driver should commit this property change to the hardware?)
Thanks & Best Regards,
Rajat
Ville, anything I missed?
BR, Jani.
-- Jani Nikula, Intel Open Source Graphics Center
dri-devel@lists.freedesktop.org
-
Jani Nikula -
Rajat Jain