[RFC v2] drm/connector: Add support for privacy-screen properties (v2)
Hans de Goede
hdegoede at redhat.com
Tue May 12 21:30:15 UTC 2020
Hi,
On 5/12/20 10:44 PM, Mario.Limonciello at dell.com wrote:
>> -----Original Message-----
>> From: Hans de Goede <hdegoede at 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 at lists.freedesktop.org
>> Subject: [RFC v2] drm/connector: Add support for privacy-screen properties
>> (v2)
>>
>>
>> [EXTERNAL EMAIL]
>>
>> From: Rajat Jain <rajatja at 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 at google.com>
>> Co-authored-by: Hans de Goede <hdegoede at redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede at 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 at dell.com>
Thank you.
Regards,
Hans
More information about the dri-devel
mailing list