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

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