On Mon, 2021-09-06 at 09:35 +0200, Hans de Goede wrote:
> From: Rajat Jain <rajatja@xxxxxxxxxx>
>
> 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.
>
> Changes in v3 (Hans de Goede)
> - Some small tweaks to the kerneldoc describing the 2 properties
>
> Changes in v4 (Hans de Goede)
> - Change the "Enabled, locked" and "Disabled, locked" hw-state enum value
> names to "Enabled-locked" and "Disabled-locked". The xrandr command shows
> all possible enum values separated by commas in its output, so having a
> comma in an enum name is not a good idea.
> - Do not add a privacy_screen_hw_state member to drm_connector_state
> since this property is immutable its value must be directly stored in the
> obj->properties->values array
>
> Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
> Co-authored-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Acked-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> Reviewed-by: Mario Limonciello <Mario.limonciello@xxxxxxxx>
> Reviewed-by: Emil Velikov <emil.l.velikov@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> Documentation/gpu/drm-kms.rst | 2 +
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> drivers/gpu/drm/drm_connector.c | 101 ++++++++++++++++++++++++++++++
> include/drm/drm_connector.h | 44 +++++++++++++
> 4 files changed, 151 insertions(+)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 1ef7951ded5e..d14bf1c35d7e 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -506,6 +506,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 909f31833181..cdd31fc78bfc 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -797,6 +797,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);
> @@ -874,6 +876,8 @@ 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 (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 e0a30e0ee86a..dd1ca68881ba 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1264,6 +1264,46 @@ static const struct drm_prop_enum_list
> dp_colorspaces[] = {
> * For DVI-I and TVout there is also a matching property "select
> subconnector"
> * allowing to switch between signal types.
> * DP subconnector corresponds to a downstream port.
> + *
> + * 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's actual state might change outside
> of
> + * control of the DRM code. E.g. there might be a firmware handled
> hotkey
> + * which toggles the actual state, or the actual 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. Any pending sw-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. Caching 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)
> @@ -2341,6 +2381,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 79fa34e5ccdb..1acbcf0626ce 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -320,6 +320,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
> @@ -781,6 +805,12 @@ struct drm_connector_state {
> */
> u8 max_bpc;
>
> + /**
> + * @privacy_screen_sw_state: See :ref:`Standard Connector
> + * Properties<standard_connector_properties>`
> + */
So THAT'S how you reference other sections. I've always wondered!
> + enum drm_privacy_screen_status privacy_screen_sw_state;
> +
> /**
> * @hdr_output_metadata:
> * DRM blob property for HDR output metadata
> @@ -1409,6 +1439,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)
> @@ -1732,6 +1774,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
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
