[RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Hans de Goede]

Hi All,

This RFC takes Rajat's earlier patch for adding privacy-screen properties
infra to drm_connector.c and then adds the results of the discussion from
the "RFC: Drm-connector properties managed by another driver / privacy
screen support" mail thread on top, hence the v2.

The most important thing here is big kernel-doc comment which gets added in
the first patch-chunk modifying drm_connector.c, this summarizes, or at
least tries to summarize, the conclusions of our previous discussion on
the userspace API and lays down the ground rules for how the 2 new
"privacy-screen sw-state" and  "privacy-screen hw-state" properties are
to be used both from the driver side as well as from the userspace side.

Other then that this modifies Rajat's patch to add 2 properties instead
of one, without much other changes.

Rajat, perhaps you can do a new version of your patch-set integration /
using this version of the properties and then if everyone is ok with
the proposed userspace API Jani can hopefully merge the whole set
through the i915 tree sometime during the 5.9 cycle.

Regards,

Hans

p.s.

I plan to start working on the lcdshadow subsystem next. As discussed the
plan for this subsystem is to allow drivers outside of the DRM subsys, such
as for example the thinkpad_acpi driver, to register a lcdshadow device,
which DRM drivers can then get a reference to and use to implement these
properties.

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

[RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Hans de Goede]

From: Rajat Jain <rajatja@google.com>

Add support for generic electronic privacy screen properties, that
can be added by systems that have an integrated EPS.

Changes in v2 (Hans de Goede)
- Create 2 properties, "privacy-screen sw-state" and
  "privacy-screen hw-state", to deal with devices where the OS might be
  locked out of making state changes
- Write kerneldoc explaining how the 2 properties work together, what
  happens when changes to the state are made outside of the DRM code's
  control, etc.

Signed-off-by: Rajat Jain <rajatja@google.com>
Co-authored-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/gpu/drm-kms.rst     |   2 +
 drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
 drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
 include/drm/drm_connector.h       |  50 +++++++++++++++
 4 files changed, 158 insertions(+)

diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 906771e03103..b72b1e0db343 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -445,6 +445,8 @@ Property Types and Blob Property Support
 .. kernel-doc:: drivers/gpu/drm/drm_property.c
    :export:
 
+.. _standard_connector_properties:
+
 Standard Connector Properties
 -----------------------------
 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index a1e5e262bae2..e56a11208515 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
 						   fence_ptr);
 	} else if (property == connector->max_bpc_property) {
 		state->max_requested_bpc = val;
+	} else if (property == connector->privacy_screen_sw_state_property) {
+		state->privacy_screen_sw_state = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -842,6 +844,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = 0;
 	} else if (property == connector->max_bpc_property) {
 		*val = state->max_requested_bpc;
+	} else if (property == connector->privacy_screen_sw_state_property) {
+		*val = state->privacy_screen_sw_state;
+	} else if (property == connector->privacy_screen_hw_state_property) {
+		*val = state->privacy_screen_hw_state;
 	} else if (connector->funcs->atomic_get_property) {
 		return connector->funcs->atomic_get_property(connector,
 				state, property, val);
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 644f0ad10671..01360edc2376 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *	can also expose this property to external outputs, in which case they
  *	must support "None", which should be the default (since external screens
  *	have a built-in scaler).
+ *
+ * privacy-screen sw-state, privacy-screen hw-state:
+ *	These 2 optional properties can be used to query the state of the
+ *	electronic privacy screen that is available on some displays; and in
+ *	some cases also control the state. If a driver implements these
+ *	properties then both properties must be present.
+ *
+ *	"privacy-screen hw-state" is read-only and reflects the actual state
+ *	of the privacy-screen, possible values: "Enabled", "Disabled,
+ *	"Enabled, locked", "Disabled, locked". The locked states indicate
+ *	that the state cannot be changed through the DRM API. E.g. there
+ *	might be devices where the firmware-setup options, or a hardware
+ *	slider-switch, offer always on / off modes.
+ *
+ *	"privacy-screen sw-state" can be set to change the privacy-screen state
+ *	when not locked. In this case the driver must update the hw-state
+ *	property to reflect the new state on completion of the commit of the
+ *	sw-state property. Setting the sw-state property when the hw-state is
+ *	locked must be interpreted by the driver as a request to change the
+ *	state to the set state when the hw-state becomes unlocked. E.g. if
+ *	"privacy-screen hw-state" is "Enabled, locked" and the sw-state
+ *	gets set to "Disabled" followed by the user unlocking the state by
+ *	changing the slider-switch position, then the driver must set the
+ *	state to "Disabled" upon receiving the unlock event.
+ *
+ *	In some cases the privacy-screen state might change outside of control
+ *	of the DRM code. E.g. there might be a firmware handled hotkey which
+ *	toggles the state, or the state might be changed through another
+ *	userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
+ *	the driver must update both the hw-state and the sw-state to reflect
+ *	the new value, overwriting any pending state requests in the sw-state.
+ *
+ *	Note that the ability for the state to change outside of control of
+ *	the DRM master process means that userspace must not cache the value
+ *	of the sw-state. Ccaching the sw-state value and including it in later
+ *	atomic commits may lead to overriding a state change done through e.g.
+ *	a firmware handled hotkey. Therefor userspace must not include the
+ *	privacy-screen sw-state in an atomic commit unless it wants to change
+ *	its value.
  */
 
 int drm_connector_create_standard_properties(struct drm_device *dev)
@@ -2152,6 +2191,67 @@ int drm_connector_set_panel_orientation_with_quirk(
 }
 EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
 
+static const struct drm_prop_enum_list privacy_screen_enum[] = {
+	{ PRIVACY_SCREEN_DISABLED,		"Disabled" },
+	{ PRIVACY_SCREEN_ENABLED,		"Enabled" },
+	{ PRIVACY_SCREEN_DISABLED_LOCKED,	"Disabled, locked" },
+	{ PRIVACY_SCREEN_ENABLED_LOCKED,	"Enabled, locked" },
+};
+
+/**
+ * drm_connector_create_privacy_screen_properties -
+ *     create the drm connecter's privacy-screen properties.
+ * @connector: connector for which to create the privacy-screen properties
+ *
+ * This function creates the "privacy-screen sw-state" and "privacy-screen
+ * hw-state" properties for the connector. They are not attached.
+ */
+void
+drm_connector_create_privacy_screen_properties(struct drm_connector *connector)
+{
+	if (connector->privacy_screen_sw_state_property)
+		return;
+
+	/* Note sw-state only supports the first 2 values of the enum */
+	connector->privacy_screen_sw_state_property =
+		drm_property_create_enum(connector->dev, DRM_MODE_PROP_ENUM,
+				"privacy-screen sw-state",
+				privacy_screen_enum, 2);
+
+	connector->privacy_screen_hw_state_property =
+		drm_property_create_enum(connector->dev,
+				DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_ENUM,
+				"privacy-screen hw-state",
+				privacy_screen_enum,
+				ARRAY_SIZE(privacy_screen_enum));
+}
+EXPORT_SYMBOL(drm_connector_create_privacy_screen_properties);
+
+/**
+ * drm_connector_attach_privacy_screen_properties -
+ *     attach the drm connecter's privacy-screen properties.
+ * @connector: connector on which to attach the privacy-screen properties
+ *
+ * This function attaches the "privacy-screen sw-state" and "privacy-screen
+ * hw-state" properties to the connector. The initial state of both is set
+ * to "Disabled".
+ */
+void
+drm_connector_attach_privacy_screen_properties(struct drm_connector *connector)
+{
+	if (!connector->privacy_screen_sw_state_property)
+		return;
+
+	drm_object_attach_property(&connector->base,
+				   connector->privacy_screen_sw_state_property,
+				   PRIVACY_SCREEN_DISABLED);
+
+	drm_object_attach_property(&connector->base,
+				   connector->privacy_screen_hw_state_property,
+				   PRIVACY_SCREEN_DISABLED);
+}
+EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
+
 int drm_connector_set_obj_prop(struct drm_mode_object *obj,
 				    struct drm_property *property,
 				    uint64_t value)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 19ae6bb5c85b..a8844f4c6ae9 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -271,6 +271,30 @@ struct drm_monitor_range_info {
 	u8 max_vfreq;
 };
 
+/**
+ * enum drm_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 sw-state"
+ * and "privacy-screen hw-state" properties. Note the _LOCKED enum values
+ * are only valid for the "privacy-screen hw-state" property.
+ *
+ * @PRIVACY_SCREEN_DISABLED:
+ *  The privacy-screen on the panel is disabled
+ * @PRIVACY_SCREEN_ENABLED:
+ *  The privacy-screen on the panel is enabled
+ * @PRIVACY_SCREEN_DISABLED_LOCKED:
+ *  The privacy-screen on the panel is disabled and locked (cannot be changed)
+ * @PRIVACY_SCREEN_ENABLED_LOCKED:
+ *  The privacy-screen on the panel is enabled and locked (cannot be changed)
+ */
+enum drm_privacy_screen_status {
+	PRIVACY_SCREEN_DISABLED = 0,
+	PRIVACY_SCREEN_ENABLED,
+	PRIVACY_SCREEN_DISABLED_LOCKED,
+	PRIVACY_SCREEN_ENABLED_LOCKED,
+};
+
 /*
  * This is a consolidated colorimetry list supported by HDMI and
  * DP protocol standard. The respective connectors will register
@@ -686,6 +710,18 @@ struct drm_connector_state {
 	 */
 	u8 max_bpc;
 
+	/**
+	 * @privacy_screen_sw_state: See :ref:`Standard Connector
+	 * Properties<standard_connector_properties>`
+	 */
+	enum drm_privacy_screen_status privacy_screen_sw_state;
+
+	/**
+	 * @privacy_screen_hw_state: See :ref:`Standard Connector
+	 * Properties<standard_connector_properties>`
+	 */
+	enum drm_privacy_screen_status privacy_screen_hw_state;
+
 	/**
 	 * @hdr_output_metadata:
 	 * DRM blob property for HDR output metadata
@@ -1285,6 +1321,18 @@ struct drm_connector {
 	 */
 	struct drm_property *max_bpc_property;
 
+	/**
+	 * @privacy_screen_sw_state_property: Optional atomic property for the
+	 * connector to control the integrated privacy screen.
+	 */
+	struct drm_property *privacy_screen_sw_state_property;
+
+	/**
+	 * @privacy_screen_hw_state_property: Optional atomic property for the
+	 * connector to report the actual integrated privacy screen state.
+	 */
+	struct drm_property *privacy_screen_hw_state_property;
+
 #define DRM_CONNECTOR_POLL_HPD (1 << 0)
 #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
 #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
@@ -1598,6 +1646,8 @@ int drm_connector_set_panel_orientation_with_quirk(
 	int width, int height);
 int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
 					  int min, int max);
+void drm_connector_create_privacy_screen_properties(struct drm_connector *conn);
+void drm_connector_attach_privacy_screen_properties(struct drm_connector *conn);
 
 /**
  * struct drm_tile_group - Tile group metadata
-- 
2.26.0

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Rajat Jain]

[-- Attachment #1.1: Type: text/plain, Size: 3034 bytes --]

Hi Hans,

On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com> wrote:

> Hi All,
>
> This RFC takes Rajat's earlier patch for adding privacy-screen properties
> infra to drm_connector.c and then adds the results of the discussion from
> the "RFC: Drm-connector properties managed by another driver / privacy
> screen support" mail thread on top, hence the v2.
>

Thank you so much for doing this. I was following the said discussion and
eventually it became quite complex for me to understand and follow :-)


>
> The most important thing here is big kernel-doc comment which gets added in
> the first patch-chunk modifying drm_connector.c, this summarizes, or at
> least tries to summarize, the conclusions of our previous discussion on
> the userspace API and lays down the ground rules for how the 2 new
> "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
> to be used both from the driver side as well as from the userspace side.
>
> Other then that this modifies Rajat's patch to add 2 properties instead
> of one, without much other changes.
>
> Rajat, perhaps you can do a new version of your patch-set integration /
> using this version of the properties and then if everyone is ok with
> the proposed userspace API Jani can hopefully merge the whole set
> through the i915 tree sometime during the 5.9 cycle.
>

SGTM. I have actually moved to working on something else now, so I will
most likely wait for this patch to get merged, before rebasing my other /
remaining patches on top of that.

Thanks & Best Regards,

Rajat


> This RFC takes Rajat's earlier patch for adding privacy-screen properties
> infra to drm_connector.c and then adds the results of the discussion from
> the "RFC: Drm-connector properties managed by another driver / privacy
> screen support" mail thread on top, hence the v2.
>
> The most important thing here is big kernel-doc comment which gets added in
> the first patch-chunk modifying drm_connector.c, this summarizes, or at
> least tries to summarize, the conclusions of our previous discussion on
> the userspace API and lays down the ground rules for how the 2 new
> "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
> to be used both from the driver side as well as from the userspace side.
>
> Other then that this modifies Rajat's patch to add 2 properties instead
> of one, without much other changes.
>
> Rajat, perhaps you can do a new version of your patch-set integration /
> using this version of the properties and then if everyone is ok with
> the proposed userspace API Jani can hopefully merge the whole set
> through the i915 tree sometime during the 5.9 cycle.
>
> Regards,
>
> Hans
>
> p.s.
>
> I plan to start working on the lcdshadow subsystem next. As discussed the
> plan for this subsystem is to allow drivers outside of the DRM subsys, such
> as for example the thinkpad_acpi driver, to register a lcdshadow device,
> which DRM drivers can then get a reference to and use to implement these
> properties.
>
>

[-- Attachment #1.2: Type: text/html, Size: 3919 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Rajat Jain]

On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> From: Rajat Jain <rajatja@google.com>
>
> Add support for generic electronic privacy screen properties, that
> can be added by systems that have an integrated EPS.
>
> Changes in v2 (Hans de Goede)
> - Create 2 properties, "privacy-screen sw-state" and
>   "privacy-screen hw-state", to deal with devices where the OS might be
>   locked out of making state changes
> - Write kerneldoc explaining how the 2 properties work together, what
>   happens when changes to the state are made outside of the DRM code's
>   control, etc.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Ack, Thanks for doing this.

>
> ---
>  Documentation/gpu/drm-kms.rst     |   2 +
>  drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
>  drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  50 +++++++++++++++
>  4 files changed, 158 insertions(+)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 906771e03103..b72b1e0db343 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -445,6 +445,8 @@ Property Types and Blob Property Support
>  .. kernel-doc:: drivers/gpu/drm/drm_property.c
>     :export:
>
> +.. _standard_connector_properties:
> +
>  Standard Connector Properties
>  -----------------------------
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index a1e5e262bae2..e56a11208515 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>                                                    fence_ptr);
>         } else if (property == connector->max_bpc_property) {
>                 state->max_requested_bpc = val;
> +       } else if (property == connector->privacy_screen_sw_state_property) {
> +               state->privacy_screen_sw_state = val;
>         } else if (connector->funcs->atomic_set_property) {
>                 return connector->funcs->atomic_set_property(connector,
>                                 state, property, val);
> @@ -842,6 +844,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>                 *val = 0;
>         } else if (property == connector->max_bpc_property) {
>                 *val = state->max_requested_bpc;
> +       } else if (property == connector->privacy_screen_sw_state_property) {
> +               *val = state->privacy_screen_sw_state;
> +       } else if (property == connector->privacy_screen_hw_state_property) {
> +               *val = state->privacy_screen_hw_state;
>         } else if (connector->funcs->atomic_get_property) {
>                 return connector->funcs->atomic_get_property(connector,
>                                 state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 644f0ad10671..01360edc2376 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *     can also expose this property to external outputs, in which case they
>   *     must support "None", which should be the default (since external screens
>   *     have a built-in scaler).
> + *
> + * privacy-screen sw-state, privacy-screen hw-state:
> + *     These 2 optional properties can be used to query the state of the
> + *     electronic privacy screen that is available on some displays; and in
> + *     some cases also control the state. If a driver implements these
> + *     properties then both properties must be present.
> + *
> + *     "privacy-screen hw-state" is read-only and reflects the actual state
> + *     of the privacy-screen, possible values: "Enabled", "Disabled,
> + *     "Enabled, locked", "Disabled, locked". The locked states indicate
> + *     that the state cannot be changed through the DRM API. E.g. there
> + *     might be devices where the firmware-setup options, or a hardware
> + *     slider-switch, offer always on / off modes.


May be add: "This is what the userspace tools must use to query and
report the actual status at the moment, if needed"

>
> + *
> + *     "privacy-screen sw-state" can be set to change the privacy-screen state
> + *     when not locked. In this case the driver must update the hw-state
> + *     property to reflect the new state on completion of the commit of the
> + *     sw-state property. Setting the sw-state property when the hw-state is
> + *     locked must be interpreted by the driver as a request to change the
> + *     state to the set state when the hw-state becomes unlocked. E.g. if
> + *     "privacy-screen hw-state" is "Enabled, locked" and the sw-state
> + *     gets set to "Disabled" followed by the user unlocking the state by
> + *     changing the slider-switch position, then the driver must set the
> + *     state to "Disabled" upon receiving the unlock event.
> + *
> + *     In some cases the privacy-screen state might change outside of control
> + *     of the DRM code. E.g. there might be a firmware handled hotkey which
> + *     toggles the state, or the state might be changed through another
> + *     userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
> + *     the driver must update both the hw-state and the sw-state to reflect
> + *     the new value, overwriting any pending state requests in the sw-state.


May be add: "The pending state requests are thus 'discarded'".

>
> + *
> + *     Note that the ability for the state to change outside of control of
> + *     the DRM master process means that userspace must not cache the value
> + *     of the sw-state. Ccaching the sw-state value and including it in later
> + *     atomic commits may lead to overriding a state change done through e.g.
> + *     a firmware handled hotkey. Therefor userspace must not include the
> + *     privacy-screen sw-state in an atomic commit unless it wants to change
> + *     its value.
>   */
>
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -2152,6 +2191,67 @@ int drm_connector_set_panel_orientation_with_quirk(
>  }
>  EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
>
> +static const struct drm_prop_enum_list privacy_screen_enum[] = {
> +       { PRIVACY_SCREEN_DISABLED,              "Disabled" },
> +       { PRIVACY_SCREEN_ENABLED,               "Enabled" },
> +       { PRIVACY_SCREEN_DISABLED_LOCKED,       "Disabled, locked" },
> +       { PRIVACY_SCREEN_ENABLED_LOCKED,        "Enabled, locked" },
> +};
> +
> +/**
> + * drm_connector_create_privacy_screen_properties -
> + *     create the drm connecter's privacy-screen properties.
> + * @connector: connector for which to create the privacy-screen properties
> + *
> + * This function creates the "privacy-screen sw-state" and "privacy-screen
> + * hw-state" properties for the connector. They are not attached.
> + */
> +void
> +drm_connector_create_privacy_screen_properties(struct drm_connector *connector)
> +{
> +       if (connector->privacy_screen_sw_state_property)
> +               return;
> +
> +       /* Note sw-state only supports the first 2 values of the enum */
> +       connector->privacy_screen_sw_state_property =
> +               drm_property_create_enum(connector->dev, DRM_MODE_PROP_ENUM,
> +                               "privacy-screen sw-state",
> +                               privacy_screen_enum, 2);
> +
> +       connector->privacy_screen_hw_state_property =
> +               drm_property_create_enum(connector->dev,
> +                               DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_ENUM,
> +                               "privacy-screen hw-state",
> +                               privacy_screen_enum,
> +                               ARRAY_SIZE(privacy_screen_enum));
> +}
> +EXPORT_SYMBOL(drm_connector_create_privacy_screen_properties);
> +
> +/**
> + * drm_connector_attach_privacy_screen_properties -
> + *     attach the drm connecter's privacy-screen properties.
> + * @connector: connector on which to attach the privacy-screen properties
> + *
> + * This function attaches the "privacy-screen sw-state" and "privacy-screen
> + * hw-state" properties to the connector. The initial state of both is set
> + * to "Disabled".
> + */
> +void
> +drm_connector_attach_privacy_screen_properties(struct drm_connector *connector)
> +{
> +       if (!connector->privacy_screen_sw_state_property)
> +               return;
> +
> +       drm_object_attach_property(&connector->base,
> +                                  connector->privacy_screen_sw_state_property,
> +                                  PRIVACY_SCREEN_DISABLED);
> +
> +       drm_object_attach_property(&connector->base,
> +                                  connector->privacy_screen_hw_state_property,
> +                                  PRIVACY_SCREEN_DISABLED);
> +}
> +EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
> +
>  int drm_connector_set_obj_prop(struct drm_mode_object *obj,
>                                     struct drm_property *property,
>                                     uint64_t value)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 19ae6bb5c85b..a8844f4c6ae9 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -271,6 +271,30 @@ struct drm_monitor_range_info {
>         u8 max_vfreq;
>  };
>
> +/**
> + * enum drm_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 sw-state"
> + * and "privacy-screen hw-state" properties. Note the _LOCKED enum values
> + * are only valid for the "privacy-screen hw-state" property.
> + *
> + * @PRIVACY_SCREEN_DISABLED:
> + *  The privacy-screen on the panel is disabled
> + * @PRIVACY_SCREEN_ENABLED:
> + *  The privacy-screen on the panel is enabled
> + * @PRIVACY_SCREEN_DISABLED_LOCKED:
> + *  The privacy-screen on the panel is disabled and locked (cannot be changed)
> + * @PRIVACY_SCREEN_ENABLED_LOCKED:
> + *  The privacy-screen on the panel is enabled and locked (cannot be changed)
> + */
> +enum drm_privacy_screen_status {
> +       PRIVACY_SCREEN_DISABLED = 0,
> +       PRIVACY_SCREEN_ENABLED,
> +       PRIVACY_SCREEN_DISABLED_LOCKED,
> +       PRIVACY_SCREEN_ENABLED_LOCKED,
> +};
> +
>  /*
>   * This is a consolidated colorimetry list supported by HDMI and
>   * DP protocol standard. The respective connectors will register
> @@ -686,6 +710,18 @@ struct drm_connector_state {
>          */
>         u8 max_bpc;
>
> +       /**
> +        * @privacy_screen_sw_state: See :ref:`Standard Connector
> +        * Properties<standard_connector_properties>`
> +        */
> +       enum drm_privacy_screen_status privacy_screen_sw_state;
> +
> +       /**
> +        * @privacy_screen_hw_state: See :ref:`Standard Connector
> +        * Properties<standard_connector_properties>`
> +        */
> +       enum drm_privacy_screen_status privacy_screen_hw_state;
> +
>         /**
>          * @hdr_output_metadata:
>          * DRM blob property for HDR output metadata
> @@ -1285,6 +1321,18 @@ struct drm_connector {
>          */
>         struct drm_property *max_bpc_property;
>
> +       /**
> +        * @privacy_screen_sw_state_property: Optional atomic property for the
> +        * connector to control the integrated privacy screen.
> +        */
> +       struct drm_property *privacy_screen_sw_state_property;
> +
> +       /**
> +        * @privacy_screen_hw_state_property: Optional atomic property for the
> +        * connector to report the actual integrated privacy screen state.
> +        */
> +       struct drm_property *privacy_screen_hw_state_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1598,6 +1646,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>         int width, int height);
>  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>                                           int min, int max);
> +void drm_connector_create_privacy_screen_properties(struct drm_connector *conn);
> +void drm_connector_attach_privacy_screen_properties(struct drm_connector *conn);
>
>  /**
>   * struct drm_tile_group - Tile group metadata
> --
> 2.26.0
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4657 bytes --]

On Mon, 11 May 2020 19:47:24 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> From: Rajat Jain <rajatja@google.com>
> 
> Add support for generic electronic privacy screen properties, that
> can be added by systems that have an integrated EPS.
> 
> Changes in v2 (Hans de Goede)
> - Create 2 properties, "privacy-screen sw-state" and
>   "privacy-screen hw-state", to deal with devices where the OS might be
>   locked out of making state changes
> - Write kerneldoc explaining how the 2 properties work together, what
>   happens when changes to the state are made outside of the DRM code's
>   control, etc.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  Documentation/gpu/drm-kms.rst     |   2 +
>  drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
>  drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  50 +++++++++++++++
>  4 files changed, 158 insertions(+)

...

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 644f0ad10671..01360edc2376 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *	can also expose this property to external outputs, in which case they
>   *	must support "None", which should be the default (since external screens
>   *	have a built-in scaler).
> + *
> + * privacy-screen sw-state, privacy-screen hw-state:
> + *	These 2 optional properties can be used to query the state of the
> + *	electronic privacy screen that is available on some displays; and in
> + *	some cases also control the state. If a driver implements these
> + *	properties then both properties must be present.
> + *
> + *	"privacy-screen hw-state" is read-only and reflects the actual state
> + *	of the privacy-screen, possible values: "Enabled", "Disabled,
> + *	"Enabled, locked", "Disabled, locked". The locked states indicate
> + *	that the state cannot be changed through the DRM API. E.g. there
> + *	might be devices where the firmware-setup options, or a hardware
> + *	slider-switch, offer always on / off modes.
> + *
> + *	"privacy-screen sw-state" can be set to change the privacy-screen state
> + *	when not locked. In this case the driver must update the hw-state
> + *	property to reflect the new state on completion of the commit of the
> + *	sw-state property. Setting the sw-state property when the hw-state is
> + *	locked must be interpreted by the driver as a request to change the
> + *	state to the set state when the hw-state becomes unlocked. E.g. if
> + *	"privacy-screen hw-state" is "Enabled, locked" and the sw-state
> + *	gets set to "Disabled" followed by the user unlocking the state by
> + *	changing the slider-switch position, then the driver must set the
> + *	state to "Disabled" upon receiving the unlock event.
> + *
> + *	In some cases the privacy-screen state might change outside of control
> + *	of the DRM code. E.g. there might be a firmware handled hotkey which
> + *	toggles the state, or the state might be changed through another

Hi,

in the above three lines, I'd use the term "hardware state" instead of
just "state" to be explicit. Or should it be "physical state" since
"hardware state" might be confused with "hw-state" property state?

I don't mind as long as it's unambiguous and distinguishes explicitly
between actual and the two property states.

> + *	userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
> + *	the driver must update both the hw-state and the sw-state to reflect
> + *	the new value, overwriting any pending state requests in the sw-state.
> + *
> + *	Note that the ability for the state to change outside of control of
> + *	the DRM master process means that userspace must not cache the value
> + *	of the sw-state. Ccaching the sw-state value and including it in later

Extra 'c' in "Caching".

> + *	atomic commits may lead to overriding a state change done through e.g.
> + *	a firmware handled hotkey. Therefor userspace must not include the
> + *	privacy-screen sw-state in an atomic commit unless it wants to change
> + *	its value.
>   */

This documentation and intended behaviour looks perfect to me. If you
can record an R-b just for the doc, please have:

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>

You can also downgrade that to Acked-by for the whole patch from my
behalf.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Hans de Goede]

Hi,

On 5/12/20 9:49 AM, Pekka Paalanen wrote:
> On Mon, 11 May 2020 19:47:24 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> From: Rajat Jain <rajatja@google.com>
>>
>> Add support for generic electronic privacy screen properties, that
>> can be added by systems that have an integrated EPS.
>>
>> Changes in v2 (Hans de Goede)
>> - Create 2 properties, "privacy-screen sw-state" and
>>    "privacy-screen hw-state", to deal with devices where the OS might be
>>    locked out of making state changes
>> - Write kerneldoc explaining how the 2 properties work together, what
>>    happens when changes to the state are made outside of the DRM code's
>>    control, etc.
>>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   Documentation/gpu/drm-kms.rst     |   2 +
>>   drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
>>   drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       |  50 +++++++++++++++
>>   4 files changed, 158 insertions(+)
> 
> ...
> 
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 644f0ad10671..01360edc2376 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>    *	can also expose this property to external outputs, in which case they
>>    *	must support "None", which should be the default (since external screens
>>    *	have a built-in scaler).
>> + *
>> + * privacy-screen sw-state, privacy-screen hw-state:
>> + *	These 2 optional properties can be used to query the state of the
>> + *	electronic privacy screen that is available on some displays; and in
>> + *	some cases also control the state. If a driver implements these
>> + *	properties then both properties must be present.
>> + *
>> + *	"privacy-screen hw-state" is read-only and reflects the actual state
>> + *	of the privacy-screen, possible values: "Enabled", "Disabled,
>> + *	"Enabled, locked", "Disabled, locked". The locked states indicate
>> + *	that the state cannot be changed through the DRM API. E.g. there
>> + *	might be devices where the firmware-setup options, or a hardware
>> + *	slider-switch, offer always on / off modes.
>> + *
>> + *	"privacy-screen sw-state" can be set to change the privacy-screen state
>> + *	when not locked. In this case the driver must update the hw-state
>> + *	property to reflect the new state on completion of the commit of the
>> + *	sw-state property. Setting the sw-state property when the hw-state is
>> + *	locked must be interpreted by the driver as a request to change the
>> + *	state to the set state when the hw-state becomes unlocked. E.g. if
>> + *	"privacy-screen hw-state" is "Enabled, locked" and the sw-state
>> + *	gets set to "Disabled" followed by the user unlocking the state by
>> + *	changing the slider-switch position, then the driver must set the
>> + *	state to "Disabled" upon receiving the unlock event.
>> + *
>> + *	In some cases the privacy-screen state might change outside of control
>> + *	of the DRM code. E.g. there might be a firmware handled hotkey which
>> + *	toggles the state, or the state might be changed through another
> 
> Hi,
> 
> in the above three lines, I'd use the term "hardware state" instead of
> just "state" to be explicit. Or should it be "physical state" since
> "hardware state" might be confused with "hw-state" property state?

Maybe "actual state"? That is what is used a few lines higher:

'"privacy-screen hw-state" is read-only and reflects the actual state'

And you use it yourself to describe what we want below:

> I don't mind as long as it's unambiguous and distinguishes explicitly
> between actual and the two property states.

So since you and I both naturally described it as "actual state" without
thinking too much what we where writing at the time (I guess that applies
to your use too), "actual state" seems a good fit ?


>> + *	userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
>> + *	the driver must update both the hw-state and the sw-state to reflect
>> + *	the new value, overwriting any pending state requests in the sw-state.
>> + *
>> + *	Note that the ability for the state to change outside of control of
>> + *	the DRM master process means that userspace must not cache the value
>> + *	of the sw-state. Ccaching the sw-state value and including it in later
> 
> Extra 'c' in "Caching".

Ack, will fix.

>> + *	atomic commits may lead to overriding a state change done through e.g.
>> + *	a firmware handled hotkey. Therefor userspace must not include the
>> + *	privacy-screen sw-state in an atomic commit unless it wants to change
>> + *	its value.
>>    */
> 
> This documentation and intended behaviour looks perfect to me.

Great I'm glad you like it.

> If you
> can record an R-b just for the doc, please have:
> 
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> 
> You can also downgrade that to Acked-by for the whole patch from my
> behalf.

I don't know of a way to add partial Reviewed-by tags, so unless
someone educates me on that I will change it to an Acked-by for the next
version.

Regards,

Hans

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

Re: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Hans de Goede]

Hi,

On 5/11/20 10:04 PM, Rajat Jain wrote:
> On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Rajat Jain <rajatja@google.com>
>>
>> Add support for generic electronic privacy screen properties, that
>> can be added by systems that have an integrated EPS.
>>
>> Changes in v2 (Hans de Goede)
>> - Create 2 properties, "privacy-screen sw-state" and
>>    "privacy-screen hw-state", to deal with devices where the OS might be
>>    locked out of making state changes
>> - Write kerneldoc explaining how the 2 properties work together, what
>>    happens when changes to the state are made outside of the DRM code's
>>    control, etc.
>>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> 
> Ack, Thanks for doing this.
> 
>>
>> ---
>>   Documentation/gpu/drm-kms.rst     |   2 +
>>   drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
>>   drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       |  50 +++++++++++++++
>>   4 files changed, 158 insertions(+)
>>
>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
>> index 906771e03103..b72b1e0db343 100644
>> --- a/Documentation/gpu/drm-kms.rst
>> +++ b/Documentation/gpu/drm-kms.rst
>> @@ -445,6 +445,8 @@ Property Types and Blob Property Support
>>   .. kernel-doc:: drivers/gpu/drm/drm_property.c
>>      :export:
>>
>> +.. _standard_connector_properties:
>> +
>>   Standard Connector Properties
>>   -----------------------------
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index a1e5e262bae2..e56a11208515 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>>                                                     fence_ptr);
>>          } else if (property == connector->max_bpc_property) {
>>                  state->max_requested_bpc = val;
>> +       } else if (property == connector->privacy_screen_sw_state_property) {
>> +               state->privacy_screen_sw_state = val;
>>          } else if (connector->funcs->atomic_set_property) {
>>                  return connector->funcs->atomic_set_property(connector,
>>                                  state, property, val);
>> @@ -842,6 +844,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>>                  *val = 0;
>>          } else if (property == connector->max_bpc_property) {
>>                  *val = state->max_requested_bpc;
>> +       } else if (property == connector->privacy_screen_sw_state_property) {
>> +               *val = state->privacy_screen_sw_state;
>> +       } else if (property == connector->privacy_screen_hw_state_property) {
>> +               *val = state->privacy_screen_hw_state;
>>          } else if (connector->funcs->atomic_get_property) {
>>                  return connector->funcs->atomic_get_property(connector,
>>                                  state, property, val);
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 644f0ad10671..01360edc2376 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>    *     can also expose this property to external outputs, in which case they
>>    *     must support "None", which should be the default (since external screens
>>    *     have a built-in scaler).
>> + *
>> + * privacy-screen sw-state, privacy-screen hw-state:
>> + *     These 2 optional properties can be used to query the state of the
>> + *     electronic privacy screen that is available on some displays; and in
>> + *     some cases also control the state. If a driver implements these
>> + *     properties then both properties must be present.
>> + *
>> + *     "privacy-screen hw-state" is read-only and reflects the actual state
>> + *     of the privacy-screen, possible values: "Enabled", "Disabled,
>> + *     "Enabled, locked", "Disabled, locked". The locked states indicate
>> + *     that the state cannot be changed through the DRM API. E.g. there
>> + *     might be devices where the firmware-setup options, or a hardware
>> + *     slider-switch, offer always on / off modes.
> 
> 
> May be add: "This is what the userspace tools must use to query and
> report the actual status at the moment, if needed"

Thank you, suggestions for improving the doc are always very welcome, so I
have tried adding this, both as is and with slightly changed wording. But it
always feels like it is just repeating earlier info. To me the
"reflect the actual state" from the beginning of the paragraph makes it
abundantly clear that this indeed is what userspace should use to get,
well, the actual state.

>> + *
>> + *     "privacy-screen sw-state" can be set to change the privacy-screen state
>> + *     when not locked. In this case the driver must update the hw-state
>> + *     property to reflect the new state on completion of the commit of the
>> + *     sw-state property. Setting the sw-state property when the hw-state is
>> + *     locked must be interpreted by the driver as a request to change the
>> + *     state to the set state when the hw-state becomes unlocked. E.g. if
>> + *     "privacy-screen hw-state" is "Enabled, locked" and the sw-state
>> + *     gets set to "Disabled" followed by the user unlocking the state by
>> + *     changing the slider-switch position, then the driver must set the
>> + *     state to "Disabled" upon receiving the unlock event.
>> + *
>> + *     In some cases the privacy-screen state might change outside of control
>> + *     of the DRM code. E.g. there might be a firmware handled hotkey which
>> + *     toggles the state, or the state might be changed through another
>> + *     userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
>> + *     the driver must update both the hw-state and the sw-state to reflect
>> + *     the new value, overwriting any pending state requests in the sw-state.
> 
> 
> May be add: "The pending state requests are thus 'discarded'".

Done for the next version (dropping the quotes around discarded).

Regards,

Hans


> 
>>
>> + *
>> + *     Note that the ability for the state to change outside of control of
>> + *     the DRM master process means that userspace must not cache the value
>> + *     of the sw-state. Ccaching the sw-state value and including it in later
>> + *     atomic commits may lead to overriding a state change done through e.g.
>> + *     a firmware handled hotkey. Therefor userspace must not include the
>> + *     privacy-screen sw-state in an atomic commit unless it wants to change
>> + *     its value.
>>    */
>>
>>   int drm_connector_create_standard_properties(struct drm_device *dev)
>> @@ -2152,6 +2191,67 @@ int drm_connector_set_panel_orientation_with_quirk(
>>   }
>>   EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
>>
>> +static const struct drm_prop_enum_list privacy_screen_enum[] = {
>> +       { PRIVACY_SCREEN_DISABLED,              "Disabled" },
>> +       { PRIVACY_SCREEN_ENABLED,               "Enabled" },
>> +       { PRIVACY_SCREEN_DISABLED_LOCKED,       "Disabled, locked" },
>> +       { PRIVACY_SCREEN_ENABLED_LOCKED,        "Enabled, locked" },
>> +};
>> +
>> +/**
>> + * drm_connector_create_privacy_screen_properties -
>> + *     create the drm connecter's privacy-screen properties.
>> + * @connector: connector for which to create the privacy-screen properties
>> + *
>> + * This function creates the "privacy-screen sw-state" and "privacy-screen
>> + * hw-state" properties for the connector. They are not attached.
>> + */
>> +void
>> +drm_connector_create_privacy_screen_properties(struct drm_connector *connector)
>> +{
>> +       if (connector->privacy_screen_sw_state_property)
>> +               return;
>> +
>> +       /* Note sw-state only supports the first 2 values of the enum */
>> +       connector->privacy_screen_sw_state_property =
>> +               drm_property_create_enum(connector->dev, DRM_MODE_PROP_ENUM,
>> +                               "privacy-screen sw-state",
>> +                               privacy_screen_enum, 2);
>> +
>> +       connector->privacy_screen_hw_state_property =
>> +               drm_property_create_enum(connector->dev,
>> +                               DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_ENUM,
>> +                               "privacy-screen hw-state",
>> +                               privacy_screen_enum,
>> +                               ARRAY_SIZE(privacy_screen_enum));
>> +}
>> +EXPORT_SYMBOL(drm_connector_create_privacy_screen_properties);
>> +
>> +/**
>> + * drm_connector_attach_privacy_screen_properties -
>> + *     attach the drm connecter's privacy-screen properties.
>> + * @connector: connector on which to attach the privacy-screen properties
>> + *
>> + * This function attaches the "privacy-screen sw-state" and "privacy-screen
>> + * hw-state" properties to the connector. The initial state of both is set
>> + * to "Disabled".
>> + */
>> +void
>> +drm_connector_attach_privacy_screen_properties(struct drm_connector *connector)
>> +{
>> +       if (!connector->privacy_screen_sw_state_property)
>> +               return;
>> +
>> +       drm_object_attach_property(&connector->base,
>> +                                  connector->privacy_screen_sw_state_property,
>> +                                  PRIVACY_SCREEN_DISABLED);
>> +
>> +       drm_object_attach_property(&connector->base,
>> +                                  connector->privacy_screen_hw_state_property,
>> +                                  PRIVACY_SCREEN_DISABLED);
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
>> +
>>   int drm_connector_set_obj_prop(struct drm_mode_object *obj,
>>                                      struct drm_property *property,
>>                                      uint64_t value)
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 19ae6bb5c85b..a8844f4c6ae9 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -271,6 +271,30 @@ struct drm_monitor_range_info {
>>          u8 max_vfreq;
>>   };
>>
>> +/**
>> + * enum drm_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 sw-state"
>> + * and "privacy-screen hw-state" properties. Note the _LOCKED enum values
>> + * are only valid for the "privacy-screen hw-state" property.
>> + *
>> + * @PRIVACY_SCREEN_DISABLED:
>> + *  The privacy-screen on the panel is disabled
>> + * @PRIVACY_SCREEN_ENABLED:
>> + *  The privacy-screen on the panel is enabled
>> + * @PRIVACY_SCREEN_DISABLED_LOCKED:
>> + *  The privacy-screen on the panel is disabled and locked (cannot be changed)
>> + * @PRIVACY_SCREEN_ENABLED_LOCKED:
>> + *  The privacy-screen on the panel is enabled and locked (cannot be changed)
>> + */
>> +enum drm_privacy_screen_status {
>> +       PRIVACY_SCREEN_DISABLED = 0,
>> +       PRIVACY_SCREEN_ENABLED,
>> +       PRIVACY_SCREEN_DISABLED_LOCKED,
>> +       PRIVACY_SCREEN_ENABLED_LOCKED,
>> +};
>> +
>>   /*
>>    * This is a consolidated colorimetry list supported by HDMI and
>>    * DP protocol standard. The respective connectors will register
>> @@ -686,6 +710,18 @@ struct drm_connector_state {
>>           */
>>          u8 max_bpc;
>>
>> +       /**
>> +        * @privacy_screen_sw_state: See :ref:`Standard Connector
>> +        * Properties<standard_connector_properties>`
>> +        */
>> +       enum drm_privacy_screen_status privacy_screen_sw_state;
>> +
>> +       /**
>> +        * @privacy_screen_hw_state: See :ref:`Standard Connector
>> +        * Properties<standard_connector_properties>`
>> +        */
>> +       enum drm_privacy_screen_status privacy_screen_hw_state;
>> +
>>          /**
>>           * @hdr_output_metadata:
>>           * DRM blob property for HDR output metadata
>> @@ -1285,6 +1321,18 @@ struct drm_connector {
>>           */
>>          struct drm_property *max_bpc_property;
>>
>> +       /**
>> +        * @privacy_screen_sw_state_property: Optional atomic property for the
>> +        * connector to control the integrated privacy screen.
>> +        */
>> +       struct drm_property *privacy_screen_sw_state_property;
>> +
>> +       /**
>> +        * @privacy_screen_hw_state_property: Optional atomic property for the
>> +        * connector to report the actual integrated privacy screen state.
>> +        */
>> +       struct drm_property *privacy_screen_hw_state_property;
>> +
>>   #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>>   #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>>   #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
>> @@ -1598,6 +1646,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>>          int width, int height);
>>   int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>>                                            int min, int max);
>> +void drm_connector_create_privacy_screen_properties(struct drm_connector *conn);
>> +void drm_connector_attach_privacy_screen_properties(struct drm_connector *conn);
>>
>>   /**
>>    * struct drm_tile_group - Tile group metadata
>> --
>> 2.26.0
>>
> 

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Hans de Goede]

Hi,

On 5/11/20 9:55 PM, Rajat Jain wrote:
> Hi Hans,
> 
> On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> 
>     Hi All,
> 
>     This RFC takes Rajat's earlier patch for adding privacy-screen properties
>     infra to drm_connector.c and then adds the results of the discussion from
>     the "RFC: Drm-connector properties managed by another driver / privacy
>     screen support" mail thread on top, hence the v2.
> 
> 
> Thank you so much for doing this. I was following the said discussion and eventually it became quite complex for me to understand and follow :-)

I hope the new doc text makes things clear again?


>     The most important thing here is big kernel-doc comment which gets added in
>     the first patch-chunk modifying drm_connector.c, this summarizes, or at
>     least tries to summarize, the conclusions of our previous discussion on
>     the userspace API and lays down the ground rules for how the 2 new
>     "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
>     to be used both from the driver side as well as from the userspace side.
> 
>     Other then that this modifies Rajat's patch to add 2 properties instead
>     of one, without much other changes.
> 
>     Rajat, perhaps you can do a new version of your patch-set integration /
>     using this version of the properties and then if everyone is ok with
>     the proposed userspace API Jani can hopefully merge the whole set
>     through the i915 tree sometime during the 5.9 cycle.
> 
> 
> SGTM. I have actually moved to working on something else now, so I will most likely wait for this patch to get merged, before rebasing my other / remaining patches on top of that.

We have the rule that code like this will not be merged until it has at least
one in kernel user. I plan to eventually use this too, but that is still
a while away as I first need to write a lcdshadow subsystem which the
thinkpad_acpi code can then use to register a lcdshadow device; and when
that all is in place, then I can hook it up on the drm code.

So since Jani said your patch-set was more or less ready I think it would
be best if you add my version of this to your patch-set and then post
a new version of your patch-set. But first let me do a v3 addressing
the remarks on doc text. Note I will wait a bit before sending out v3
to see if I get more feedback.

Regards,

Hans


> 
> Thanks & Best Regards,
> 
> Rajat
> 
>     This RFC takes Rajat's earlier patch for adding privacy-screen properties
>     infra to drm_connector.c and then adds the results of the discussion from
>     the "RFC: Drm-connector properties managed by another driver / privacy
>     screen support" mail thread on top, hence the v2.
> 
>     The most important thing here is big kernel-doc comment which gets added in
>     the first patch-chunk modifying drm_connector.c, this summarizes, or at
>     least tries to summarize, the conclusions of our previous discussion on
>     the userspace API and lays down the ground rules for how the 2 new
>     "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
>     to be used both from the driver side as well as from the userspace side.
> 
>     Other then that this modifies Rajat's patch to add 2 properties instead
>     of one, without much other changes.
> 
>     Rajat, perhaps you can do a new version of your patch-set integration /
>     using this version of the properties and then if everyone is ok with
>     the proposed userspace API Jani can hopefully merge the whole set
>     through the i915 tree sometime during the 5.9 cycle.
> 
>     Regards,
> 
>     Hans
> 
>     p.s.
> 
>     I plan to start working on the lcdshadow subsystem next. As discussed the
>     plan for this subsystem is to allow drivers outside of the DRM subsys, such
>     as for example the thinkpad_acpi driver, to register a lcdshadow device,
>     which DRM drivers can then get a reference to and use to implement these
>     properties.
> 

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

Re: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4463 bytes --]

On Tue, 12 May 2020 10:02:12 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 5/12/20 9:49 AM, Pekka Paalanen wrote:
> > On Mon, 11 May 2020 19:47:24 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> From: Rajat Jain <rajatja@google.com>
> >>
> >> Add support for generic electronic privacy screen properties, that
> >> can be added by systems that have an integrated EPS.
> >>
> >> Changes in v2 (Hans de Goede)
> >> - Create 2 properties, "privacy-screen sw-state" and
> >>    "privacy-screen hw-state", to deal with devices where the OS might be
> >>    locked out of making state changes
> >> - Write kerneldoc explaining how the 2 properties work together, what
> >>    happens when changes to the state are made outside of the DRM code's
> >>    control, etc.
> >>
> >> Signed-off-by: Rajat Jain <rajatja@google.com>
> >> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>   Documentation/gpu/drm-kms.rst     |   2 +
> >>   drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
> >>   drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
> >>   include/drm/drm_connector.h       |  50 +++++++++++++++
> >>   4 files changed, 158 insertions(+)  
> > 
> > ...
> >   
> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> >> index 644f0ad10671..01360edc2376 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
> >>    *	can also expose this property to external outputs, in which case they
> >>    *	must support "None", which should be the default (since external screens
> >>    *	have a built-in scaler).
> >> + *
> >> + * privacy-screen sw-state, privacy-screen hw-state:
> >> + *	These 2 optional properties can be used to query the state of the
> >> + *	electronic privacy screen that is available on some displays; and in
> >> + *	some cases also control the state. If a driver implements these
> >> + *	properties then both properties must be present.
> >> + *
> >> + *	"privacy-screen hw-state" is read-only and reflects the actual state
> >> + *	of the privacy-screen, possible values: "Enabled", "Disabled,
> >> + *	"Enabled, locked", "Disabled, locked". The locked states indicate
> >> + *	that the state cannot be changed through the DRM API. E.g. there
> >> + *	might be devices where the firmware-setup options, or a hardware
> >> + *	slider-switch, offer always on / off modes.
> >> + *
> >> + *	"privacy-screen sw-state" can be set to change the privacy-screen state
> >> + *	when not locked. In this case the driver must update the hw-state
> >> + *	property to reflect the new state on completion of the commit of the
> >> + *	sw-state property. Setting the sw-state property when the hw-state is
> >> + *	locked must be interpreted by the driver as a request to change the
> >> + *	state to the set state when the hw-state becomes unlocked. E.g. if
> >> + *	"privacy-screen hw-state" is "Enabled, locked" and the sw-state
> >> + *	gets set to "Disabled" followed by the user unlocking the state by
> >> + *	changing the slider-switch position, then the driver must set the
> >> + *	state to "Disabled" upon receiving the unlock event.
> >> + *
> >> + *	In some cases the privacy-screen state might change outside of control
> >> + *	of the DRM code. E.g. there might be a firmware handled hotkey which
> >> + *	toggles the state, or the state might be changed through another  
> > 
> > Hi,
> > 
> > in the above three lines, I'd use the term "hardware state" instead of
> > just "state" to be explicit. Or should it be "physical state" since
> > "hardware state" might be confused with "hw-state" property state?  
> 
> Maybe "actual state"? That is what is used a few lines higher:
> 
> '"privacy-screen hw-state" is read-only and reflects the actual state'
> 
> And you use it yourself to describe what we want below:
> 
> > I don't mind as long as it's unambiguous and distinguishes explicitly
> > between actual and the two property states.  
> 
> So since you and I both naturally described it as "actual state" without
> thinking too much what we where writing at the time (I guess that applies
> to your use too), "actual state" seems a good fit ?

Sure!


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2571 bytes --]

On Tue, 12 May 2020 10:18:31 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 5/11/20 9:55 PM, Rajat Jain wrote:
> > Hi Hans,
> > 
> > On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> > 
> >     Hi All,
> > 
> >     This RFC takes Rajat's earlier patch for adding privacy-screen properties
> >     infra to drm_connector.c and then adds the results of the discussion from
> >     the "RFC: Drm-connector properties managed by another driver / privacy
> >     screen support" mail thread on top, hence the v2.
> > 
> > 
> > Thank you so much for doing this. I was following the said discussion and eventually it became quite complex for me to understand and follow :-)  
> 
> I hope the new doc text makes things clear again?
> 
> 
> >     The most important thing here is big kernel-doc comment which gets added in
> >     the first patch-chunk modifying drm_connector.c, this summarizes, or at
> >     least tries to summarize, the conclusions of our previous discussion on
> >     the userspace API and lays down the ground rules for how the 2 new
> >     "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
> >     to be used both from the driver side as well as from the userspace side.
> > 
> >     Other then that this modifies Rajat's patch to add 2 properties instead
> >     of one, without much other changes.
> > 
> >     Rajat, perhaps you can do a new version of your patch-set integration /
> >     using this version of the properties and then if everyone is ok with
> >     the proposed userspace API Jani can hopefully merge the whole set
> >     through the i915 tree sometime during the 5.9 cycle.
> > 
> > 
> > SGTM. I have actually moved to working on something else now, so I will most likely wait for this patch to get merged, before rebasing my other / remaining patches on top of that.  
> 
> We have the rule that code like this will not be merged until it has at least
> one in kernel user. I plan to eventually use this too, but that is still
> a while away as I first need to write a lcdshadow subsystem which the
> thinkpad_acpi code can then use to register a lcdshadow device; and when
> that all is in place, then I can hook it up on the drm code.

Hi,

I believe this falls under "new UAPI" rules, because this is adding new
KMS properties. Hence an in-kernel user is not enough:

https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Hans de Goede]

Hi,

On 5/12/20 4:20 PM, Pekka Paalanen wrote:
> On Tue, 12 May 2020 10:18:31 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 5/11/20 9:55 PM, Rajat Jain wrote:
>>> Hi Hans,
>>>
>>> On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
>>>
>>>      Hi All,
>>>
>>>      This RFC takes Rajat's earlier patch for adding privacy-screen properties
>>>      infra to drm_connector.c and then adds the results of the discussion from
>>>      the "RFC: Drm-connector properties managed by another driver / privacy
>>>      screen support" mail thread on top, hence the v2.
>>>
>>>
>>> Thank you so much for doing this. I was following the said discussion and eventually it became quite complex for me to understand and follow :-)
>>
>> I hope the new doc text makes things clear again?
>>
>>
>>>      The most important thing here is big kernel-doc comment which gets added in
>>>      the first patch-chunk modifying drm_connector.c, this summarizes, or at
>>>      least tries to summarize, the conclusions of our previous discussion on
>>>      the userspace API and lays down the ground rules for how the 2 new
>>>      "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
>>>      to be used both from the driver side as well as from the userspace side.
>>>
>>>      Other then that this modifies Rajat's patch to add 2 properties instead
>>>      of one, without much other changes.
>>>
>>>      Rajat, perhaps you can do a new version of your patch-set integration /
>>>      using this version of the properties and then if everyone is ok with
>>>      the proposed userspace API Jani can hopefully merge the whole set
>>>      through the i915 tree sometime during the 5.9 cycle.
>>>
>>>
>>> SGTM. I have actually moved to working on something else now, so I will most likely wait for this patch to get merged, before rebasing my other / remaining patches on top of that.
>>
>> We have the rule that code like this will not be merged until it has at least
>> one in kernel user. I plan to eventually use this too, but that is still
>> a while away as I first need to write a lcdshadow subsystem which the
>> thinkpad_acpi code can then use to register a lcdshadow device; and when
>> that all is in place, then I can hook it up on the drm code.
> 
> Hi,
> 
> I believe this falls under "new UAPI" rules, because this is adding new
> KMS properties. Hence an in-kernel user is not enough:
> 
> https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements

Hmm, I believe that that mostly applies to new DRI (ioclt) interfaces for
submitting rendering commands to new GPUs and other complex new APIs and
not necessarily to introducing new properties.    Also note that all
properties are exported under X through Xrandr, at least reading them,
not sure about setting them.

Anyways I do plan to write some mutter code to test my lcdshadow subsys <->
DRM driver integration when that is all more then just vaporware. But I
would prefer for Rajat's series to land before that so that I can build
on top of it.

Regards,

Hans

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Rajat Jain]

On Tue, May 12, 2020 at 9:09 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 5/12/20 4:20 PM, Pekka Paalanen wrote:
> > On Tue, 12 May 2020 10:18:31 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >
> >> Hi,
> >>
> >> On 5/11/20 9:55 PM, Rajat Jain wrote:
> >>> Hi Hans,
> >>>
> >>> On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> >>>
> >>>      Hi All,
> >>>
> >>>      This RFC takes Rajat's earlier patch for adding privacy-screen properties
> >>>      infra to drm_connector.c and then adds the results of the discussion from
> >>>      the "RFC: Drm-connector properties managed by another driver / privacy
> >>>      screen support" mail thread on top, hence the v2.
> >>>
> >>>
> >>> Thank you so much for doing this. I was following the said discussion and eventually it became quite complex for me to understand and follow :-)
> >>
> >> I hope the new doc text makes things clear again?

Yes it does.

> >>
> >>
> >>>      The most important thing here is big kernel-doc comment which gets added in
> >>>      the first patch-chunk modifying drm_connector.c, this summarizes, or at
> >>>      least tries to summarize, the conclusions of our previous discussion on
> >>>      the userspace API and lays down the ground rules for how the 2 new
> >>>      "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
> >>>      to be used both from the driver side as well as from the userspace side.
> >>>
> >>>      Other then that this modifies Rajat's patch to add 2 properties instead
> >>>      of one, without much other changes.
> >>>
> >>>      Rajat, perhaps you can do a new version of your patch-set integration /
> >>>      using this version of the properties and then if everyone is ok with
> >>>      the proposed userspace API Jani can hopefully merge the whole set
> >>>      through the i915 tree sometime during the 5.9 cycle.
> >>>
> >>>
> >>> SGTM. I have actually moved to working on something else now, so I will most likely wait for this patch to get merged, before rebasing my other / remaining patches on top of that.
> >>
> >> We have the rule that code like this will not be merged until it has at least
> >> one in kernel user. I plan to eventually use this too, but that is still
> >> a while away as I first need to write a lcdshadow subsystem which the
> >> thinkpad_acpi code can then use to register a lcdshadow device; and when
> >> that all is in place, then I can hook it up on the drm code.

OK. In that case, sure, I will respin my patchset with this patch once
we have some more comments from Jani et al, and thus a newer iteration
of this patch.

> >
> > Hi,
> >
> > I believe this falls under "new UAPI" rules, because this is adding new
> > KMS properties. Hence an in-kernel user is not enough:
> >
> > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements

The chrome browser currently uses the API exposed by my (previous)
patchset to control privacy screen.
https://source.chromium.org/chromium/chromium/src/+/master:ui/ozone/platform/drm/common/drm_util.cc;l=180?q=%22privacy-screen%22%20-f:third_party%2Fkernel%2Fv&originalUrl=https:%2F%2Fcs.chromium.org%2F

I know this doesn't help directly, but just to say that there are
users waiting to use an API that we release. If these changes are
accepted, I expect to see the change in browser again, to match the
new API,  although that will be not until we decide to uprev our
kernel again.

Thanks,

Rajat

>
> Hmm, I believe that that mostly applies to new DRI (ioclt) interfaces for
> submitting rendering commands to new GPUs and other complex new APIs and
> not necessarily to introducing new properties.    Also note that all
> properties are exported under X through Xrandr, at least reading them,
> not sure about setting them.
>
> Anyways I do plan to write some mutter code to test my lcdshadow subsys <->
> DRM driver integration when that is all more then just vaporware. But I
> would prefer for Rajat's series to land before that so that I can build
> on top of it.
>
> Regards,
>
> Hans
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Mario.Limonciello]

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Monday, May 11, 2020 12:47 PM
> To: Maarten Lankhorst; Maxime Ripard; Thomas Zimmermann; Daniel Vetter; David
> Airlie; Rajat Jain; Jani Nikula
> Cc: Hans de Goede; Pekka Paalanen; Limonciello, Mario; Quintanilla, Sonny;
> Jared Dominguez; Mark Pearson; dri-devel@lists.freedesktop.org
> Subject: [RFC v2] drm/connector: Add support for privacy-screen properties
> (v2)
> 
> 
> [EXTERNAL EMAIL]
> 
> From: Rajat Jain <rajatja@google.com>
> 
> Add support for generic electronic privacy screen properties, that
> can be added by systems that have an integrated EPS.
> 
> Changes in v2 (Hans de Goede)
> - Create 2 properties, "privacy-screen sw-state" and
>   "privacy-screen hw-state", to deal with devices where the OS might be
>   locked out of making state changes
> - Write kerneldoc explaining how the 2 properties work together, what
>   happens when changes to the state are made outside of the DRM code's
>   control, etc.
> 
> Signed-off-by: Rajat Jain <rajatja@google.com>
> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  Documentation/gpu/drm-kms.rst     |   2 +
>  drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
>  drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h       |  50 +++++++++++++++
>  4 files changed, 158 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 906771e03103..b72b1e0db343 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -445,6 +445,8 @@ Property Types and Blob Property Support
>  .. kernel-doc:: drivers/gpu/drm/drm_property.c
>     :export:
> 
> +.. _standard_connector_properties:
> +
>  Standard Connector Properties
>  -----------------------------
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index a1e5e262bae2..e56a11208515 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
>  						   fence_ptr);
>  	} else if (property == connector->max_bpc_property) {
>  		state->max_requested_bpc = val;
> +	} else if (property == connector->privacy_screen_sw_state_property) {
> +		state->privacy_screen_sw_state = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -842,6 +844,10 @@ drm_atomic_connector_get_property(struct drm_connector
> *connector,
>  		*val = 0;
>  	} else if (property == connector->max_bpc_property) {
>  		*val = state->max_requested_bpc;
> +	} else if (property == connector->privacy_screen_sw_state_property) {
> +		*val = state->privacy_screen_sw_state;
> +	} else if (property == connector->privacy_screen_hw_state_property) {
> +		*val = state->privacy_screen_hw_state;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
>  				state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 644f0ad10671..01360edc2376 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[]
> = {
>   *	can also expose this property to external outputs, in which case they
>   *	must support "None", which should be the default (since external screens
>   *	have a built-in scaler).
> + *
> + * privacy-screen sw-state, privacy-screen hw-state:
> + *	These 2 optional properties can be used to query the state of the
> + *	electronic privacy screen that is available on some displays; and in
> + *	some cases also control the state. If a driver implements these
> + *	properties then both properties must be present.
> + *
> + *	"privacy-screen hw-state" is read-only and reflects the actual state
> + *	of the privacy-screen, possible values: "Enabled", "Disabled,
> + *	"Enabled, locked", "Disabled, locked". The locked states indicate
> + *	that the state cannot be changed through the DRM API. E.g. there
> + *	might be devices where the firmware-setup options, or a hardware
> + *	slider-switch, offer always on / off modes.
> + *
> + *	"privacy-screen sw-state" can be set to change the privacy-screen state
> + *	when not locked. In this case the driver must update the hw-state
> + *	property to reflect the new state on completion of the commit of the
> + *	sw-state property. Setting the sw-state property when the hw-state is
> + *	locked must be interpreted by the driver as a request to change the
> + *	state to the set state when the hw-state becomes unlocked. E.g. if
> + *	"privacy-screen hw-state" is "Enabled, locked" and the sw-state
> + *	gets set to "Disabled" followed by the user unlocking the state by
> + *	changing the slider-switch position, then the driver must set the
> + *	state to "Disabled" upon receiving the unlock event.
> + *
> + *	In some cases the privacy-screen state might change outside of control
> + *	of the DRM code. E.g. there might be a firmware handled hotkey which
> + *	toggles the state, or the state might be changed through another
> + *	userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
> + *	the driver must update both the hw-state and the sw-state to reflect
> + *	the new value, overwriting any pending state requests in the sw-state.
> + *
> + *	Note that the ability for the state to change outside of control of
> + *	the DRM master process means that userspace must not cache the value
> + *	of the sw-state. Ccaching the sw-state value and including it in later
> + *	atomic commits may lead to overriding a state change done through e.g.
> + *	a firmware handled hotkey. Therefor userspace must not include the
> + *	privacy-screen sw-state in an atomic commit unless it wants to change
> + *	its value.
>   */
> 
>  int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -2152,6 +2191,67 @@ int drm_connector_set_panel_orientation_with_quirk(
>  }
>  EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
> 
> +static const struct drm_prop_enum_list privacy_screen_enum[] = {
> +	{ PRIVACY_SCREEN_DISABLED,		"Disabled" },
> +	{ PRIVACY_SCREEN_ENABLED,		"Enabled" },
> +	{ PRIVACY_SCREEN_DISABLED_LOCKED,	"Disabled, locked" },
> +	{ PRIVACY_SCREEN_ENABLED_LOCKED,	"Enabled, locked" },
> +};
> +
> +/**
> + * drm_connector_create_privacy_screen_properties -
> + *     create the drm connecter's privacy-screen properties.
> + * @connector: connector for which to create the privacy-screen properties
> + *
> + * This function creates the "privacy-screen sw-state" and "privacy-screen
> + * hw-state" properties for the connector. They are not attached.
> + */
> +void
> +drm_connector_create_privacy_screen_properties(struct drm_connector
> *connector)
> +{
> +	if (connector->privacy_screen_sw_state_property)
> +		return;
> +
> +	/* Note sw-state only supports the first 2 values of the enum */
> +	connector->privacy_screen_sw_state_property =
> +		drm_property_create_enum(connector->dev, DRM_MODE_PROP_ENUM,
> +				"privacy-screen sw-state",
> +				privacy_screen_enum, 2);
> +
> +	connector->privacy_screen_hw_state_property =
> +		drm_property_create_enum(connector->dev,
> +				DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_ENUM,
> +				"privacy-screen hw-state",
> +				privacy_screen_enum,
> +				ARRAY_SIZE(privacy_screen_enum));
> +}
> +EXPORT_SYMBOL(drm_connector_create_privacy_screen_properties);
> +
> +/**
> + * drm_connector_attach_privacy_screen_properties -
> + *     attach the drm connecter's privacy-screen properties.
> + * @connector: connector on which to attach the privacy-screen properties
> + *
> + * This function attaches the "privacy-screen sw-state" and "privacy-screen
> + * hw-state" properties to the connector. The initial state of both is set
> + * to "Disabled".
> + */
> +void
> +drm_connector_attach_privacy_screen_properties(struct drm_connector
> *connector)
> +{
> +	if (!connector->privacy_screen_sw_state_property)
> +		return;
> +
> +	drm_object_attach_property(&connector->base,
> +				   connector->privacy_screen_sw_state_property,
> +				   PRIVACY_SCREEN_DISABLED);
> +
> +	drm_object_attach_property(&connector->base,
> +				   connector->privacy_screen_hw_state_property,
> +				   PRIVACY_SCREEN_DISABLED);
> +}
> +EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
> +
>  int drm_connector_set_obj_prop(struct drm_mode_object *obj,
>  				    struct drm_property *property,
>  				    uint64_t value)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 19ae6bb5c85b..a8844f4c6ae9 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -271,6 +271,30 @@ struct drm_monitor_range_info {
>  	u8 max_vfreq;
>  };
> 
> +/**
> + * enum drm_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 sw-state"
> + * and "privacy-screen hw-state" properties. Note the _LOCKED enum values
> + * are only valid for the "privacy-screen hw-state" property.
> + *
> + * @PRIVACY_SCREEN_DISABLED:
> + *  The privacy-screen on the panel is disabled
> + * @PRIVACY_SCREEN_ENABLED:
> + *  The privacy-screen on the panel is enabled
> + * @PRIVACY_SCREEN_DISABLED_LOCKED:
> + *  The privacy-screen on the panel is disabled and locked (cannot be
> changed)
> + * @PRIVACY_SCREEN_ENABLED_LOCKED:
> + *  The privacy-screen on the panel is enabled and locked (cannot be changed)
> + */
> +enum drm_privacy_screen_status {
> +	PRIVACY_SCREEN_DISABLED = 0,
> +	PRIVACY_SCREEN_ENABLED,
> +	PRIVACY_SCREEN_DISABLED_LOCKED,
> +	PRIVACY_SCREEN_ENABLED_LOCKED,
> +};
> +
>  /*
>   * This is a consolidated colorimetry list supported by HDMI and
>   * DP protocol standard. The respective connectors will register
> @@ -686,6 +710,18 @@ struct drm_connector_state {
>  	 */
>  	u8 max_bpc;
> 
> +	/**
> +	 * @privacy_screen_sw_state: See :ref:`Standard Connector
> +	 * Properties<standard_connector_properties>`
> +	 */
> +	enum drm_privacy_screen_status privacy_screen_sw_state;
> +
> +	/**
> +	 * @privacy_screen_hw_state: See :ref:`Standard Connector
> +	 * Properties<standard_connector_properties>`
> +	 */
> +	enum drm_privacy_screen_status privacy_screen_hw_state;
> +
>  	/**
>  	 * @hdr_output_metadata:
>  	 * DRM blob property for HDR output metadata
> @@ -1285,6 +1321,18 @@ struct drm_connector {
>  	 */
>  	struct drm_property *max_bpc_property;
> 
> +	/**
> +	 * @privacy_screen_sw_state_property: Optional atomic property for the
> +	 * connector to control the integrated privacy screen.
> +	 */
> +	struct drm_property *privacy_screen_sw_state_property;
> +
> +	/**
> +	 * @privacy_screen_hw_state_property: Optional atomic property for the
> +	 * connector to report the actual integrated privacy screen state.
> +	 */
> +	struct drm_property *privacy_screen_hw_state_property;
> +
>  #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>  #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>  #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
> @@ -1598,6 +1646,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>  	int width, int height);
>  int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>  					  int min, int max);
> +void drm_connector_create_privacy_screen_properties(struct drm_connector
> *conn);
> +void drm_connector_attach_privacy_screen_properties(struct drm_connector
> *conn);
> 
>  /**
>   * struct drm_tile_group - Tile group metadata
> --
> 2.26.0

Hans,

Thanks for putting together this set of modifications.  I believe it does sufficiently
reflect the implementation of privacy screens present on Dell notebooks today containing
them: Latitude 7300 and Latitude 7400.

Those models only offer a HW controlled screen via a hotkey, but that hotkey control
can be removed permanently locking them in an enabled or disabled state.

I feel that your concept of HW state "Enabled, Locked" and "Disabled, Locked" sufficiently
reflects that.

Reviewed-by: Mario Limonciello <Mario.limonciello@dell.com>

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

Re: [RFC v2] drm/connector: Add support for privacy-screen properties (v2)

[Hans de Goede]

Hi,

On 5/12/20 10:44 PM, Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Monday, May 11, 2020 12:47 PM
>> To: Maarten Lankhorst; Maxime Ripard; Thomas Zimmermann; Daniel Vetter; David
>> Airlie; Rajat Jain; Jani Nikula
>> Cc: Hans de Goede; Pekka Paalanen; Limonciello, Mario; Quintanilla, Sonny;
>> Jared Dominguez; Mark Pearson; dri-devel@lists.freedesktop.org
>> Subject: [RFC v2] drm/connector: Add support for privacy-screen properties
>> (v2)
>>
>>
>> [EXTERNAL EMAIL]
>>
>> From: Rajat Jain <rajatja@google.com>
>>
>> Add support for generic electronic privacy screen properties, that
>> can be added by systems that have an integrated EPS.
>>
>> Changes in v2 (Hans de Goede)
>> - Create 2 properties, "privacy-screen sw-state" and
>>    "privacy-screen hw-state", to deal with devices where the OS might be
>>    locked out of making state changes
>> - Write kerneldoc explaining how the 2 properties work together, what
>>    happens when changes to the state are made outside of the DRM code's
>>    control, etc.
>>
>> Signed-off-by: Rajat Jain <rajatja@google.com>
>> Co-authored-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   Documentation/gpu/drm-kms.rst     |   2 +
>>   drivers/gpu/drm/drm_atomic_uapi.c |   6 ++
>>   drivers/gpu/drm/drm_connector.c   | 100 ++++++++++++++++++++++++++++++
>>   include/drm/drm_connector.h       |  50 +++++++++++++++
>>   4 files changed, 158 insertions(+)
>>
>> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
>> index 906771e03103..b72b1e0db343 100644
>> --- a/Documentation/gpu/drm-kms.rst
>> +++ b/Documentation/gpu/drm-kms.rst
>> @@ -445,6 +445,8 @@ Property Types and Blob Property Support
>>   .. kernel-doc:: drivers/gpu/drm/drm_property.c
>>      :export:
>>
>> +.. _standard_connector_properties:
>> +
>>   Standard Connector Properties
>>   -----------------------------
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>> b/drivers/gpu/drm/drm_atomic_uapi.c
>> index a1e5e262bae2..e56a11208515 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -766,6 +766,8 @@ static int drm_atomic_connector_set_property(struct
>> drm_connector *connector,
>>   						   fence_ptr);
>>   	} else if (property == connector->max_bpc_property) {
>>   		state->max_requested_bpc = val;
>> +	} else if (property == connector->privacy_screen_sw_state_property) {
>> +		state->privacy_screen_sw_state = val;
>>   	} else if (connector->funcs->atomic_set_property) {
>>   		return connector->funcs->atomic_set_property(connector,
>>   				state, property, val);
>> @@ -842,6 +844,10 @@ drm_atomic_connector_get_property(struct drm_connector
>> *connector,
>>   		*val = 0;
>>   	} else if (property == connector->max_bpc_property) {
>>   		*val = state->max_requested_bpc;
>> +	} else if (property == connector->privacy_screen_sw_state_property) {
>> +		*val = state->privacy_screen_sw_state;
>> +	} else if (property == connector->privacy_screen_hw_state_property) {
>> +		*val = state->privacy_screen_hw_state;
>>   	} else if (connector->funcs->atomic_get_property) {
>>   		return connector->funcs->atomic_get_property(connector,
>>   				state, property, val);
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 644f0ad10671..01360edc2376 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -1186,6 +1186,45 @@ static const struct drm_prop_enum_list dp_colorspaces[]
>> = {
>>    *	can also expose this property to external outputs, in which case they
>>    *	must support "None", which should be the default (since external screens
>>    *	have a built-in scaler).
>> + *
>> + * privacy-screen sw-state, privacy-screen hw-state:
>> + *	These 2 optional properties can be used to query the state of the
>> + *	electronic privacy screen that is available on some displays; and in
>> + *	some cases also control the state. If a driver implements these
>> + *	properties then both properties must be present.
>> + *
>> + *	"privacy-screen hw-state" is read-only and reflects the actual state
>> + *	of the privacy-screen, possible values: "Enabled", "Disabled,
>> + *	"Enabled, locked", "Disabled, locked". The locked states indicate
>> + *	that the state cannot be changed through the DRM API. E.g. there
>> + *	might be devices where the firmware-setup options, or a hardware
>> + *	slider-switch, offer always on / off modes.
>> + *
>> + *	"privacy-screen sw-state" can be set to change the privacy-screen state
>> + *	when not locked. In this case the driver must update the hw-state
>> + *	property to reflect the new state on completion of the commit of the
>> + *	sw-state property. Setting the sw-state property when the hw-state is
>> + *	locked must be interpreted by the driver as a request to change the
>> + *	state to the set state when the hw-state becomes unlocked. E.g. if
>> + *	"privacy-screen hw-state" is "Enabled, locked" and the sw-state
>> + *	gets set to "Disabled" followed by the user unlocking the state by
>> + *	changing the slider-switch position, then the driver must set the
>> + *	state to "Disabled" upon receiving the unlock event.
>> + *
>> + *	In some cases the privacy-screen state might change outside of control
>> + *	of the DRM code. E.g. there might be a firmware handled hotkey which
>> + *	toggles the state, or the state might be changed through another
>> + *	userspace API such as writing /proc/acpi/ibm/lcdshadow. In this case
>> + *	the driver must update both the hw-state and the sw-state to reflect
>> + *	the new value, overwriting any pending state requests in the sw-state.
>> + *
>> + *	Note that the ability for the state to change outside of control of
>> + *	the DRM master process means that userspace must not cache the value
>> + *	of the sw-state. Ccaching the sw-state value and including it in later
>> + *	atomic commits may lead to overriding a state change done through e.g.
>> + *	a firmware handled hotkey. Therefor userspace must not include the
>> + *	privacy-screen sw-state in an atomic commit unless it wants to change
>> + *	its value.
>>    */
>>
>>   int drm_connector_create_standard_properties(struct drm_device *dev)
>> @@ -2152,6 +2191,67 @@ int drm_connector_set_panel_orientation_with_quirk(
>>   }
>>   EXPORT_SYMBOL(drm_connector_set_panel_orientation_with_quirk);
>>
>> +static const struct drm_prop_enum_list privacy_screen_enum[] = {
>> +	{ PRIVACY_SCREEN_DISABLED,		"Disabled" },
>> +	{ PRIVACY_SCREEN_ENABLED,		"Enabled" },
>> +	{ PRIVACY_SCREEN_DISABLED_LOCKED,	"Disabled, locked" },
>> +	{ PRIVACY_SCREEN_ENABLED_LOCKED,	"Enabled, locked" },
>> +};
>> +
>> +/**
>> + * drm_connector_create_privacy_screen_properties -
>> + *     create the drm connecter's privacy-screen properties.
>> + * @connector: connector for which to create the privacy-screen properties
>> + *
>> + * This function creates the "privacy-screen sw-state" and "privacy-screen
>> + * hw-state" properties for the connector. They are not attached.
>> + */
>> +void
>> +drm_connector_create_privacy_screen_properties(struct drm_connector
>> *connector)
>> +{
>> +	if (connector->privacy_screen_sw_state_property)
>> +		return;
>> +
>> +	/* Note sw-state only supports the first 2 values of the enum */
>> +	connector->privacy_screen_sw_state_property =
>> +		drm_property_create_enum(connector->dev, DRM_MODE_PROP_ENUM,
>> +				"privacy-screen sw-state",
>> +				privacy_screen_enum, 2);
>> +
>> +	connector->privacy_screen_hw_state_property =
>> +		drm_property_create_enum(connector->dev,
>> +				DRM_MODE_PROP_IMMUTABLE | DRM_MODE_PROP_ENUM,
>> +				"privacy-screen hw-state",
>> +				privacy_screen_enum,
>> +				ARRAY_SIZE(privacy_screen_enum));
>> +}
>> +EXPORT_SYMBOL(drm_connector_create_privacy_screen_properties);
>> +
>> +/**
>> + * drm_connector_attach_privacy_screen_properties -
>> + *     attach the drm connecter's privacy-screen properties.
>> + * @connector: connector on which to attach the privacy-screen properties
>> + *
>> + * This function attaches the "privacy-screen sw-state" and "privacy-screen
>> + * hw-state" properties to the connector. The initial state of both is set
>> + * to "Disabled".
>> + */
>> +void
>> +drm_connector_attach_privacy_screen_properties(struct drm_connector
>> *connector)
>> +{
>> +	if (!connector->privacy_screen_sw_state_property)
>> +		return;
>> +
>> +	drm_object_attach_property(&connector->base,
>> +				   connector->privacy_screen_sw_state_property,
>> +				   PRIVACY_SCREEN_DISABLED);
>> +
>> +	drm_object_attach_property(&connector->base,
>> +				   connector->privacy_screen_hw_state_property,
>> +				   PRIVACY_SCREEN_DISABLED);
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties);
>> +
>>   int drm_connector_set_obj_prop(struct drm_mode_object *obj,
>>   				    struct drm_property *property,
>>   				    uint64_t value)
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 19ae6bb5c85b..a8844f4c6ae9 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -271,6 +271,30 @@ struct drm_monitor_range_info {
>>   	u8 max_vfreq;
>>   };
>>
>> +/**
>> + * enum drm_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 sw-state"
>> + * and "privacy-screen hw-state" properties. Note the _LOCKED enum values
>> + * are only valid for the "privacy-screen hw-state" property.
>> + *
>> + * @PRIVACY_SCREEN_DISABLED:
>> + *  The privacy-screen on the panel is disabled
>> + * @PRIVACY_SCREEN_ENABLED:
>> + *  The privacy-screen on the panel is enabled
>> + * @PRIVACY_SCREEN_DISABLED_LOCKED:
>> + *  The privacy-screen on the panel is disabled and locked (cannot be
>> changed)
>> + * @PRIVACY_SCREEN_ENABLED_LOCKED:
>> + *  The privacy-screen on the panel is enabled and locked (cannot be changed)
>> + */
>> +enum drm_privacy_screen_status {
>> +	PRIVACY_SCREEN_DISABLED = 0,
>> +	PRIVACY_SCREEN_ENABLED,
>> +	PRIVACY_SCREEN_DISABLED_LOCKED,
>> +	PRIVACY_SCREEN_ENABLED_LOCKED,
>> +};
>> +
>>   /*
>>    * This is a consolidated colorimetry list supported by HDMI and
>>    * DP protocol standard. The respective connectors will register
>> @@ -686,6 +710,18 @@ struct drm_connector_state {
>>   	 */
>>   	u8 max_bpc;
>>
>> +	/**
>> +	 * @privacy_screen_sw_state: See :ref:`Standard Connector
>> +	 * Properties<standard_connector_properties>`
>> +	 */
>> +	enum drm_privacy_screen_status privacy_screen_sw_state;
>> +
>> +	/**
>> +	 * @privacy_screen_hw_state: See :ref:`Standard Connector
>> +	 * Properties<standard_connector_properties>`
>> +	 */
>> +	enum drm_privacy_screen_status privacy_screen_hw_state;
>> +
>>   	/**
>>   	 * @hdr_output_metadata:
>>   	 * DRM blob property for HDR output metadata
>> @@ -1285,6 +1321,18 @@ struct drm_connector {
>>   	 */
>>   	struct drm_property *max_bpc_property;
>>
>> +	/**
>> +	 * @privacy_screen_sw_state_property: Optional atomic property for the
>> +	 * connector to control the integrated privacy screen.
>> +	 */
>> +	struct drm_property *privacy_screen_sw_state_property;
>> +
>> +	/**
>> +	 * @privacy_screen_hw_state_property: Optional atomic property for the
>> +	 * connector to report the actual integrated privacy screen state.
>> +	 */
>> +	struct drm_property *privacy_screen_hw_state_property;
>> +
>>   #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>>   #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>>   #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
>> @@ -1598,6 +1646,8 @@ int drm_connector_set_panel_orientation_with_quirk(
>>   	int width, int height);
>>   int drm_connector_attach_max_bpc_property(struct drm_connector *connector,
>>   					  int min, int max);
>> +void drm_connector_create_privacy_screen_properties(struct drm_connector
>> *conn);
>> +void drm_connector_attach_privacy_screen_properties(struct drm_connector
>> *conn);
>>
>>   /**
>>    * struct drm_tile_group - Tile group metadata
>> --
>> 2.26.0
> 
> Hans,
> 
> Thanks for putting together this set of modifications.  I believe it does sufficiently
> reflect the implementation of privacy screens present on Dell notebooks today containing
> them: Latitude 7300 and Latitude 7400.
> 
> Those models only offer a HW controlled screen via a hotkey, but that hotkey control
> can be removed permanently locking them in an enabled or disabled state.
> 
> I feel that your concept of HW state "Enabled, Locked" and "Disabled, Locked" sufficiently
> reflects that.

That is good to hear.

> Reviewed-by: Mario Limonciello <Mario.limonciello@dell.com>

Thank you.

Regards,

Hans

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 5130 bytes --]

On Tue, 12 May 2020 18:09:15 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 5/12/20 4:20 PM, Pekka Paalanen wrote:
> > On Tue, 12 May 2020 10:18:31 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> Hi,
> >>
> >> On 5/11/20 9:55 PM, Rajat Jain wrote:  
> >>> Hi Hans,
> >>>
> >>> On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> >>>
> >>>      Hi All,
> >>>
> >>>      This RFC takes Rajat's earlier patch for adding privacy-screen properties
> >>>      infra to drm_connector.c and then adds the results of the discussion from
> >>>      the "RFC: Drm-connector properties managed by another driver / privacy
> >>>      screen support" mail thread on top, hence the v2.
> >>>
> >>>
> >>> Thank you so much for doing this. I was following the said discussion and eventually it became quite complex for me to understand and follow :-)  
> >>
> >> I hope the new doc text makes things clear again?
> >>
> >>  
> >>>      The most important thing here is big kernel-doc comment which gets added in
> >>>      the first patch-chunk modifying drm_connector.c, this summarizes, or at
> >>>      least tries to summarize, the conclusions of our previous discussion on
> >>>      the userspace API and lays down the ground rules for how the 2 new
> >>>      "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
> >>>      to be used both from the driver side as well as from the userspace side.
> >>>
> >>>      Other then that this modifies Rajat's patch to add 2 properties instead
> >>>      of one, without much other changes.
> >>>
> >>>      Rajat, perhaps you can do a new version of your patch-set integration /
> >>>      using this version of the properties and then if everyone is ok with
> >>>      the proposed userspace API Jani can hopefully merge the whole set
> >>>      through the i915 tree sometime during the 5.9 cycle.
> >>>
> >>>
> >>> SGTM. I have actually moved to working on something else now, so I will most likely wait for this patch to get merged, before rebasing my other / remaining patches on top of that.  
> >>
> >> We have the rule that code like this will not be merged until it has at least
> >> one in kernel user. I plan to eventually use this too, but that is still
> >> a while away as I first need to write a lcdshadow subsystem which the
> >> thinkpad_acpi code can then use to register a lcdshadow device; and when
> >> that all is in place, then I can hook it up on the drm code.  
> > 
> > Hi,
> > 
> > I believe this falls under "new UAPI" rules, because this is adding new
> > KMS properties. Hence an in-kernel user is not enough:
> > 
> > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements  
> 
> Hmm, I believe that that mostly applies to new DRI (ioclt) interfaces for
> submitting rendering commands to new GPUs and other complex new APIs and
> not necessarily to introducing new properties.    Also note that all
> properties are exported under X through Xrandr, at least reading them,
> not sure about setting them.

Please check with Daniel Vetter.

My belief is that all new KMS properties that were never exposed by any
driver before are new UAPI.

My personal opinion is that Xorg/RandR exposing a KMS property does
*not* count as real userspace *alone*. Simply plumbing a KMS property
through RandR and then nothing actually using it does not prove
anything about the property's design or usability.

IMO, if you use Xorg/RandR as your userspace, you also need something
that uses RandR and really pokes at the new property to prove it's
viable.

But that's just me.

> Anyways I do plan to write some mutter code to test my lcdshadow subsys <->
> DRM driver integration when that is all more then just vaporware. But I
> would prefer for Rajat's series to land before that so that I can build
> on top of it.

The DRM maintainers make that call.


On Tue, 12 May 2020 10:38:11 -0700
Rajat Jain <rajatja@google.com> wrote:

> The chrome browser currently uses the API exposed by my (previous)
> patchset to control privacy screen.
> https://source.chromium.org/chromium/chromium/src/+/master:ui/ozone/platform/drm/common/drm_util.cc;l=180?q=%22privacy-screen%22%20-f:third_party%2Fkernel%2Fv&originalUrl=https:%2F%2Fcs.chromium.org%2F
> 
> I know this doesn't help directly, but just to say that there are
> users waiting to use an API that we release. If these changes are
> accepted, I expect to see the change in browser again, to match the
> new API,  although that will be not until we decide to uprev our
> kernel again.

Chromium counts as userspace, I think many new features have landed
with it as the userspace.

Is that from some development branch, not actually merged or released
yet? If yes, very good. When you submit kernel patches with new UAPI,
it would be nice to point to the userspace review discussion where the
userspace patches have been reviewed and accepted but not merged.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Rajat Jain]

On Wed, May 13, 2020 at 12:49 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Tue, 12 May 2020 18:09:15 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
>
> > Hi,
> >
> > On 5/12/20 4:20 PM, Pekka Paalanen wrote:
> > > On Tue, 12 May 2020 10:18:31 +0200
> > > Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > >> Hi,
> > >>
> > >> On 5/11/20 9:55 PM, Rajat Jain wrote:
> > >>> Hi Hans,
> > >>>
> > >>> On Mon, May 11, 2020 at 10:47 AM Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
> > >>>
> > >>>      Hi All,
> > >>>
> > >>>      This RFC takes Rajat's earlier patch for adding privacy-screen properties
> > >>>      infra to drm_connector.c and then adds the results of the discussion from
> > >>>      the "RFC: Drm-connector properties managed by another driver / privacy
> > >>>      screen support" mail thread on top, hence the v2.
> > >>>
> > >>>
> > >>> Thank you so much for doing this. I was following the said discussion and eventually it became quite complex for me to understand and follow :-)
> > >>
> > >> I hope the new doc text makes things clear again?
> > >>
> > >>
> > >>>      The most important thing here is big kernel-doc comment which gets added in
> > >>>      the first patch-chunk modifying drm_connector.c, this summarizes, or at
> > >>>      least tries to summarize, the conclusions of our previous discussion on
> > >>>      the userspace API and lays down the ground rules for how the 2 new
> > >>>      "privacy-screen sw-state" and  "privacy-screen hw-state" properties are
> > >>>      to be used both from the driver side as well as from the userspace side.
> > >>>
> > >>>      Other then that this modifies Rajat's patch to add 2 properties instead
> > >>>      of one, without much other changes.
> > >>>
> > >>>      Rajat, perhaps you can do a new version of your patch-set integration /
> > >>>      using this version of the properties and then if everyone is ok with
> > >>>      the proposed userspace API Jani can hopefully merge the whole set
> > >>>      through the i915 tree sometime during the 5.9 cycle.
> > >>>
> > >>>
> > >>> SGTM. I have actually moved to working on something else now, so I will most likely wait for this patch to get merged, before rebasing my other / remaining patches on top of that.
> > >>
> > >> We have the rule that code like this will not be merged until it has at least
> > >> one in kernel user. I plan to eventually use this too, but that is still
> > >> a while away as I first need to write a lcdshadow subsystem which the
> > >> thinkpad_acpi code can then use to register a lcdshadow device; and when
> > >> that all is in place, then I can hook it up on the drm code.
> > >
> > > Hi,
> > >
> > > I believe this falls under "new UAPI" rules, because this is adding new
> > > KMS properties. Hence an in-kernel user is not enough:
> > >
> > > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements
> >
> > Hmm, I believe that that mostly applies to new DRI (ioclt) interfaces for
> > submitting rendering commands to new GPUs and other complex new APIs and
> > not necessarily to introducing new properties.    Also note that all
> > properties are exported under X through Xrandr, at least reading them,
> > not sure about setting them.
>
> Please check with Daniel Vetter.
>
> My belief is that all new KMS properties that were never exposed by any
> driver before are new UAPI.
>
> My personal opinion is that Xorg/RandR exposing a KMS property does
> *not* count as real userspace *alone*. Simply plumbing a KMS property
> through RandR and then nothing actually using it does not prove
> anything about the property's design or usability.
>
> IMO, if you use Xorg/RandR as your userspace, you also need something
> that uses RandR and really pokes at the new property to prove it's
> viable.
>
> But that's just me.
>
> > Anyways I do plan to write some mutter code to test my lcdshadow subsys <->
> > DRM driver integration when that is all more then just vaporware. But I
> > would prefer for Rajat's series to land before that so that I can build
> > on top of it.
>
> The DRM maintainers make that call.
>
>
> On Tue, 12 May 2020 10:38:11 -0700
> Rajat Jain <rajatja@google.com> wrote:
>
> > The chrome browser currently uses the API exposed by my (previous)
> > patchset to control privacy screen.
> > https://source.chromium.org/chromium/chromium/src/+/master:ui/ozone/platform/drm/common/drm_util.cc;l=180?q=%22privacy-screen%22%20-f:third_party%2Fkernel%2Fv&originalUrl=https:%2F%2Fcs.chromium.org%2F
> >
> > I know this doesn't help directly, but just to say that there are
> > users waiting to use an API that we release. If these changes are
> > accepted, I expect to see the change in browser again, to match the
> > new API,  although that will be not until we decide to uprev our
> > kernel again.
>
> Chromium counts as userspace, I think many new features have landed
> with it as the userspace.
>
> Is that from some development branch, not actually merged or released
> yet? If yes, very good.

No, it's released (in Chromium for chromeOS platforms).

> When you submit kernel patches with new UAPI,
> it would be nice to point to the userspace review discussion where the
> userspace patches have been reviewed and accepted but not merged.

I doubt if that would happen - because they won't do it unless a
feature is available in the kernel they are using. I can definitely
create a public bug about what they need to do though.

Thanks,

Rajat

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

Re: [RFC v2 0/1] drm/connector: Add support for privacy-screen properties

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 2330 bytes --]

On Wed, 13 May 2020 11:28:38 -0700
Rajat Jain <rajatja@google.com> wrote:

> On Wed, May 13, 2020 at 12:49 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:

...

> > On Tue, 12 May 2020 10:38:11 -0700
> > Rajat Jain <rajatja@google.com> wrote:
> >  
> > > The chrome browser currently uses the API exposed by my (previous)
> > > patchset to control privacy screen.
> > > https://source.chromium.org/chromium/chromium/src/+/master:ui/ozone/platform/drm/common/drm_util.cc;l=180?q=%22privacy-screen%22%20-f:third_party%2Fkernel%2Fv&originalUrl=https:%2F%2Fcs.chromium.org%2F
> > >
> > > I know this doesn't help directly, but just to say that there are
> > > users waiting to use an API that we release. If these changes are
> > > accepted, I expect to see the change in browser again, to match the
> > > new API,  although that will be not until we decide to uprev our
> > > kernel again.  
> >
> > Chromium counts as userspace, I think many new features have landed
> > with it as the userspace.
> >
> > Is that from some development branch, not actually merged or released
> > yet? If yes, very good.  
> 
> No, it's released (in Chromium for chromeOS platforms).

That is really, really bad.

You are lucky that the upstream discussions ended up changing all the
property names anyway, since now there is no way to ever use the
original names you used. Changing their meaning would have broken your
released userspace, and the kernel is simply not allowed to do that.

So that's a door closed for the kernel. Thankfully we didn't want to go
through that door in the end.

> > When you submit kernel patches with new UAPI,
> > it would be nice to point to the userspace review discussion where the
> > userspace patches have been reviewed and accepted but not merged.  
> 
> I doubt if that would happen - because they won't do it unless a
> feature is available in the kernel they are using. I can definitely
> create a public bug about what they need to do though.

Sorry, but that is the DRM development policy. Point your people to
https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements

I have the feeling that the doc in the link does not underline enough
"fully reviewed but NOT merged to something that will release".


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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