[v3,3/9] drm: Add Content Protection property

Commit Message

Sean Paul Dec. 5, 2017, 5:15 a.m. UTC

This patch adds a new optional connector property to allow userspace to enable
protection over the content it is displaying. This will typically be implemented
by the driver using HDCP.

The property is a tri-state with the following values:
- OFF: Self explanatory, no content protection
- DESIRED: Userspace requests that the driver enable protection
- ENABLED: Once the driver has authenticated the link, it sets this value

The driver is responsible for downgrading ENABLED to DESIRED if the link becomes
unprotected. The driver should also maintain the desiredness of protection
across hotplug/dpms/suspend.

If this looks familiar, I posted [1] this 3 years ago. We have been using this
in ChromeOS across exynos, mediatek, and rockchip over that time.

Changes in v2:
 - Pimp kerneldoc for content_protection_property (Daniel)
 - Drop sysfs attribute
Changes in v3:
 - None

Cc: Daniel Vetter <[e-mail]>
Signed-off-by: Sean Paul <[e-mail]>

[1] https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html
---
 drivers/gpu/drm/drm_atomic.c    |  8 +++++
 drivers/gpu/drm/drm_connector.c | 71 +++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_sysfs.c     |  1 +
 include/drm/drm_connector.h     | 16 ++++++++++
 include/uapi/drm/drm_mode.h     |  4 +++
 5 files changed, 100 insertions(+)

Comments

Hans Verkuil Dec. 5, 2017, 8:07 a.m. UTC | #1

On 12/05/2017 06:15 AM, Sean Paul wrote:
> This patch adds a new optional connector property to allow userspace to enable
> protection over the content it is displaying. This will typically be implemented
> by the driver using HDCP.
> 
> The property is a tri-state with the following values:
> - OFF: Self explanatory, no content protection
> - DESIRED: Userspace requests that the driver enable protection
> - ENABLED: Once the driver has authenticated the link, it sets this value
> 
> The driver is responsible for downgrading ENABLED to DESIRED if the link becomes
> unprotected. The driver should also maintain the desiredness of protection
> across hotplug/dpms/suspend.
> 
> If this looks familiar, I posted [1] this 3 years ago. We have been using this
> in ChromeOS across exynos, mediatek, and rockchip over that time.
> 
> Changes in v2:
>  - Pimp kerneldoc for content_protection_property (Daniel)
>  - Drop sysfs attribute
> Changes in v3:
>  - None
> 
> Cc: Daniel Vetter <[e-mail]>
> Signed-off-by: Sean Paul <[e-mail]>
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html
> ---
>  drivers/gpu/drm/drm_atomic.c    |  8 +++++
>  drivers/gpu/drm/drm_connector.c | 71 +++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_sysfs.c     |  1 +
>  include/drm/drm_connector.h     | 16 ++++++++++
>  include/uapi/drm/drm_mode.h     |  4 +++
>  5 files changed, 100 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c2da5585e201..676025d755b2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1196,6 +1196,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->picture_aspect_ratio = val;
>  	} else if (property == connector->scaling_mode_property) {
>  		state->scaling_mode = val;
> +	} else if (property == connector->content_protection_property) {
> +		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> +			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> +			return -EINVAL;
> +		}
> +		state->content_protection = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->picture_aspect_ratio;
>  	} else if (property == connector->scaling_mode_property) {
>  		*val = state->scaling_mode;
> +	} else if (property == connector->content_protection_property) {
> +		*val = state->content_protection;
>  	} 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 f14b48e6e839..8626aa8f485e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
>  DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>  		 drm_tv_subconnector_enum_list)
>  
> +static struct drm_prop_enum_list drm_cp_enum_list[] = {
> +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
> +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
> +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
> +
>  /**
>   * DOC: standard connector properties
>   *
> @@ -764,6 +771,34 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   *      after modeset, the kernel driver may set this to "BAD" and issue a
>   *      hotplug uevent. Drivers should update this value using
>   *      drm_mode_connector_set_link_status_property().
> + * Content Protection:
> + *	This property is used by userspace to request the kernel protect future
> + *	content communicated over the link. When requested, kernel will apply
> + *	the appropriate means of protection (most often HDCP), and use the
> + *	property to tell userspace the protection is active.
> + *
> + *	The value of this property can be one of the following:
> + *
> + *	- DRM_MODE_CONTENT_PROTECTION_OFF = 0
> + *		The link is not protected, content is transmitted in the clear.
> + *	- DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
> + *		Userspace has requested content protection, but the link is not
> + *		currently protected. When in this state, kernel should enable
> + *		Content Protection as soon as possible.
> + *	- DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
> + *		Userspace has requested content protection, and the link is
> + *		protected. Only the driver can set the property to this value.
> + *		If userspace attempts to set to ENABLED, kernel will return
> + *		-EINVAL.
> + *
> + *	A few guidelines:
> + *
> + *	- DESIRED state should be preserved until userspace de-asserts it by
> + *	  setting the property to OFF. This means ENABLED should only transition
> + *	  to OFF when the user explicitly requests it.
> + *	- If the state is DESIRED, kernel should attempt to re-authenticate the
> + *	  link whenever possible. This includes across disable/enable, dpms,
> + *	  hotplug, downstream device changes, link status failures, etc..
>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -1047,6 +1082,42 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
>  
> +/**
> + * drm_connector_attach_content_protection_property - attach content protection
> + * property
> + *
> + * @connector: connector to attach CP property on.
> + *
> + * This is used to add support for content protection on select connectors.
> + * Content Protection is intentionally vague to allow for different underlying
> + * technologies, however it is most implemented by HDCP.
> + *
> + * The content protection will be set to &drm_connector_state.content_protection
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_content_protection_property(
> +		struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_enum(dev, 0, "Content Protection",
> +					drm_cp_enum_list,
> +					ARRAY_SIZE(drm_cp_enum_list));
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&connector->base, prop,
> +				   DRM_MODE_CONTENT_PROTECTION_OFF);
> +
> +	connector->content_protection_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> +
>  /**
>   * drm_mode_create_aspect_ratio_property - create aspect ratio property
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 1c5b5ce1fd7f..2385c7e0bef5 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -21,6 +21,7 @@
>  #include <drm/drm_sysfs.h>
>  #include <drm/drmP.h>
>  #include "drm_internal.h"
> +#include "drm_crtc_internal.h"
>  
>  #define to_drm_minor(d) dev_get_drvdata(d)
>  #define to_drm_connector(d) dev_get_drvdata(d)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7a7140543012..828878addd03 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -370,6 +370,12 @@ struct drm_connector_state {
>  	 * upscaling, mostly used for built-in panels.
>  	 */
>  	unsigned int scaling_mode;
> +
> +	/**
> +	 * @content_protection: Connector property to request content
> +	 * protection. This is most commonly used for HDCP.
> +	 */
> +	unsigned int content_protection;
>  };
>  
>  /**
> @@ -718,6 +724,7 @@ struct drm_cmdline_mode {
>   * @tile_h_size: horizontal size of this tile.
>   * @tile_v_size: vertical size of this tile.
>   * @scaling_mode_property:  Optional atomic property to control the upscaling.
> + * @content_protection_property: Optional property to control content protection
>   *
>   * Each connector may be connected to one or more CRTCs, or may be clonable by
>   * another connector if they can share a CRTC.  Each connector also has a specific
> @@ -808,6 +815,12 @@ struct drm_connector {
>  
>  	struct drm_property *scaling_mode_property;
>  
> +	/**
> +	 * @content_protection_property: DRM ENUM property for content
> +	 * protection
> +	 */
> +	struct drm_property *content_protection_property;
> +
>  	/**
>  	 * @path_blob_ptr:
>  	 *
> @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int val);
>  const char *drm_get_dvi_i_select_name(int val);
>  const char *drm_get_tv_subconnector_name(int val);
>  const char *drm_get_tv_select_name(int val);
> +const char *drm_get_content_protection_name(int val);
>  
>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>  int drm_mode_create_tv_properties(struct drm_device *dev,
> @@ -1010,6 +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
>  int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  					       u32 scaling_mode_mask);
> +int drm_connector_attach_content_protection_property(
> +		struct drm_connector *connector);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>  int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>  
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 5597a87154e5..03f4d22305c2 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -173,6 +173,10 @@ extern "C" {
>  		DRM_MODE_REFLECT_X | \
>  		DRM_MODE_REFLECT_Y)
>  
> +/* Content Protection Flags */
> +#define DRM_MODE_CONTENT_PROTECTION_OFF		0
> +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2

What about HDCP 1.4 and HDCP 2.2? Userspace would need to know which version
was negotiated since content protected 4k videos require HDCP 2.2. Perhaps
provide a property with the HDCP version?

I'm also missing a method for userspace to read the BKSV from the transmitter.

Regards,

	Hans

Ramalingam C Dec. 5, 2017, 2:04 p.m. UTC | #2

On Tuesday 05 December 2017 01:37 PM, Hans Verkuil wrote:
> On 12/05/2017 06:15 AM, Sean Paul wrote:
>> This patch adds a new optional connector property to allow userspace to enable
>> protection over the content it is displaying. This will typically be implemented
>> by the driver using HDCP.
>>
>> The property is a tri-state with the following values:
>> - OFF: Self explanatory, no content protection
>> - DESIRED: Userspace requests that the driver enable protection
>> - ENABLED: Once the driver has authenticated the link, it sets this value
>>
>> The driver is responsible for downgrading ENABLED to DESIRED if the link becomes
>> unprotected. The driver should also maintain the desiredness of protection
>> across hotplug/dpms/suspend.
>>
>> If this looks familiar, I posted [1] this 3 years ago. We have been using this
>> in ChromeOS across exynos, mediatek, and rockchip over that time.
>>
>> Changes in v2:
>>   - Pimp kerneldoc for content_protection_property (Daniel)
>>   - Drop sysfs attribute
>> Changes in v3:
>>   - None
>>
>> Cc: Daniel Vetter <[e-mail]>
>> Signed-off-by: Sean Paul <[e-mail]>
>>
>> [1] https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html
>> ---
>>   drivers/gpu/drm/drm_atomic.c    |  8 +++++
>>   drivers/gpu/drm/drm_connector.c | 71 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/drm_sysfs.c     |  1 +
>>   include/drm/drm_connector.h     | 16 ++++++++++
>>   include/uapi/drm/drm_mode.h     |  4 +++
>>   5 files changed, 100 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index c2da5585e201..676025d755b2 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1196,6 +1196,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>>   		state->picture_aspect_ratio = val;
>>   	} else if (property == connector->scaling_mode_property) {
>>   		state->scaling_mode = val;
>> +	} else if (property == connector->content_protection_property) {
>> +		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
>> +			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
>> +			return -EINVAL;
>> +		}
>> +		state->content_protection = val;
>>   	} else if (connector->funcs->atomic_set_property) {
>>   		return connector->funcs->atomic_set_property(connector,
>>   				state, property, val);
>> @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>>   		*val = state->picture_aspect_ratio;
>>   	} else if (property == connector->scaling_mode_property) {
>>   		*val = state->scaling_mode;
>> +	} else if (property == connector->content_protection_property) {
>> +		*val = state->content_protection;
>>   	} 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 f14b48e6e839..8626aa8f485e 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
>>   DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>>   		 drm_tv_subconnector_enum_list)
>>   
>> +static struct drm_prop_enum_list drm_cp_enum_list[] = {
>> +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
>> +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
>> +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },
>> +};
>> +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
>> +
>>   /**
>>    * DOC: standard connector properties
>>    *
>> @@ -764,6 +771,34 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>>    *      after modeset, the kernel driver may set this to "BAD" and issue a
>>    *      hotplug uevent. Drivers should update this value using
>>    *      drm_mode_connector_set_link_status_property().
>> + * Content Protection:
>> + *	This property is used by userspace to request the kernel protect future
>> + *	content communicated over the link. When requested, kernel will apply
>> + *	the appropriate means of protection (most often HDCP), and use the
>> + *	property to tell userspace the protection is active.
>> + *
>> + *	The value of this property can be one of the following:
>> + *
>> + *	- DRM_MODE_CONTENT_PROTECTION_OFF = 0
>> + *		The link is not protected, content is transmitted in the clear.
>> + *	- DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
>> + *		Userspace has requested content protection, but the link is not
>> + *		currently protected. When in this state, kernel should enable
>> + *		Content Protection as soon as possible.
>> + *	- DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
>> + *		Userspace has requested content protection, and the link is
>> + *		protected. Only the driver can set the property to this value.
>> + *		If userspace attempts to set to ENABLED, kernel will return
>> + *		-EINVAL.
>> + *
>> + *	A few guidelines:
>> + *
>> + *	- DESIRED state should be preserved until userspace de-asserts it by
>> + *	  setting the property to OFF. This means ENABLED should only transition
>> + *	  to OFF when the user explicitly requests it.
>> + *	- If the state is DESIRED, kernel should attempt to re-authenticate the
>> + *	  link whenever possible. This includes across disable/enable, dpms,
>> + *	  hotplug, downstream device changes, link status failures, etc..
>>    *
>>    * Connectors also have one standardized atomic property:
>>    *
>> @@ -1047,6 +1082,42 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>>   }
>>   EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
>>   
>> +/**
>> + * drm_connector_attach_content_protection_property - attach content protection
>> + * property
>> + *
>> + * @connector: connector to attach CP property on.
>> + *
>> + * This is used to add support for content protection on select connectors.
>> + * Content Protection is intentionally vague to allow for different underlying
>> + * technologies, however it is most implemented by HDCP.
>> + *
>> + * The content protection will be set to &drm_connector_state.content_protection
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int drm_connector_attach_content_protection_property(
>> +		struct drm_connector *connector)
>> +{
>> +	struct drm_device *dev = connector->dev;
>> +	struct drm_property *prop;
>> +
>> +	prop = drm_property_create_enum(dev, 0, "Content Protection",
>> +					drm_cp_enum_list,
>> +					ARRAY_SIZE(drm_cp_enum_list));
>> +	if (!prop)
>> +		return -ENOMEM;
>> +
>> +	drm_object_attach_property(&connector->base, prop,
>> +				   DRM_MODE_CONTENT_PROTECTION_OFF);
>> +
>> +	connector->content_protection_property = prop;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
>> +
>>   /**
>>    * drm_mode_create_aspect_ratio_property - create aspect ratio property
>>    * @dev: DRM device
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index 1c5b5ce1fd7f..2385c7e0bef5 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -21,6 +21,7 @@
>>   #include <drm/drm_sysfs.h>
>>   #include <drm/drmP.h>
>>   #include "drm_internal.h"
>> +#include "drm_crtc_internal.h"
>>   
>>   #define to_drm_minor(d) dev_get_drvdata(d)
>>   #define to_drm_connector(d) dev_get_drvdata(d)
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 7a7140543012..828878addd03 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -370,6 +370,12 @@ struct drm_connector_state {
>>   	 * upscaling, mostly used for built-in panels.
>>   	 */
>>   	unsigned int scaling_mode;
>> +
>> +	/**
>> +	 * @content_protection: Connector property to request content
>> +	 * protection. This is most commonly used for HDCP.
>> +	 */
>> +	unsigned int content_protection;
>>   };
>>   
>>   /**
>> @@ -718,6 +724,7 @@ struct drm_cmdline_mode {
>>    * @tile_h_size: horizontal size of this tile.
>>    * @tile_v_size: vertical size of this tile.
>>    * @scaling_mode_property:  Optional atomic property to control the upscaling.
>> + * @content_protection_property: Optional property to control content protection
>>    *
>>    * Each connector may be connected to one or more CRTCs, or may be clonable by
>>    * another connector if they can share a CRTC.  Each connector also has a specific
>> @@ -808,6 +815,12 @@ struct drm_connector {
>>   
>>   	struct drm_property *scaling_mode_property;
>>   
>> +	/**
>> +	 * @content_protection_property: DRM ENUM property for content
>> +	 * protection
>> +	 */
>> +	struct drm_property *content_protection_property;
>> +
>>   	/**
>>   	 * @path_blob_ptr:
>>   	 *
>> @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int val);
>>   const char *drm_get_dvi_i_select_name(int val);
>>   const char *drm_get_tv_subconnector_name(int val);
>>   const char *drm_get_tv_select_name(int val);
>> +const char *drm_get_content_protection_name(int val);
>>   
>>   int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>>   int drm_mode_create_tv_properties(struct drm_device *dev,
>> @@ -1010,6 +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
>>   int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>>   int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>>   					       u32 scaling_mode_mask);
>> +int drm_connector_attach_content_protection_property(
>> +		struct drm_connector *connector);
>>   int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>>   int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>>   
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 5597a87154e5..03f4d22305c2 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -173,6 +173,10 @@ extern "C" {
>>   		DRM_MODE_REFLECT_X | \
>>   		DRM_MODE_REFLECT_Y)
>>   
>> +/* Content Protection Flags */
>> +#define DRM_MODE_CONTENT_PROTECTION_OFF		0
>> +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>> +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
> What about HDCP 1.4 and HDCP 2.2? Userspace would need to know which version
> was negotiated since content protected 4k videos require HDCP 2.2. Perhaps
> provide a property with the HDCP version?
>
> I'm also missing a method for userspace to read the BKSV from the transmitter.
Hans,

I guess you are asking about the use case explained at 
http://www.spinics.net/lists/intel-gfx/msg134813.html

Sean,
As this solution is only for 1.4, this version requirement might not 
matter here.
But as a community could we at least ack such a need, so that inevitable 
extension of this uAPI will be well thought about?

As we discussed before Enum values required are

    -OFF           : Disable any content protection
    -DESIRED       : For any possible HDCP protection (v1.4/2.2)
    -DESIRED_TYPE1 : For HDCP2.2 only
    -ENABLED       : Highest HDCP protection is enabled (Could be v1.4/2.2)
    -ENABLED_TYPE1 : HDCP2.2 enabled

And another gap i am seeing with this uAPI is that there is no 
communication back about the
HDCP authentication failure, as DESIRED will remain without 
transitioning into ENABLED.
So userspace has to identify the failure of the HDCP req with polling 
and Timeout.
Timeout also will vary with system to system, as
with big downstream topology(worst case 127 devices with 7depth) 
authentication could take 5+ Sec.
     with only receiver < few 100 mSec(~200mSec?)


So could it help userspace if we could indicate the authentication failure.
Agreed that runtime link integrity lost is indicated by the 
ENABLED->DESIRED transition.

--Ram

>
> Regards,
>
> 	Hans
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Sean Paul Dec. 5, 2017, 2:36 p.m. UTC | #3

On Tue, Dec 5, 2017 at 9:04 AM, Ramalingam C <[e-mail]> wrote:
>
>
> On Tuesday 05 December 2017 01:37 PM, Hans Verkuil wrote:
>
> On 12/05/2017 06:15 AM, Sean Paul wrote:
>
> This patch adds a new optional connector property to allow userspace to
> enable
> protection over the content it is displaying. This will typically be
> implemented
> by the driver using HDCP.
>
> The property is a tri-state with the following values:
> - OFF: Self explanatory, no content protection
> - DESIRED: Userspace requests that the driver enable protection
> - ENABLED: Once the driver has authenticated the link, it sets this value
>
> The driver is responsible for downgrading ENABLED to DESIRED if the link
> becomes
> unprotected. The driver should also maintain the desiredness of protection
> across hotplug/dpms/suspend.
>
> If this looks familiar, I posted [1] this 3 years ago. We have been using
> this
> in ChromeOS across exynos, mediatek, and rockchip over that time.
>
> Changes in v2:
>  - Pimp kerneldoc for content_protection_property (Daniel)
>  - Drop sysfs attribute
> Changes in v3:
>  - None
>
> Cc: Daniel Vetter <[e-mail]>
> Signed-off-by: Sean Paul <[e-mail]>
>
> [1]
> https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html
> ---
>  drivers/gpu/drm/drm_atomic.c    |  8 +++++
>  drivers/gpu/drm/drm_connector.c | 71
> +++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_sysfs.c     |  1 +
>  include/drm/drm_connector.h     | 16 ++++++++++
>  include/uapi/drm/drm_mode.h     |  4 +++
>  5 files changed, 100 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c2da5585e201..676025d755b2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1196,6 +1196,12 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
>   state->picture_aspect_ratio = val;
>   } else if (property == connector->scaling_mode_property) {
>   state->scaling_mode = val;
> + } else if (property == connector->content_protection_property) {
> + if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> + DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> + return -EINVAL;
> + }
> + state->content_protection = val;
>   } else if (connector->funcs->atomic_set_property) {
>   return connector->funcs->atomic_set_property(connector,
>   state, property, val);
> @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct drm_connector
> *connector,
>   *val = state->picture_aspect_ratio;
>   } else if (property == connector->scaling_mode_property) {
>   *val = state->scaling_mode;
> + } else if (property == connector->content_protection_property) {
> + *val = state->content_protection;
>   } 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 f14b48e6e839..8626aa8f485e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list
> drm_tv_subconnector_enum_list[] = {
>  DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   drm_tv_subconnector_enum_list)
>
> +static struct drm_prop_enum_list drm_cp_enum_list[] = {
> +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
> +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
> +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
> +
>  /**
>   * DOC: standard connector properties
>   *
> @@ -764,6 +771,34 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   *      after modeset, the kernel driver may set this to "BAD" and issue a
>   *      hotplug uevent. Drivers should update this value using
>   *      drm_mode_connector_set_link_status_property().
> + * Content Protection:
> + * This property is used by userspace to request the kernel protect future
> + * content communicated over the link. When requested, kernel will apply
> + * the appropriate means of protection (most often HDCP), and use the
> + * property to tell userspace the protection is active.
> + *
> + * The value of this property can be one of the following:
> + *
> + * - DRM_MODE_CONTENT_PROTECTION_OFF = 0
> + * The link is not protected, content is transmitted in the clear.
> + * - DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
> + * Userspace has requested content protection, but the link is not
> + * currently protected. When in this state, kernel should enable
> + * Content Protection as soon as possible.
> + * - DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
> + * Userspace has requested content protection, and the link is
> + * protected. Only the driver can set the property to this value.
> + * If userspace attempts to set to ENABLED, kernel will return
> + * -EINVAL.
> + *
> + * A few guidelines:
> + *
> + * - DESIRED state should be preserved until userspace de-asserts it by
> + *  setting the property to OFF. This means ENABLED should only transition
> + *  to OFF when the user explicitly requests it.
> + * - If the state is DESIRED, kernel should attempt to re-authenticate the
> + *  link whenever possible. This includes across disable/enable, dpms,
> + *  hotplug, downstream device changes, link status failures, etc..
>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -1047,6 +1082,42 @@ int drm_connector_attach_scaling_mode_property(struct
> drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
>
> +/**
> + * drm_connector_attach_content_protection_property - attach content
> protection
> + * property
> + *
> + * @connector: connector to attach CP property on.
> + *
> + * This is used to add support for content protection on select connectors.
> + * Content Protection is intentionally vague to allow for different
> underlying
> + * technologies, however it is most implemented by HDCP.
> + *
> + * The content protection will be set to
> &drm_connector_state.content_protection
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_content_protection_property(
> + struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *prop;
> +
> + prop = drm_property_create_enum(dev, 0, "Content Protection",
> + drm_cp_enum_list,
> + ARRAY_SIZE(drm_cp_enum_list));
> + if (!prop)
> + return -ENOMEM;
> +
> + drm_object_attach_property(&connector->base, prop,
> +   DRM_MODE_CONTENT_PROTECTION_OFF);
> +
> + connector->content_protection_property = prop;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> +
>  /**
>   * drm_mode_create_aspect_ratio_property - create aspect ratio property
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 1c5b5ce1fd7f..2385c7e0bef5 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -21,6 +21,7 @@
>  #include <drm/drm_sysfs.h>
>  #include <drm/drmP.h>
>  #include "drm_internal.h"
> +#include "drm_crtc_internal.h"
>
>  #define to_drm_minor(d) dev_get_drvdata(d)
>  #define to_drm_connector(d) dev_get_drvdata(d)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7a7140543012..828878addd03 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -370,6 +370,12 @@ struct drm_connector_state {
>   * upscaling, mostly used for built-in panels.
>   */
>   unsigned int scaling_mode;
> +
> + /**
> + * @content_protection: Connector property to request content
> + * protection. This is most commonly used for HDCP.
> + */
> + unsigned int content_protection;
>  };
>
>  /**
> @@ -718,6 +724,7 @@ struct drm_cmdline_mode {
>   * @tile_h_size: horizontal size of this tile.
>   * @tile_v_size: vertical size of this tile.
>   * @scaling_mode_property:  Optional atomic property to control the
> upscaling.
> + * @content_protection_property: Optional property to control content
> protection
>   *
>   * Each connector may be connected to one or more CRTCs, or may be clonable
> by
>   * another connector if they can share a CRTC.  Each connector also has a
> specific
> @@ -808,6 +815,12 @@ struct drm_connector {
>
>   struct drm_property *scaling_mode_property;
>
> + /**
> + * @content_protection_property: DRM ENUM property for content
> + * protection
> + */
> + struct drm_property *content_protection_property;
> +
>   /**
>   * @path_blob_ptr:
>   *
> @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int val);
>  const char *drm_get_dvi_i_select_name(int val);
>  const char *drm_get_tv_subconnector_name(int val);
>  const char *drm_get_tv_select_name(int val);
> +const char *drm_get_content_protection_name(int val);
>
>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>  int drm_mode_create_tv_properties(struct drm_device *dev,
> @@ -1010,6 +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device
> *dev,
>  int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  int drm_connector_attach_scaling_mode_property(struct drm_connector
> *connector,
>         u32 scaling_mode_mask);
> +int drm_connector_attach_content_protection_property(
> + struct drm_connector *connector);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>  int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 5597a87154e5..03f4d22305c2 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -173,6 +173,10 @@ extern "C" {
>   DRM_MODE_REFLECT_X | \
>   DRM_MODE_REFLECT_Y)
>
> +/* Content Protection Flags */
> +#define DRM_MODE_CONTENT_PROTECTION_OFF 0
> +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>
> What about HDCP 1.4 and HDCP 2.2? Userspace would need to know which version
> was negotiated since content protected 4k videos require HDCP 2.2. Perhaps
> provide a property with the HDCP version?
>
> I'm also missing a method for userspace to read the BKSV from the
> transmitter.
>
> Hans,
>
> I guess you are asking about the use case explained at
> http://www.spinics.net/lists/intel-gfx/msg134813.html
>
> Sean,
> As this solution is only for 1.4, this version requirement might not matter
> here.
> But as a community could we at least ack such a need, so that inevitable
> extension of this uAPI will be well thought about?
>
> As we discussed before Enum values required are
>
> -OFF           : Disable any content protection
> -DESIRED       : For any possible HDCP protection (v1.4/2.2)
> -DESIRED_TYPE1 : For HDCP2.2 only
> -ENABLED       : Highest HDCP protection is enabled (Could be v1.4/2.2)
> -ENABLED_TYPE1 : HDCP2.2 enabled
>

I'd rather keep the property as-is and expose an HDCP version property
alongside it (or perhaps something more elaborate that includes bksv
and the downstream bksvs). The reason I prefer that is it will also
cover the 1.2 vs 1.4 difference that is a more immediate need.

This property is intentionally vague about the underlying encryption,
and tying it to HDCP (and specific versions, at that) is not
consistent with the design.


> And another gap i am seeing with this uAPI is that there is no communication
> back about the
> HDCP authentication failure, as DESIRED will remain without transitioning
> into ENABLED.
> So userspace has to identify the failure of the HDCP req with polling and
> Timeout.
> Timeout also will vary with system to system, as
>     with big downstream topology(worst case 127 devices with 7depth)
> authentication could take 5+ Sec.
>     with only receiver < few 100 mSec(~200mSec?)
>

It's worked on CrOS for a number of years now, why change what isn't broken?

Sean

>
> So could it help userspace if we could indicate the authentication failure.
> Agreed that runtime link integrity lost is indicated by the ENABLED->DESIRED
> transition.
>
> --Ram
>
>
> Regards,
>
> Hans
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Daniel Vetter Dec. 5, 2017, 3:27 p.m. UTC | #4

On Tue, Dec 05, 2017 at 09:07:58AM +0100, Hans Verkuil wrote:
> On 12/05/2017 06:15 AM, Sean Paul wrote:
> > This patch adds a new optional connector property to allow userspace to enable
> > protection over the content it is displaying. This will typically be implemented
> > by the driver using HDCP.
> > 
> > The property is a tri-state with the following values:
> > - OFF: Self explanatory, no content protection
> > - DESIRED: Userspace requests that the driver enable protection
> > - ENABLED: Once the driver has authenticated the link, it sets this value
> > 
> > The driver is responsible for downgrading ENABLED to DESIRED if the link becomes
> > unprotected. The driver should also maintain the desiredness of protection
> > across hotplug/dpms/suspend.
> > 
> > If this looks familiar, I posted [1] this 3 years ago. We have been using this
> > in ChromeOS across exynos, mediatek, and rockchip over that time.
> > 
> > Changes in v2:
> >  - Pimp kerneldoc for content_protection_property (Daniel)
> >  - Drop sysfs attribute
> > Changes in v3:
> >  - None
> > 
> > Cc: Daniel Vetter <[e-mail]>
> > Signed-off-by: Sean Paul <[e-mail]>
> > 
> > [1] https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html
> > ---
> >  drivers/gpu/drm/drm_atomic.c    |  8 +++++
> >  drivers/gpu/drm/drm_connector.c | 71 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/drm_sysfs.c     |  1 +
> >  include/drm/drm_connector.h     | 16 ++++++++++
> >  include/uapi/drm/drm_mode.h     |  4 +++
> >  5 files changed, 100 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index c2da5585e201..676025d755b2 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1196,6 +1196,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  		state->picture_aspect_ratio = val;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		state->scaling_mode = val;
> > +	} else if (property == connector->content_protection_property) {
> > +		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > +			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> > +			return -EINVAL;
> > +		}
> > +		state->content_protection = val;
> >  	} else if (connector->funcs->atomic_set_property) {
> >  		return connector->funcs->atomic_set_property(connector,
> >  				state, property, val);
> > @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >  		*val = state->picture_aspect_ratio;
> >  	} else if (property == connector->scaling_mode_property) {
> >  		*val = state->scaling_mode;
> > +	} else if (property == connector->content_protection_property) {
> > +		*val = state->content_protection;
> >  	} 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 f14b48e6e839..8626aa8f485e 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
> >  DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
> >  		 drm_tv_subconnector_enum_list)
> >  
> > +static struct drm_prop_enum_list drm_cp_enum_list[] = {
> > +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
> > +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
> > +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },
> > +};
> > +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
> > +
> >  /**
> >   * DOC: standard connector properties
> >   *
> > @@ -764,6 +771,34 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
> >   *      after modeset, the kernel driver may set this to "BAD" and issue a
> >   *      hotplug uevent. Drivers should update this value using
> >   *      drm_mode_connector_set_link_status_property().
> > + * Content Protection:
> > + *	This property is used by userspace to request the kernel protect future
> > + *	content communicated over the link. When requested, kernel will apply
> > + *	the appropriate means of protection (most often HDCP), and use the
> > + *	property to tell userspace the protection is active.
> > + *
> > + *	The value of this property can be one of the following:
> > + *
> > + *	- DRM_MODE_CONTENT_PROTECTION_OFF = 0
> > + *		The link is not protected, content is transmitted in the clear.
> > + *	- DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
> > + *		Userspace has requested content protection, but the link is not
> > + *		currently protected. When in this state, kernel should enable
> > + *		Content Protection as soon as possible.
> > + *	- DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
> > + *		Userspace has requested content protection, and the link is
> > + *		protected. Only the driver can set the property to this value.
> > + *		If userspace attempts to set to ENABLED, kernel will return
> > + *		-EINVAL.
> > + *
> > + *	A few guidelines:
> > + *
> > + *	- DESIRED state should be preserved until userspace de-asserts it by
> > + *	  setting the property to OFF. This means ENABLED should only transition
> > + *	  to OFF when the user explicitly requests it.
> > + *	- If the state is DESIRED, kernel should attempt to re-authenticate the
> > + *	  link whenever possible. This includes across disable/enable, dpms,
> > + *	  hotplug, downstream device changes, link status failures, etc..
> >   *
> >   * Connectors also have one standardized atomic property:
> >   *
> > @@ -1047,6 +1082,42 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
> >  
> > +/**
> > + * drm_connector_attach_content_protection_property - attach content protection
> > + * property
> > + *
> > + * @connector: connector to attach CP property on.
> > + *
> > + * This is used to add support for content protection on select connectors.
> > + * Content Protection is intentionally vague to allow for different underlying
> > + * technologies, however it is most implemented by HDCP.
> > + *
> > + * The content protection will be set to &drm_connector_state.content_protection
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_attach_content_protection_property(
> > +		struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_property *prop;
> > +
> > +	prop = drm_property_create_enum(dev, 0, "Content Protection",
> > +					drm_cp_enum_list,
> > +					ARRAY_SIZE(drm_cp_enum_list));
> > +	if (!prop)
> > +		return -ENOMEM;
> > +
> > +	drm_object_attach_property(&connector->base, prop,
> > +				   DRM_MODE_CONTENT_PROTECTION_OFF);
> > +
> > +	connector->content_protection_property = prop;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> > +
> >  /**
> >   * drm_mode_create_aspect_ratio_property - create aspect ratio property
> >   * @dev: DRM device
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index 1c5b5ce1fd7f..2385c7e0bef5 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -21,6 +21,7 @@
> >  #include <drm/drm_sysfs.h>
> >  #include <drm/drmP.h>
> >  #include "drm_internal.h"
> > +#include "drm_crtc_internal.h"
> >  
> >  #define to_drm_minor(d) dev_get_drvdata(d)
> >  #define to_drm_connector(d) dev_get_drvdata(d)
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 7a7140543012..828878addd03 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -370,6 +370,12 @@ struct drm_connector_state {
> >  	 * upscaling, mostly used for built-in panels.
> >  	 */
> >  	unsigned int scaling_mode;
> > +
> > +	/**
> > +	 * @content_protection: Connector property to request content
> > +	 * protection. This is most commonly used for HDCP.
> > +	 */
> > +	unsigned int content_protection;
> >  };
> >  
> >  /**
> > @@ -718,6 +724,7 @@ struct drm_cmdline_mode {
> >   * @tile_h_size: horizontal size of this tile.
> >   * @tile_v_size: vertical size of this tile.
> >   * @scaling_mode_property:  Optional atomic property to control the upscaling.
> > + * @content_protection_property: Optional property to control content protection
> >   *
> >   * Each connector may be connected to one or more CRTCs, or may be clonable by
> >   * another connector if they can share a CRTC.  Each connector also has a specific
> > @@ -808,6 +815,12 @@ struct drm_connector {
> >  
> >  	struct drm_property *scaling_mode_property;
> >  
> > +	/**
> > +	 * @content_protection_property: DRM ENUM property for content
> > +	 * protection
> > +	 */
> > +	struct drm_property *content_protection_property;
> > +
> >  	/**
> >  	 * @path_blob_ptr:
> >  	 *
> > @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int val);
> >  const char *drm_get_dvi_i_select_name(int val);
> >  const char *drm_get_tv_subconnector_name(int val);
> >  const char *drm_get_tv_select_name(int val);
> > +const char *drm_get_content_protection_name(int val);
> >  
> >  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
> >  int drm_mode_create_tv_properties(struct drm_device *dev,
> > @@ -1010,6 +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
> >  int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> >  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
> >  					       u32 scaling_mode_mask);
> > +int drm_connector_attach_content_protection_property(
> > +		struct drm_connector *connector);
> >  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> >  int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
> >  
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 5597a87154e5..03f4d22305c2 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -173,6 +173,10 @@ extern "C" {
> >  		DRM_MODE_REFLECT_X | \
> >  		DRM_MODE_REFLECT_Y)
> >  
> > +/* Content Protection Flags */
> > +#define DRM_MODE_CONTENT_PROTECTION_OFF		0
> > +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> > +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
> 
> What about HDCP 1.4 and HDCP 2.2? Userspace would need to know which version
> was negotiated since content protected 4k videos require HDCP 2.2. Perhaps
> provide a property with the HDCP version?
> 
> I'm also missing a method for userspace to read the BKSV from the transmitter.

Atm the only open source userspace we have is chrome os, which doesn't
care about either. That's why these two pieces have been left out, which
just need open source userspace for this stuff.

The rough idea for hdcp2 was to add a desired_type2 and enabled_type2,
which will guarantee hdcp2. _desired alone could give you either. Or we
try the most we can automatically, and publish the type1 vs. type2 stuff
in a 2nd property.

Either way, not having that from the start doesn't prevent us from adding
it later on in a backwards compatible way. Properties are rather nice this
way :-)
-Daniel

Daniel Vetter Dec. 5, 2017, 3:34 p.m. UTC | #5

On Tue, Dec 05, 2017 at 12:15:02AM -0500, Sean Paul wrote:
> This patch adds a new optional connector property to allow userspace to enable
> protection over the content it is displaying. This will typically be implemented
> by the driver using HDCP.
> 
> The property is a tri-state with the following values:
> - OFF: Self explanatory, no content protection
> - DESIRED: Userspace requests that the driver enable protection
> - ENABLED: Once the driver has authenticated the link, it sets this value
> 
> The driver is responsible for downgrading ENABLED to DESIRED if the link becomes
> unprotected. The driver should also maintain the desiredness of protection
> across hotplug/dpms/suspend.
> 
> If this looks familiar, I posted [1] this 3 years ago. We have been using this
> in ChromeOS across exynos, mediatek, and rockchip over that time.
> 
> Changes in v2:
>  - Pimp kerneldoc for content_protection_property (Daniel)
>  - Drop sysfs attribute
> Changes in v3:
>  - None
> 
> Cc: Daniel Vetter <[e-mail]>
> Signed-off-by: Sean Paul <[e-mail]>
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html
> ---
>  drivers/gpu/drm/drm_atomic.c    |  8 +++++
>  drivers/gpu/drm/drm_connector.c | 71 +++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_sysfs.c     |  1 +
>  include/drm/drm_connector.h     | 16 ++++++++++
>  include/uapi/drm/drm_mode.h     |  4 +++
>  5 files changed, 100 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c2da5585e201..676025d755b2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1196,6 +1196,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->picture_aspect_ratio = val;
>  	} else if (property == connector->scaling_mode_property) {
>  		state->scaling_mode = val;
> +	} else if (property == connector->content_protection_property) {
> +		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> +			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> +			return -EINVAL;
> +		}
> +		state->content_protection = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
>  				state, property, val);
> @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->picture_aspect_ratio;
>  	} else if (property == connector->scaling_mode_property) {
>  		*val = state->scaling_mode;
> +	} else if (property == connector->content_protection_property) {
> +		*val = state->content_protection;
>  	} 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 f14b48e6e839..8626aa8f485e 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
>  DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>  		 drm_tv_subconnector_enum_list)
>  
> +static struct drm_prop_enum_list drm_cp_enum_list[] = {
> +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
> +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
> +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
> +
>  /**
>   * DOC: standard connector properties
>   *
> @@ -764,6 +771,34 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   *      after modeset, the kernel driver may set this to "BAD" and issue a
>   *      hotplug uevent. Drivers should update this value using
>   *      drm_mode_connector_set_link_status_property().
> + * Content Protection:
> + *	This property is used by userspace to request the kernel protect future
> + *	content communicated over the link. When requested, kernel will apply
> + *	the appropriate means of protection (most often HDCP), and use the
> + *	property to tell userspace the protection is active.
> + *
> + *	The value of this property can be one of the following:
> + *
> + *	- DRM_MODE_CONTENT_PROTECTION_OFF = 0
> + *		The link is not protected, content is transmitted in the clear.
> + *	- DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
> + *		Userspace has requested content protection, but the link is not
> + *		currently protected. When in this state, kernel should enable
> + *		Content Protection as soon as possible.
> + *	- DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
> + *		Userspace has requested content protection, and the link is
> + *		protected. Only the driver can set the property to this value.
> + *		If userspace attempts to set to ENABLED, kernel will return
> + *		-EINVAL.
> + *
> + *	A few guidelines:
> + *
> + *	- DESIRED state should be preserved until userspace de-asserts it by
> + *	  setting the property to OFF. This means ENABLED should only transition
> + *	  to OFF when the user explicitly requests it.
> + *	- If the state is DESIRED, kernel should attempt to re-authenticate the
> + *	  link whenever possible. This includes across disable/enable, dpms,
> + *	  hotplug, downstream device changes, link status failures, etc..

Two bits missing imo:
- Should explain that userspace should poll this property to detect a
  change from ENABLED to DESIRED (and take adequate actions and e.g. stop
  the stream). No uevent will be sent out because the HDCP specs require
  polling anyway.
- One sentence to explain that drivers set this up using
  drm_connector_attach_content_protection_property().

Also, pls double-check the html output looks decent, you have quite some
fancy formatting in the above section :-)

With that all addressed:

Reviewed-by: Daniel Vetter <[e-mail]>


>   *
>   * Connectors also have one standardized atomic property:
>   *
> @@ -1047,6 +1082,42 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
>  
> +/**
> + * drm_connector_attach_content_protection_property - attach content protection
> + * property
> + *
> + * @connector: connector to attach CP property on.
> + *
> + * This is used to add support for content protection on select connectors.
> + * Content Protection is intentionally vague to allow for different underlying
> + * technologies, however it is most implemented by HDCP.
> + *
> + * The content protection will be set to &drm_connector_state.content_protection
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_content_protection_property(
> +		struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_enum(dev, 0, "Content Protection",
> +					drm_cp_enum_list,
> +					ARRAY_SIZE(drm_cp_enum_list));
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&connector->base, prop,
> +				   DRM_MODE_CONTENT_PROTECTION_OFF);
> +
> +	connector->content_protection_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> +
>  /**
>   * drm_mode_create_aspect_ratio_property - create aspect ratio property
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 1c5b5ce1fd7f..2385c7e0bef5 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -21,6 +21,7 @@
>  #include <drm/drm_sysfs.h>
>  #include <drm/drmP.h>
>  #include "drm_internal.h"
> +#include "drm_crtc_internal.h"
>  
>  #define to_drm_minor(d) dev_get_drvdata(d)
>  #define to_drm_connector(d) dev_get_drvdata(d)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 7a7140543012..828878addd03 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -370,6 +370,12 @@ struct drm_connector_state {
>  	 * upscaling, mostly used for built-in panels.
>  	 */
>  	unsigned int scaling_mode;
> +
> +	/**
> +	 * @content_protection: Connector property to request content
> +	 * protection. This is most commonly used for HDCP.
> +	 */
> +	unsigned int content_protection;
>  };
>  
>  /**
> @@ -718,6 +724,7 @@ struct drm_cmdline_mode {
>   * @tile_h_size: horizontal size of this tile.
>   * @tile_v_size: vertical size of this tile.
>   * @scaling_mode_property:  Optional atomic property to control the upscaling.
> + * @content_protection_property: Optional property to control content protection
>   *
>   * Each connector may be connected to one or more CRTCs, or may be clonable by
>   * another connector if they can share a CRTC.  Each connector also has a specific
> @@ -808,6 +815,12 @@ struct drm_connector {
>  
>  	struct drm_property *scaling_mode_property;
>  
> +	/**
> +	 * @content_protection_property: DRM ENUM property for content
> +	 * protection
> +	 */
> +	struct drm_property *content_protection_property;
> +
>  	/**
>  	 * @path_blob_ptr:
>  	 *
> @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int val);
>  const char *drm_get_dvi_i_select_name(int val);
>  const char *drm_get_tv_subconnector_name(int val);
>  const char *drm_get_tv_select_name(int val);
> +const char *drm_get_content_protection_name(int val);
>  
>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>  int drm_mode_create_tv_properties(struct drm_device *dev,
> @@ -1010,6 +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
>  int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  					       u32 scaling_mode_mask);
> +int drm_connector_attach_content_protection_property(
> +		struct drm_connector *connector);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>  int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>  
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 5597a87154e5..03f4d22305c2 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -173,6 +173,10 @@ extern "C" {
>  		DRM_MODE_REFLECT_X | \
>  		DRM_MODE_REFLECT_Y)
>  
> +/* Content Protection Flags */
> +#define DRM_MODE_CONTENT_PROTECTION_OFF		0
> +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
> +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>  
>  struct drm_mode_modeinfo {
>  	__u32 clock;
> -- 
> 2.15.0.531.g2ccb3012c9-goog
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Chris Wilson Dec. 5, 2017, 3:40 p.m. UTC | #6

Quoting Daniel Vetter (2017-12-05 15:34:36)
> Two bits missing imo:
> - Should explain that userspace should poll this property to detect a
>   change from ENABLED to DESIRED (and take adequate actions and e.g. stop
>   the stream). No uevent will be sent out because the HDCP specs require
>   polling anyway.

What's more polling than poll()? :-p
-Chris

Hans Verkuil Dec. 5, 2017, 4:10 p.m. UTC | #7

On 12/05/17 15:36, Sean Paul wrote:
> On Tue, Dec 5, 2017 at 9:04 AM, Ramalingam C <[e-mail]> wrote:
>>
>>
>> On Tuesday 05 December 2017 01:37 PM, Hans Verkuil wrote:
>>
>> On 12/05/2017 06:15 AM, Sean Paul wrote:
>>
>> This patch adds a new optional connector property to allow userspace to
>> enable
>> protection over the content it is displaying. This will typically be
>> implemented
>> by the driver using HDCP.
>>
>> The property is a tri-state with the following values:
>> - OFF: Self explanatory, no content protection
>> - DESIRED: Userspace requests that the driver enable protection
>> - ENABLED: Once the driver has authenticated the link, it sets this value
>>
>> The driver is responsible for downgrading ENABLED to DESIRED if the link
>> becomes
>> unprotected. The driver should also maintain the desiredness of protection
>> across hotplug/dpms/suspend.
>>
>> If this looks familiar, I posted [1] this 3 years ago. We have been using
>> this
>> in ChromeOS across exynos, mediatek, and rockchip over that time.
>>
>> Changes in v2:
>>  - Pimp kerneldoc for content_protection_property (Daniel)
>>  - Drop sysfs attribute
>> Changes in v3:
>>  - None
>>
>> Cc: Daniel Vetter <[e-mail]>
>> Signed-off-by: Sean Paul <[e-mail]>
>>
>> [1]
>> https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.html
>> ---
>>  drivers/gpu/drm/drm_atomic.c    |  8 +++++
>>  drivers/gpu/drm/drm_connector.c | 71
>> +++++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_sysfs.c     |  1 +
>>  include/drm/drm_connector.h     | 16 ++++++++++
>>  include/uapi/drm/drm_mode.h     |  4 +++
>>  5 files changed, 100 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index c2da5585e201..676025d755b2 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -1196,6 +1196,12 @@ static int drm_atomic_connector_set_property(struct
>> drm_connector *connector,
>>   state->picture_aspect_ratio = val;
>>   } else if (property == connector->scaling_mode_property) {
>>   state->scaling_mode = val;
>> + } else if (property == connector->content_protection_property) {
>> + if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
>> + DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
>> + return -EINVAL;
>> + }
>> + state->content_protection = val;
>>   } else if (connector->funcs->atomic_set_property) {
>>   return connector->funcs->atomic_set_property(connector,
>>   state, property, val);
>> @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct drm_connector
>> *connector,
>>   *val = state->picture_aspect_ratio;
>>   } else if (property == connector->scaling_mode_property) {
>>   *val = state->scaling_mode;
>> + } else if (property == connector->content_protection_property) {
>> + *val = state->content_protection;
>>   } 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 f14b48e6e839..8626aa8f485e 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list
>> drm_tv_subconnector_enum_list[] = {
>>  DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>>   drm_tv_subconnector_enum_list)
>>
>> +static struct drm_prop_enum_list drm_cp_enum_list[] = {
>> +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
>> +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
>> +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },
>> +};
>> +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
>> +
>>  /**
>>   * DOC: standard connector properties
>>   *
>> @@ -764,6 +771,34 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>>   *      after modeset, the kernel driver may set this to "BAD" and issue a
>>   *      hotplug uevent. Drivers should update this value using
>>   *      drm_mode_connector_set_link_status_property().
>> + * Content Protection:
>> + * This property is used by userspace to request the kernel protect future
>> + * content communicated over the link. When requested, kernel will apply
>> + * the appropriate means of protection (most often HDCP), and use the
>> + * property to tell userspace the protection is active.
>> + *
>> + * The value of this property can be one of the following:
>> + *
>> + * - DRM_MODE_CONTENT_PROTECTION_OFF = 0
>> + * The link is not protected, content is transmitted in the clear.
>> + * - DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
>> + * Userspace has requested content protection, but the link is not
>> + * currently protected. When in this state, kernel should enable
>> + * Content Protection as soon as possible.
>> + * - DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
>> + * Userspace has requested content protection, and the link is
>> + * protected. Only the driver can set the property to this value.
>> + * If userspace attempts to set to ENABLED, kernel will return
>> + * -EINVAL.
>> + *
>> + * A few guidelines:
>> + *
>> + * - DESIRED state should be preserved until userspace de-asserts it by
>> + *  setting the property to OFF. This means ENABLED should only transition
>> + *  to OFF when the user explicitly requests it.
>> + * - If the state is DESIRED, kernel should attempt to re-authenticate the
>> + *  link whenever possible. This includes across disable/enable, dpms,
>> + *  hotplug, downstream device changes, link status failures, etc..
>>   *
>>   * Connectors also have one standardized atomic property:
>>   *
>> @@ -1047,6 +1082,42 @@ int drm_connector_attach_scaling_mode_property(struct
>> drm_connector *connector,
>>  }
>>  EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
>>
>> +/**
>> + * drm_connector_attach_content_protection_property - attach content
>> protection
>> + * property
>> + *
>> + * @connector: connector to attach CP property on.
>> + *
>> + * This is used to add support for content protection on select connectors.
>> + * Content Protection is intentionally vague to allow for different
>> underlying
>> + * technologies, however it is most implemented by HDCP.
>> + *
>> + * The content protection will be set to
>> &drm_connector_state.content_protection
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int drm_connector_attach_content_protection_property(
>> + struct drm_connector *connector)
>> +{
>> + struct drm_device *dev = connector->dev;
>> + struct drm_property *prop;
>> +
>> + prop = drm_property_create_enum(dev, 0, "Content Protection",
>> + drm_cp_enum_list,
>> + ARRAY_SIZE(drm_cp_enum_list));
>> + if (!prop)
>> + return -ENOMEM;
>> +
>> + drm_object_attach_property(&connector->base, prop,
>> +   DRM_MODE_CONTENT_PROTECTION_OFF);
>> +
>> + connector->content_protection_property = prop;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
>> +
>>  /**
>>   * drm_mode_create_aspect_ratio_property - create aspect ratio property
>>   * @dev: DRM device
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index 1c5b5ce1fd7f..2385c7e0bef5 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -21,6 +21,7 @@
>>  #include <drm/drm_sysfs.h>
>>  #include <drm/drmP.h>
>>  #include "drm_internal.h"
>> +#include "drm_crtc_internal.h"
>>
>>  #define to_drm_minor(d) dev_get_drvdata(d)
>>  #define to_drm_connector(d) dev_get_drvdata(d)
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 7a7140543012..828878addd03 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -370,6 +370,12 @@ struct drm_connector_state {
>>   * upscaling, mostly used for built-in panels.
>>   */
>>   unsigned int scaling_mode;
>> +
>> + /**
>> + * @content_protection: Connector property to request content
>> + * protection. This is most commonly used for HDCP.
>> + */
>> + unsigned int content_protection;
>>  };
>>
>>  /**
>> @@ -718,6 +724,7 @@ struct drm_cmdline_mode {
>>   * @tile_h_size: horizontal size of this tile.
>>   * @tile_v_size: vertical size of this tile.
>>   * @scaling_mode_property:  Optional atomic property to control the
>> upscaling.
>> + * @content_protection_property: Optional property to control content
>> protection
>>   *
>>   * Each connector may be connected to one or more CRTCs, or may be clonable
>> by
>>   * another connector if they can share a CRTC.  Each connector also has a
>> specific
>> @@ -808,6 +815,12 @@ struct drm_connector {
>>
>>   struct drm_property *scaling_mode_property;
>>
>> + /**
>> + * @content_protection_property: DRM ENUM property for content
>> + * protection
>> + */
>> + struct drm_property *content_protection_property;
>> +
>>   /**
>>   * @path_blob_ptr:
>>   *
>> @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int val);
>>  const char *drm_get_dvi_i_select_name(int val);
>>  const char *drm_get_tv_subconnector_name(int val);
>>  const char *drm_get_tv_select_name(int val);
>> +const char *drm_get_content_protection_name(int val);
>>
>>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>>  int drm_mode_create_tv_properties(struct drm_device *dev,
>> @@ -1010,6 +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device
>> *dev,
>>  int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>>  int drm_connector_attach_scaling_mode_property(struct drm_connector
>> *connector,
>>         u32 scaling_mode_mask);
>> +int drm_connector_attach_content_protection_property(
>> + struct drm_connector *connector);
>>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>>  int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>>
>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> index 5597a87154e5..03f4d22305c2 100644
>> --- a/include/uapi/drm/drm_mode.h
>> +++ b/include/uapi/drm/drm_mode.h
>> @@ -173,6 +173,10 @@ extern "C" {
>>   DRM_MODE_REFLECT_X | \
>>   DRM_MODE_REFLECT_Y)
>>
>> +/* Content Protection Flags */
>> +#define DRM_MODE_CONTENT_PROTECTION_OFF 0
>> +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>> +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>>
>> What about HDCP 1.4 and HDCP 2.2? Userspace would need to know which version
>> was negotiated since content protected 4k videos require HDCP 2.2. Perhaps
>> provide a property with the HDCP version?
>>
>> I'm also missing a method for userspace to read the BKSV from the
>> transmitter.
>>
>> Hans,
>>
>> I guess you are asking about the use case explained at
>> http://www.spinics.net/lists/intel-gfx/msg134813.html
>>
>> Sean,
>> As this solution is only for 1.4, this version requirement might not matter
>> here.
>> But as a community could we at least ack such a need, so that inevitable
>> extension of this uAPI will be well thought about?
>>
>> As we discussed before Enum values required are
>>
>> -OFF           : Disable any content protection
>> -DESIRED       : For any possible HDCP protection (v1.4/2.2)
>> -DESIRED_TYPE1 : For HDCP2.2 only
>> -ENABLED       : Highest HDCP protection is enabled (Could be v1.4/2.2)
>> -ENABLED_TYPE1 : HDCP2.2 enabled
>>
> 
> I'd rather keep the property as-is and expose an HDCP version property
> alongside it (or perhaps something more elaborate that includes bksv
> and the downstream bksvs). The reason I prefer that is it will also
> cover the 1.2 vs 1.4 difference that is a more immediate need.

I agree with that. 'DESIRED' negotiates the 'highest' level it can, and all
that's needed is to report what that level is back to userspace.

I think it makes sense to add the version property now, otherwise applications
would have to add logic like: 'if property HDCP_VERSION exists, then use it,
otherwise assume HDCP 1.4', which is rather awkward.

Exporting the BKSV(s) can be postponed. If it is not there, then you simply
cannot perform certain functions. It would be nice if it is documented as a
TODO somewhere.

> This property is intentionally vague about the underlying encryption,
> and tying it to HDCP (and specific versions, at that) is not
> consistent with the design.

Indeed.

Regards,

	Hans

> 
> 
>> And another gap i am seeing with this uAPI is that there is no communication
>> back about the
>> HDCP authentication failure, as DESIRED will remain without transitioning
>> into ENABLED.
>> So userspace has to identify the failure of the HDCP req with polling and
>> Timeout.
>> Timeout also will vary with system to system, as
>>     with big downstream topology(worst case 127 devices with 7depth)
>> authentication could take 5+ Sec.
>>     with only receiver < few 100 mSec(~200mSec?)
>>
> 
> It's worked on CrOS for a number of years now, why change what isn't broken?
> 
> Sean
> 
>>
>> So could it help userspace if we could indicate the authentication failure.
>> Agreed that runtime link integrity lost is indicated by the ENABLED->DESIRED
>> transition.
>>
>> --Ram
>>
>>
>> Regards,
>>
>> Hans
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>

Ramalingam C Dec. 5, 2017, 5:11 p.m. UTC | #8

Best Regards,
Ramalingam C

> -----Original Message-----

> From: Sean Paul [mailto:[e-mail]]

> Sent: Tuesday, December 5, 2017 8:07 PM

> To: C, Ramalingam <[e-mail]>

> Cc: dri-devel <[e-mail]>; Hans Verkuil

> <[e-mail]>

> Subject: Re: [PATCH v3 3/9] drm: Add Content Protection property

> 

> On Tue, Dec 5, 2017 at 9:04 AM, Ramalingam C <[e-mail]>

> wrote:

> >

> >

> > On Tuesday 05 December 2017 01:37 PM, Hans Verkuil wrote:

> >

> > On 12/05/2017 06:15 AM, Sean Paul wrote:

> >

> > This patch adds a new optional connector property to allow userspace

> > to enable protection over the content it is displaying. This will

> > typically be implemented by the driver using HDCP.

> >

> > The property is a tri-state with the following values:

> > - OFF: Self explanatory, no content protection

> > - DESIRED: Userspace requests that the driver enable protection

> > - ENABLED: Once the driver has authenticated the link, it sets this

> > value

> >

> > The driver is responsible for downgrading ENABLED to DESIRED if the

> > link becomes unprotected. The driver should also maintain the

> > desiredness of protection across hotplug/dpms/suspend.

> >

> > If this looks familiar, I posted [1] this 3 years ago. We have been

> > using this in ChromeOS across exynos, mediatek, and rockchip over that

> > time.

> >

> > Changes in v2:

> >  - Pimp kerneldoc for content_protection_property (Daniel)

> >  - Drop sysfs attribute

> > Changes in v3:

> >  - None

> >

> > Cc: Daniel Vetter <[e-mail]>

> > Signed-off-by: Sean Paul <[e-mail]>

> >

> > [1]

> > https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.

> > html

> > ---

> >  drivers/gpu/drm/drm_atomic.c    |  8 +++++

> >  drivers/gpu/drm/drm_connector.c | 71

> > +++++++++++++++++++++++++++++++++++++++++

> >  drivers/gpu/drm/drm_sysfs.c     |  1 +

> >  include/drm/drm_connector.h     | 16 ++++++++++

> >  include/uapi/drm/drm_mode.h     |  4 +++

> >  5 files changed, 100 insertions(+)

> >

> > diff --git a/drivers/gpu/drm/drm_atomic.c

> > b/drivers/gpu/drm/drm_atomic.c index c2da5585e201..676025d755b2 100644

> > --- a/drivers/gpu/drm/drm_atomic.c

> > +++ b/drivers/gpu/drm/drm_atomic.c

> > @@ -1196,6 +1196,12 @@ static int

> > drm_atomic_connector_set_property(struct

> > drm_connector *connector,

> >   state->picture_aspect_ratio = val;

> >   } else if (property == connector->scaling_mode_property) {

> >   state->scaling_mode = val;

> > + } else if (property == connector->content_protection_property) { if

> > + (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {

> DRM_DEBUG_KMS("only

> > + drivers can set CP Enabled\n"); return -EINVAL; }

> > + state->content_protection = val;

> >   } else if (connector->funcs->atomic_set_property) {

> >   return connector->funcs->atomic_set_property(connector,

> >   state, property, val);

> > @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct

> > drm_connector *connector,

> >   *val = state->picture_aspect_ratio;

> >   } else if (property == connector->scaling_mode_property) {

> >   *val = state->scaling_mode;

> > + } else if (property == connector->content_protection_property) {

> > + *val = state->content_protection;

> >   } 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 f14b48e6e839..8626aa8f485e

> > 100644

> > --- a/drivers/gpu/drm/drm_connector.c

> > +++ b/drivers/gpu/drm/drm_connector.c

> > @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list

> > drm_tv_subconnector_enum_list[] = {

> > DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,

> >   drm_tv_subconnector_enum_list)

> >

> > +static struct drm_prop_enum_list drm_cp_enum_list[] = {

> > +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },

> > +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },

> > +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" }, };

> > +DRM_ENUM_NAME_FN(drm_get_content_protection_name,

> drm_cp_enum_list)

> > +

> >  /**

> >   * DOC: standard connector properties

> >   *

> > @@ -764,6 +771,34 @@

> DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,

> >   *      after modeset, the kernel driver may set this to "BAD" and issue a

> >   *      hotplug uevent. Drivers should update this value using

> >   *      drm_mode_connector_set_link_status_property().

> > + * Content Protection:

> > + * This property is used by userspace to request the kernel protect

> > + future

> > + * content communicated over the link. When requested, kernel will

> > + apply

> > + * the appropriate means of protection (most often HDCP), and use the

> > + * property to tell userspace the protection is active.

> > + *

> > + * The value of this property can be one of the following:

> > + *

> > + * - DRM_MODE_CONTENT_PROTECTION_OFF = 0

> > + * The link is not protected, content is transmitted in the clear.

> > + * - DRM_MODE_CONTENT_PROTECTION_DESIRED = 1

> > + * Userspace has requested content protection, but the link is not

> > + * currently protected. When in this state, kernel should enable

> > + * Content Protection as soon as possible.

> > + * - DRM_MODE_CONTENT_PROTECTION_ENABLED = 2

> > + * Userspace has requested content protection, and the link is

> > + * protected. Only the driver can set the property to this value.

> > + * If userspace attempts to set to ENABLED, kernel will return

> > + * -EINVAL.

> > + *

> > + * A few guidelines:

> > + *

> > + * - DESIRED state should be preserved until userspace de-asserts it

> > + by

> > + *  setting the property to OFF. This means ENABLED should only

> > + transition

> > + *  to OFF when the user explicitly requests it.

> > + * - If the state is DESIRED, kernel should attempt to

> > + re-authenticate the

> > + *  link whenever possible. This includes across disable/enable,

> > + dpms,

> > + *  hotplug, downstream device changes, link status failures, etc..

> >   *

> >   * Connectors also have one standardized atomic property:

> >   *

> > @@ -1047,6 +1082,42 @@ int

> > drm_connector_attach_scaling_mode_property(struct

> > drm_connector *connector,

> >  }

> >  EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);

> >

> > +/**

> > + * drm_connector_attach_content_protection_property - attach content

> > protection

> > + * property

> > + *

> > + * @connector: connector to attach CP property on.

> > + *

> > + * This is used to add support for content protection on select connectors.

> > + * Content Protection is intentionally vague to allow for different

> > underlying

> > + * technologies, however it is most implemented by HDCP.

> > + *

> > + * The content protection will be set to

> > &drm_connector_state.content_protection

> > + *

> > + * Returns:

> > + * Zero on success, negative errno on failure.

> > + */

> > +int drm_connector_attach_content_protection_property(

> > + struct drm_connector *connector)

> > +{

> > + struct drm_device *dev = connector->dev;  struct drm_property *prop;

> > +

> > + prop = drm_property_create_enum(dev, 0, "Content Protection",

> > + drm_cp_enum_list, ARRAY_SIZE(drm_cp_enum_list)); if (!prop) return

> > + -ENOMEM;

> > +

> > + drm_object_attach_property(&connector->base, prop,

> > +   DRM_MODE_CONTENT_PROTECTION_OFF);

> > +

> > + connector->content_protection_property = prop;

> > +

> > + return 0;

> > +}

> > +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);

> > +

> >  /**

> >   * drm_mode_create_aspect_ratio_property - create aspect ratio property

> >   * @dev: DRM device

> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c

> > index 1c5b5ce1fd7f..2385c7e0bef5 100644

> > --- a/drivers/gpu/drm/drm_sysfs.c

> > +++ b/drivers/gpu/drm/drm_sysfs.c

> > @@ -21,6 +21,7 @@

> >  #include <drm/drm_sysfs.h>

> >  #include <drm/drmP.h>

> >  #include "drm_internal.h"

> > +#include "drm_crtc_internal.h"

> >

> >  #define to_drm_minor(d) dev_get_drvdata(d)  #define

> > to_drm_connector(d) dev_get_drvdata(d) diff --git

> > a/include/drm/drm_connector.h b/include/drm/drm_connector.h index

> > 7a7140543012..828878addd03 100644

> > --- a/include/drm/drm_connector.h

> > +++ b/include/drm/drm_connector.h

> > @@ -370,6 +370,12 @@ struct drm_connector_state {

> >   * upscaling, mostly used for built-in panels.

> >   */

> >   unsigned int scaling_mode;

> > +

> > + /**

> > + * @content_protection: Connector property to request content

> > + * protection. This is most commonly used for HDCP.

> > + */

> > + unsigned int content_protection;

> >  };

> >

> >  /**

> > @@ -718,6 +724,7 @@ struct drm_cmdline_mode {

> >   * @tile_h_size: horizontal size of this tile.

> >   * @tile_v_size: vertical size of this tile.

> >   * @scaling_mode_property:  Optional atomic property to control the

> > upscaling.

> > + * @content_protection_property: Optional property to control content

> > protection

> >   *

> >   * Each connector may be connected to one or more CRTCs, or may be

> > clonable by

> >   * another connector if they can share a CRTC.  Each connector also

> > has a specific @@ -808,6 +815,12 @@ struct drm_connector {

> >

> >   struct drm_property *scaling_mode_property;

> >

> > + /**

> > + * @content_protection_property: DRM ENUM property for content

> > + * protection

> > + */

> > + struct drm_property *content_protection_property;

> > +

> >   /**

> >   * @path_blob_ptr:

> >   *

> > @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int

> > val);  const char *drm_get_dvi_i_select_name(int val);  const char

> > *drm_get_tv_subconnector_name(int val);  const char

> > *drm_get_tv_select_name(int val);

> > +const char *drm_get_content_protection_name(int val);

> >

> >  int drm_mode_create_dvi_i_properties(struct drm_device *dev);  int

> > drm_mode_create_tv_properties(struct drm_device *dev, @@ -1010,6

> > +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device *dev,

> > int drm_mode_create_scaling_mode_property(struct drm_device *dev);

> > int drm_connector_attach_scaling_mode_property(struct drm_connector

> > *connector,

> >         u32 scaling_mode_mask);

> > +int drm_connector_attach_content_protection_property(

> > + struct drm_connector *connector);

> >  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);

> > int drm_mode_create_suggested_offset_properties(struct drm_device

> > *dev);

> >

> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h

> > index 5597a87154e5..03f4d22305c2 100644

> > --- a/include/uapi/drm/drm_mode.h

> > +++ b/include/uapi/drm/drm_mode.h

> > @@ -173,6 +173,10 @@ extern "C" {

> >   DRM_MODE_REFLECT_X | \

> >   DRM_MODE_REFLECT_Y)

> >

> > +/* Content Protection Flags */

> > +#define DRM_MODE_CONTENT_PROTECTION_OFF 0

> > +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1

> > +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2

> >

> > What about HDCP 1.4 and HDCP 2.2? Userspace would need to know which

> > version was negotiated since content protected 4k videos require HDCP

> > 2.2. Perhaps provide a property with the HDCP version?

> >

> > I'm also missing a method for userspace to read the BKSV from the

> > transmitter.

> >

> > Hans,

> >

> > I guess you are asking about the use case explained at

> > http://www.spinics.net/lists/intel-gfx/msg134813.html

> >

> > Sean,

> > As this solution is only for 1.4, this version requirement might not

> > matter here.

> > But as a community could we at least ack such a need, so that

> > inevitable extension of this uAPI will be well thought about?

> >

> > As we discussed before Enum values required are

> >

> > -OFF           : Disable any content protection

> > -DESIRED       : For any possible HDCP protection (v1.4/2.2)

> > -DESIRED_TYPE1 : For HDCP2.2 only

> > -ENABLED       : Highest HDCP protection is enabled (Could be v1.4/2.2)

> > -ENABLED_TYPE1 : HDCP2.2 enabled

> >

> 

> I'd rather keep the property as-is and expose an HDCP version property

> alongside it (or perhaps something more elaborate that includes bksv and the

> downstream bksvs). The reason I prefer that is it will also cover the 1.2 vs 1.4

> difference that is a more immediate need.


HDCP specification wants to differentiate 2.2 vs <2.2 as 2.2 is not a backward compatible.
Why do we need to differentiate 1.2 and 1.4? Any use case?

> 

> This property is intentionally vague about the underlying encryption, and tying it

> to HDCP (and specific versions, at that) is not consistent with the design.


To differentiate non 2.2(mostly 1.4?) vs 2.2,  we could add another enum for content_type with states
	Type 0 - Indicating protected content for any HDCP protection
	Type 1 - Indicating protected content for HDCP2.2 only

And with respect to passing downstream topology information(bksv list, device count and depth) to userspace, we might need blob property!?
Anyway we need to worry about this only when we have a userspace consumer for such need.

> 

> 

> > And another gap i am seeing with this uAPI is that there is no

> > communication back about the HDCP authentication failure, as DESIRED

> > will remain without transitioning into ENABLED.

> > So userspace has to identify the failure of the HDCP req with polling

> > and Timeout.

> > Timeout also will vary with system to system, as

> >     with big downstream topology(worst case 127 devices with 7depth)

> > authentication could take 5+ Sec.

> >     with only receiver < few 100 mSec(~200mSec?)

> >

> 

> It's worked on CrOS for a number of years now, why change what isn't broken?


I am confident it will work as intact :) I am just curious if we can do it better
for all possible consumer here. So just mentioning the gaps here for discussion.

Main concern is that as far as I understand if new uAPI is accepted we are not allowed to modify easily, if at all.
So IMHO we must think through such that immediate required
usecases can be covered by just extending this uAPI or by simple additions.

--Ram

> 

> Sean

> 

> >

> > So could it help userspace if we could indicate the authentication failure.

> > Agreed that runtime link integrity lost is indicated by the

> > ENABLED->DESIRED transition.

> >

> > --Ram

> >

> >

> > Regards,

> >

> > Hans

> > _______________________________________________

> > dri-devel mailing list

> > [e-mail]

> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

> >

> >

> >

> > _______________________________________________

> > dri-devel mailing list

> > [e-mail]

> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

> >

Sean Paul Dec. 6, 2017, 4:26 p.m. UTC | #9

On Tue, Dec 5, 2017 at 12:11 PM, C, Ramalingam <[e-mail]> wrote:
>
>
>
> Best Regards,
> Ramalingam C
>
>> -----Original Message-----
>> From: Sean Paul [mailto:[e-mail]]
>> Sent: Tuesday, December 5, 2017 8:07 PM
>> To: C, Ramalingam <[e-mail]>
>> Cc: dri-devel <[e-mail]>; Hans Verkuil
>> <[e-mail]>
>> Subject: Re: [PATCH v3 3/9] drm: Add Content Protection property
>>
>> On Tue, Dec 5, 2017 at 9:04 AM, Ramalingam C <[e-mail]>
>> wrote:
>> >
>> >
>> > On Tuesday 05 December 2017 01:37 PM, Hans Verkuil wrote:
>> >
>> > On 12/05/2017 06:15 AM, Sean Paul wrote:
>> >
>> > This patch adds a new optional connector property to allow userspace
>> > to enable protection over the content it is displaying. This will
>> > typically be implemented by the driver using HDCP.
>> >
>> > The property is a tri-state with the following values:
>> > - OFF: Self explanatory, no content protection
>> > - DESIRED: Userspace requests that the driver enable protection
>> > - ENABLED: Once the driver has authenticated the link, it sets this
>> > value
>> >
>> > The driver is responsible for downgrading ENABLED to DESIRED if the
>> > link becomes unprotected. The driver should also maintain the
>> > desiredness of protection across hotplug/dpms/suspend.
>> >
>> > If this looks familiar, I posted [1] this 3 years ago. We have been
>> > using this in ChromeOS across exynos, mediatek, and rockchip over that
>> > time.
>> >
>> > Changes in v2:
>> >  - Pimp kerneldoc for content_protection_property (Daniel)
>> >  - Drop sysfs attribute
>> > Changes in v3:
>> >  - None
>> >
>> > Cc: Daniel Vetter <[e-mail]>
>> > Signed-off-by: Sean Paul <[e-mail]>
>> >
>> > [1]
>> > https://lists.freedesktop.org/archives/dri-devel/2014-December/073336.
>> > html
>> > ---
>> >  drivers/gpu/drm/drm_atomic.c    |  8 +++++
>> >  drivers/gpu/drm/drm_connector.c | 71
>> > +++++++++++++++++++++++++++++++++++++++++
>> >  drivers/gpu/drm/drm_sysfs.c     |  1 +
>> >  include/drm/drm_connector.h     | 16 ++++++++++
>> >  include/uapi/drm/drm_mode.h     |  4 +++
>> >  5 files changed, 100 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_atomic.c
>> > b/drivers/gpu/drm/drm_atomic.c index c2da5585e201..676025d755b2 100644
>> > --- a/drivers/gpu/drm/drm_atomic.c
>> > +++ b/drivers/gpu/drm/drm_atomic.c
>> > @@ -1196,6 +1196,12 @@ static int
>> > drm_atomic_connector_set_property(struct
>> > drm_connector *connector,
>> >   state->picture_aspect_ratio = val;
>> >   } else if (property == connector->scaling_mode_property) {
>> >   state->scaling_mode = val;
>> > + } else if (property == connector->content_protection_property) { if
>> > + (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
>> DRM_DEBUG_KMS("only
>> > + drivers can set CP Enabled\n"); return -EINVAL; }
>> > + state->content_protection = val;
>> >   } else if (connector->funcs->atomic_set_property) {
>> >   return connector->funcs->atomic_set_property(connector,
>> >   state, property, val);
>> > @@ -1275,6 +1281,8 @@ drm_atomic_connector_get_property(struct
>> > drm_connector *connector,
>> >   *val = state->picture_aspect_ratio;
>> >   } else if (property == connector->scaling_mode_property) {
>> >   *val = state->scaling_mode;
>> > + } else if (property == connector->content_protection_property) {
>> > + *val = state->content_protection;
>> >   } 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 f14b48e6e839..8626aa8f485e
>> > 100644
>> > --- a/drivers/gpu/drm/drm_connector.c
>> > +++ b/drivers/gpu/drm/drm_connector.c
>> > @@ -698,6 +698,13 @@ static const struct drm_prop_enum_list
>> > drm_tv_subconnector_enum_list[] = {
>> > DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>> >   drm_tv_subconnector_enum_list)
>> >
>> > +static struct drm_prop_enum_list drm_cp_enum_list[] = {
>> > +        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
>> > +        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
>> > +        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" }, };
>> > +DRM_ENUM_NAME_FN(drm_get_content_protection_name,
>> drm_cp_enum_list)
>> > +
>> >  /**
>> >   * DOC: standard connector properties
>> >   *
>> > @@ -764,6 +771,34 @@
>> DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>> >   *      after modeset, the kernel driver may set this to "BAD" and issue a
>> >   *      hotplug uevent. Drivers should update this value using
>> >   *      drm_mode_connector_set_link_status_property().
>> > + * Content Protection:
>> > + * This property is used by userspace to request the kernel protect
>> > + future
>> > + * content communicated over the link. When requested, kernel will
>> > + apply
>> > + * the appropriate means of protection (most often HDCP), and use the
>> > + * property to tell userspace the protection is active.
>> > + *
>> > + * The value of this property can be one of the following:
>> > + *
>> > + * - DRM_MODE_CONTENT_PROTECTION_OFF = 0
>> > + * The link is not protected, content is transmitted in the clear.
>> > + * - DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
>> > + * Userspace has requested content protection, but the link is not
>> > + * currently protected. When in this state, kernel should enable
>> > + * Content Protection as soon as possible.
>> > + * - DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
>> > + * Userspace has requested content protection, and the link is
>> > + * protected. Only the driver can set the property to this value.
>> > + * If userspace attempts to set to ENABLED, kernel will return
>> > + * -EINVAL.
>> > + *
>> > + * A few guidelines:
>> > + *
>> > + * - DESIRED state should be preserved until userspace de-asserts it
>> > + by
>> > + *  setting the property to OFF. This means ENABLED should only
>> > + transition
>> > + *  to OFF when the user explicitly requests it.
>> > + * - If the state is DESIRED, kernel should attempt to
>> > + re-authenticate the
>> > + *  link whenever possible. This includes across disable/enable,
>> > + dpms,
>> > + *  hotplug, downstream device changes, link status failures, etc..
>> >   *
>> >   * Connectors also have one standardized atomic property:
>> >   *
>> > @@ -1047,6 +1082,42 @@ int
>> > drm_connector_attach_scaling_mode_property(struct
>> > drm_connector *connector,
>> >  }
>> >  EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
>> >
>> > +/**
>> > + * drm_connector_attach_content_protection_property - attach content
>> > protection
>> > + * property
>> > + *
>> > + * @connector: connector to attach CP property on.
>> > + *
>> > + * This is used to add support for content protection on select connectors.
>> > + * Content Protection is intentionally vague to allow for different
>> > underlying
>> > + * technologies, however it is most implemented by HDCP.
>> > + *
>> > + * The content protection will be set to
>> > &drm_connector_state.content_protection
>> > + *
>> > + * Returns:
>> > + * Zero on success, negative errno on failure.
>> > + */
>> > +int drm_connector_attach_content_protection_property(
>> > + struct drm_connector *connector)
>> > +{
>> > + struct drm_device *dev = connector->dev;  struct drm_property *prop;
>> > +
>> > + prop = drm_property_create_enum(dev, 0, "Content Protection",
>> > + drm_cp_enum_list, ARRAY_SIZE(drm_cp_enum_list)); if (!prop) return
>> > + -ENOMEM;
>> > +
>> > + drm_object_attach_property(&connector->base, prop,
>> > +   DRM_MODE_CONTENT_PROTECTION_OFF);
>> > +
>> > + connector->content_protection_property = prop;
>> > +
>> > + return 0;
>> > +}
>> > +EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
>> > +
>> >  /**
>> >   * drm_mode_create_aspect_ratio_property - create aspect ratio property
>> >   * @dev: DRM device
>> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> > index 1c5b5ce1fd7f..2385c7e0bef5 100644
>> > --- a/drivers/gpu/drm/drm_sysfs.c
>> > +++ b/drivers/gpu/drm/drm_sysfs.c
>> > @@ -21,6 +21,7 @@
>> >  #include <drm/drm_sysfs.h>
>> >  #include <drm/drmP.h>
>> >  #include "drm_internal.h"
>> > +#include "drm_crtc_internal.h"
>> >
>> >  #define to_drm_minor(d) dev_get_drvdata(d)  #define
>> > to_drm_connector(d) dev_get_drvdata(d) diff --git
>> > a/include/drm/drm_connector.h b/include/drm/drm_connector.h index
>> > 7a7140543012..828878addd03 100644
>> > --- a/include/drm/drm_connector.h
>> > +++ b/include/drm/drm_connector.h
>> > @@ -370,6 +370,12 @@ struct drm_connector_state {
>> >   * upscaling, mostly used for built-in panels.
>> >   */
>> >   unsigned int scaling_mode;
>> > +
>> > + /**
>> > + * @content_protection: Connector property to request content
>> > + * protection. This is most commonly used for HDCP.
>> > + */
>> > + unsigned int content_protection;
>> >  };
>> >
>> >  /**
>> > @@ -718,6 +724,7 @@ struct drm_cmdline_mode {
>> >   * @tile_h_size: horizontal size of this tile.
>> >   * @tile_v_size: vertical size of this tile.
>> >   * @scaling_mode_property:  Optional atomic property to control the
>> > upscaling.
>> > + * @content_protection_property: Optional property to control content
>> > protection
>> >   *
>> >   * Each connector may be connected to one or more CRTCs, or may be
>> > clonable by
>> >   * another connector if they can share a CRTC.  Each connector also
>> > has a specific @@ -808,6 +815,12 @@ struct drm_connector {
>> >
>> >   struct drm_property *scaling_mode_property;
>> >
>> > + /**
>> > + * @content_protection_property: DRM ENUM property for content
>> > + * protection
>> > + */
>> > + struct drm_property *content_protection_property;
>> > +
>> >   /**
>> >   * @path_blob_ptr:
>> >   *
>> > @@ -1002,6 +1015,7 @@ const char *drm_get_dvi_i_subconnector_name(int
>> > val);  const char *drm_get_dvi_i_select_name(int val);  const char
>> > *drm_get_tv_subconnector_name(int val);  const char
>> > *drm_get_tv_select_name(int val);
>> > +const char *drm_get_content_protection_name(int val);
>> >
>> >  int drm_mode_create_dvi_i_properties(struct drm_device *dev);  int
>> > drm_mode_create_tv_properties(struct drm_device *dev, @@ -1010,6
>> > +1024,8 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
>> > int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>> > int drm_connector_attach_scaling_mode_property(struct drm_connector
>> > *connector,
>> >         u32 scaling_mode_mask);
>> > +int drm_connector_attach_content_protection_property(
>> > + struct drm_connector *connector);
>> >  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>> > int drm_mode_create_suggested_offset_properties(struct drm_device
>> > *dev);
>> >
>> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>> > index 5597a87154e5..03f4d22305c2 100644
>> > --- a/include/uapi/drm/drm_mode.h
>> > +++ b/include/uapi/drm/drm_mode.h
>> > @@ -173,6 +173,10 @@ extern "C" {
>> >   DRM_MODE_REFLECT_X | \
>> >   DRM_MODE_REFLECT_Y)
>> >
>> > +/* Content Protection Flags */
>> > +#define DRM_MODE_CONTENT_PROTECTION_OFF 0
>> > +#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>> > +#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>> >
>> > What about HDCP 1.4 and HDCP 2.2? Userspace would need to know which
>> > version was negotiated since content protected 4k videos require HDCP
>> > 2.2. Perhaps provide a property with the HDCP version?
>> >
>> > I'm also missing a method for userspace to read the BKSV from the
>> > transmitter.
>> >
>> > Hans,
>> >
>> > I guess you are asking about the use case explained at
>> > http://www.spinics.net/lists/intel-gfx/msg134813.html
>> >
>> > Sean,
>> > As this solution is only for 1.4, this version requirement might not
>> > matter here.
>> > But as a community could we at least ack such a need, so that
>> > inevitable extension of this uAPI will be well thought about?
>> >
>> > As we discussed before Enum values required are
>> >
>> > -OFF           : Disable any content protection
>> > -DESIRED       : For any possible HDCP protection (v1.4/2.2)
>> > -DESIRED_TYPE1 : For HDCP2.2 only
>> > -ENABLED       : Highest HDCP protection is enabled (Could be v1.4/2.2)
>> > -ENABLED_TYPE1 : HDCP2.2 enabled
>> >
>>
>> I'd rather keep the property as-is and expose an HDCP version property
>> alongside it (or perhaps something more elaborate that includes bksv and the
>> downstream bksvs). The reason I prefer that is it will also cover the 1.2 vs 1.4
>> difference that is a more immediate need.
>
> HDCP specification wants to differentiate 2.2 vs <2.2 as 2.2 is not a backward compatible.
> Why do we need to differentiate 1.2 and 1.4? Any use case?
>

Someone on the Chrome side asked me to surface the version, apparently
they care about 1.2 vs 1.4.

>>
>> This property is intentionally vague about the underlying encryption, and tying it
>> to HDCP (and specific versions, at that) is not consistent with the design.
>
> To differentiate non 2.2(mostly 1.4?) vs 2.2,  we could add another enum for content_type with states
>         Type 0 - Indicating protected content for any HDCP protection
>         Type 1 - Indicating protected content for HDCP2.2 only
>
> And with respect to passing downstream topology information(bksv list, device count and depth) to userspace, we might need blob property!?
> Anyway we need to worry about this only when we have a userspace consumer for such need.
>

Yeah, so if we're already adding a blob, perhaps version fits better
there. At any rate, I don't want to speculate on what uapi we'll need
in the future if I don't have to. So until I dig into exposing
version, I'd rather defer this decision.

>>
>>
>> > And another gap i am seeing with this uAPI is that there is no
>> > communication back about the HDCP authentication failure, as DESIRED
>> > will remain without transitioning into ENABLED.
>> > So userspace has to identify the failure of the HDCP req with polling
>> > and Timeout.
>> > Timeout also will vary with system to system, as
>> >     with big downstream topology(worst case 127 devices with 7depth)
>> > authentication could take 5+ Sec.
>> >     with only receiver < few 100 mSec(~200mSec?)
>> >
>>
>> It's worked on CrOS for a number of years now, why change what isn't broken?
>
> I am confident it will work as intact :) I am just curious if we can do it better
> for all possible consumer here. So just mentioning the gaps here for discussion.
>
> Main concern is that as far as I understand if new uAPI is accepted we are not allowed to modify easily, if at all.

It can be modified as long as it's backwards compatible. At the
moment, the only userspace is Chrome, and afaik, everything is happy
there.

Sean

> So IMHO we must think through such that immediate required
> usecases can be covered by just extending this uAPI or by simple additions.
>
> --Ram
>
>>
>> Sean
>>
>> >
>> > So could it help userspace if we could indicate the authentication failure.
>> > Agreed that runtime link integrity lost is indicated by the
>> > ENABLED->DESIRED transition.
>> >
>> > --Ram
>> >
>> >
>> > Regards,
>> >
>> > Hans
>> > _______________________________________________
>> > dri-devel mailing list
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >

Ramalingam C Dec. 7, 2017, 4:34 a.m. UTC | #10

On Wednesday 06 December 2017 09:56 PM, Sean Paul wrote:
>>> I'd rather keep the property as-is and expose an HDCP version property
>>> alongside it (or perhaps something more elaborate that includes bksv and the
>>> downstream bksvs). The reason I prefer that is it will also cover the 1.2 vs 1.4
>>> difference that is a more immediate need.
>> HDCP specification wants to differentiate 2.2 vs <2.2 as 2.2 is not a backward compatible.
>> Why do we need to differentiate 1.2 and 1.4? Any use case?
>>
> Someone on the Chrome side asked me to surface the version, apparently
> they care about 1.2 vs 1.4.
>
Sean, if you can get the reason for differentiating v1.2 and v1.4 at 
chrome, it will be educative info. - Thanks

-Ram

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c2da5585e201..676025d755b2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1196,6 +1196,12 @@  static int drm_atomic_connector_set_property(struct drm_connector *connector,
 		state->picture_aspect_ratio = val;
 	} else if (property == connector->scaling_mode_property) {
 		state->scaling_mode = val;
+	} else if (property == connector->content_protection_property) {
+		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
+			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
+			return -EINVAL;
+		}
+		state->content_protection = val;
 	} else if (connector->funcs->atomic_set_property) {
 		return connector->funcs->atomic_set_property(connector,
 				state, property, val);
@@ -1275,6 +1281,8 @@  drm_atomic_connector_get_property(struct drm_connector *connector,
 		*val = state->picture_aspect_ratio;
 	} else if (property == connector->scaling_mode_property) {
 		*val = state->scaling_mode;
+	} else if (property == connector->content_protection_property) {
+		*val = state->content_protection;
 	} 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 f14b48e6e839..8626aa8f485e 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -698,6 +698,13 @@  static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
 DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
 		 drm_tv_subconnector_enum_list)
 
+static struct drm_prop_enum_list drm_cp_enum_list[] = {
+        { DRM_MODE_CONTENT_PROTECTION_OFF, "Off" },
+        { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
+        { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },
+};
+DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
+
 /**
  * DOC: standard connector properties
  *
@@ -764,6 +771,34 @@  DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
  *      after modeset, the kernel driver may set this to "BAD" and issue a
  *      hotplug uevent. Drivers should update this value using
  *      drm_mode_connector_set_link_status_property().
+ * Content Protection:
+ *	This property is used by userspace to request the kernel protect future
+ *	content communicated over the link. When requested, kernel will apply
+ *	the appropriate means of protection (most often HDCP), and use the
+ *	property to tell userspace the protection is active.
+ *
+ *	The value of this property can be one of the following:
+ *
+ *	- DRM_MODE_CONTENT_PROTECTION_OFF = 0
+ *		The link is not protected, content is transmitted in the clear.
+ *	- DRM_MODE_CONTENT_PROTECTION_DESIRED = 1
+ *		Userspace has requested content protection, but the link is not
+ *		currently protected. When in this state, kernel should enable
+ *		Content Protection as soon as possible.
+ *	- DRM_MODE_CONTENT_PROTECTION_ENABLED = 2
+ *		Userspace has requested content protection, and the link is
+ *		protected. Only the driver can set the property to this value.
+ *		If userspace attempts to set to ENABLED, kernel will return
+ *		-EINVAL.
+ *
+ *	A few guidelines:
+ *
+ *	- DESIRED state should be preserved until userspace de-asserts it by
+ *	  setting the property to OFF. This means ENABLED should only transition
+ *	  to OFF when the user explicitly requests it.
+ *	- If the state is DESIRED, kernel should attempt to re-authenticate the
+ *	  link whenever possible. This includes across disable/enable, dpms,
+ *	  hotplug, downstream device changes, link status failures, etc..
  *
  * Connectors also have one standardized atomic property:
  *
@@ -1047,6 +1082,42 @@  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
 
+/**
+ * drm_connector_attach_content_protection_property - attach content protection
+ * property
+ *
+ * @connector: connector to attach CP property on.
+ *
+ * This is used to add support for content protection on select connectors.
+ * Content Protection is intentionally vague to allow for different underlying
+ * technologies, however it is most implemented by HDCP.
+ *
+ * The content protection will be set to &drm_connector_state.content_protection
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int drm_connector_attach_content_protection_property(
+		struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_property *prop;
+
+	prop = drm_property_create_enum(dev, 0, "Content Protection",
+					drm_cp_enum_list,
+					ARRAY_SIZE(drm_cp_enum_list));
+	if (!prop)
+		return -ENOMEM;
+
+	drm_object_attach_property(&connector->base, prop,
+				   DRM_MODE_CONTENT_PROTECTION_OFF);
+
+	connector->content_protection_property = prop;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
+
 /**
  * drm_mode_create_aspect_ratio_property - create aspect ratio property
  * @dev: DRM device
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 1c5b5ce1fd7f..2385c7e0bef5 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -21,6 +21,7 @@ 
 #include <drm/drm_sysfs.h>
 #include <drm/drmP.h>
 #include "drm_internal.h"
+#include "drm_crtc_internal.h"
 
 #define to_drm_minor(d) dev_get_drvdata(d)
 #define to_drm_connector(d) dev_get_drvdata(d)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 7a7140543012..828878addd03 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -370,6 +370,12 @@  struct drm_connector_state {
 	 * upscaling, mostly used for built-in panels.
 	 */
 	unsigned int scaling_mode;
+
+	/**
+	 * @content_protection: Connector property to request content
+	 * protection. This is most commonly used for HDCP.
+	 */
+	unsigned int content_protection;
 };
 
 /**
@@ -718,6 +724,7 @@  struct drm_cmdline_mode {
  * @tile_h_size: horizontal size of this tile.
  * @tile_v_size: vertical size of this tile.
  * @scaling_mode_property:  Optional atomic property to control the upscaling.
+ * @content_protection_property: Optional property to control content protection
  *
  * Each connector may be connected to one or more CRTCs, or may be clonable by
  * another connector if they can share a CRTC.  Each connector also has a specific
@@ -808,6 +815,12 @@  struct drm_connector {
 
 	struct drm_property *scaling_mode_property;
 
+	/**
+	 * @content_protection_property: DRM ENUM property for content
+	 * protection
+	 */
+	struct drm_property *content_protection_property;
+
 	/**
 	 * @path_blob_ptr:
 	 *
@@ -1002,6 +1015,7 @@  const char *drm_get_dvi_i_subconnector_name(int val);
 const char *drm_get_dvi_i_select_name(int val);
 const char *drm_get_tv_subconnector_name(int val);
 const char *drm_get_tv_select_name(int val);
+const char *drm_get_content_protection_name(int val);
 
 int drm_mode_create_dvi_i_properties(struct drm_device *dev);
 int drm_mode_create_tv_properties(struct drm_device *dev,
@@ -1010,6 +1024,8 @@  int drm_mode_create_tv_properties(struct drm_device *dev,
 int drm_mode_create_scaling_mode_property(struct drm_device *dev);
 int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
 					       u32 scaling_mode_mask);
+int drm_connector_attach_content_protection_property(
+		struct drm_connector *connector);
 int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
 int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 5597a87154e5..03f4d22305c2 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -173,6 +173,10 @@  extern "C" {
 		DRM_MODE_REFLECT_X | \
 		DRM_MODE_REFLECT_Y)
 
+/* Content Protection Flags */
+#define DRM_MODE_CONTENT_PROTECTION_OFF		0
+#define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
+#define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
 
 struct drm_mode_modeinfo {
 	__u32 clock;