Re: [PATCH v5 03/12] drm: Add Content protection type property

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 18, 2019 at 02:27:56PM +0530, Ramalingam C wrote:
> This patch adds a DRM ENUM property to the selected connectors.
> This property is used for mentioning the protected content's type
> from userspace to kernel HDCP authentication.
> 
> Type of the stream is decided by the protected content providers.
> Type 0 content can be rendered on any HDCP protected display wires.
> But Type 1 content can be rendered only on HDCP2.2 protected paths.
> 
> So when a userspace sets this property to Type 1 and starts the HDCP
> enable, kernel will honour it only if HDCP2.2 authentication is through
> for type 1. Else HDCP enable will be failed.
> 
> v2:
>   cp_content_type is replaced with content_protection_type [daniel]
>   check at atomic_set_property is removed [Maarten]
> v3:
>   %s/content_protection_type/hdcp_content_type [Pekka]
> v4:
>   property is created for the first requested connector and then reused.
> 	[Danvet]
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_uapi.c |  4 +++
>  drivers/gpu/drm/drm_connector.c   | 53 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_hdcp.c |  4 ++-
>  include/drm/drm_connector.h       |  9 +++++-
>  include/drm/drm_mode_config.h     |  6 ++++
>  include/uapi/drm/drm_mode.h       |  4 +++
>  6 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 002dcede7915..0b0747869963 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -733,6 +733,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  			return -EINVAL;
>  		}
>  		state->content_protection = val;
> +	} else if (property == config->hdcp_content_type_property) {
> +		state->hdcp_content_type = val;
>  	} else if (property == connector->colorspace_property) {
>  		state->colorspace = val;
>  	} else if (property == config->writeback_fb_id_property) {
> @@ -809,6 +811,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->scaling_mode;
>  	} else if (property == config->content_protection_property) {
>  		*val = state->content_protection;
> +	} else if (property == config->hdcp_content_type_property) {
> +		*val = state->hdcp_content_type;
>  	} else if (property == config->writeback_fb_id_property) {
>  		/* Writeback framebuffer is one-shot, write and forget */
>  		*val = 0;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 7c0eda9cca60..03907d13ef66 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -857,6 +857,13 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>  	{ DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
>  };
>  
> +static struct drm_prop_enum_list drm_hdcp_content_type_enum_list[] = {
> +	{ DRM_MODE_HDCP_CONTENT_TYPE0, "HDCP Type0" },
> +	{ DRM_MODE_HDCP_CONTENT_TYPE1, "HDCP Type1" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,
> +		 drm_hdcp_content_type_enum_list)
> +
>  /**
>   * DOC: standard connector properties
>   *
> @@ -962,6 +969,23 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {
>   *	  the value transitions from ENABLED to DESIRED. This signifies the link
>   *	  is no longer protected and userspace should take appropriate action
>   *	  (whatever that might be).
> + * HDCP Content Type:
> + *	This property is used by the userspace to configure the kernel with
> + *	upcoming stream's content type. Content Type of a stream is decided by

"upcoming" sounds a bit like "in a future patch", which is confusing in
documentation. Maybe "to be displayed" instead of "upcoming"?

> + *	the owner of the stream, as HDCP Type0 or HDCP Type1.
> + *
> + *	The value of the property can be one the below:
> + *	  - DRM_MODE_HDCP_CONTENT_TYPE0 = 0
> + *		HDCP Type0 streams can be transmitted on a link which is
> + *		encrypted with HDCP 1.4 or HDCP 2.2.
> + *	  - DRM_MODE_HDCP_CONTENT_TYPE1 = 1
> + *		HDCP Type1 streams can be transmitted on a link which is
> + *		encrypted only with HDCP 2.2.
> + *
> + *	Please note this content type is introduced at HDCP 2.2 and used in its
> + *	authentication process.

I'd replace this with "Note that the HDCP Content Type property is
optional, and defaults to type 0. It is only exposed by drivers supporting
HDCP 2.x." I think that makes it clearer for userspace people what this
means.

> If content type is changed when
> + *	content_protection is not UNDESIRED, then kernel will disable the HDCP
> + *	and re-enable with new type in the same atomic commit
>   *
>   * max bpc:
>   *	This range property is used by userspace to limit the bit depth. When
> @@ -1520,18 +1544,29 @@ EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
>   * property
>   *
>   * @connector: connector to attach CP property on.
> + * @hdcp_content_type: is HDCP Content Type property needed for connector
>   *
>   * 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.
>   *
> + * When hdcp_content_type is true enum property called HDCP Content Type is
> + * created (if it is not already) and attached to the connector.
> + *
> + * This property is used for sending the protected content's stream type
> + * from userspace to kernel on selected connectors. Protected content provider
> + * will decide their type of their content and declare the same to kernel.
> + *
> + * Content type will be used during the HDCP 2.2 authentication.
> + * Content type will be set to &drm_connector_state.hdcp_content_type.
> + *
>   * 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_connector *connector, bool hdcp_content_type)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_property *prop =
> @@ -1548,6 +1583,22 @@ int drm_connector_attach_content_protection_property(
>  				   DRM_MODE_CONTENT_PROTECTION_UNDESIRED);
>  	dev->mode_config.content_protection_property = prop;
>  
> +	if (!hdcp_content_type)
> +		return 0;
> +
> +	prop = dev->mode_config.hdcp_content_type_property;
> +	if (!prop)
> +		prop = drm_property_create_enum(dev, 0, "HDCP Content Type",
> +					drm_hdcp_content_type_enum_list,
> +					ARRAY_SIZE(
> +					drm_hdcp_content_type_enum_list));
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&connector->base, prop,
> +				   DRM_MODE_HDCP_CONTENT_TYPE0);
> +	dev->mode_config.hdcp_content_type_property = prop;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 440fef448feb..a61391d374e8 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -1798,7 +1798,9 @@ int intel_hdcp_init(struct intel_connector *connector,
>  	if (!shim)
>  		return -EINVAL;
>  
> -	ret = drm_connector_attach_content_protection_property(&connector->base);
> +	ret =
> +	drm_connector_attach_content_protection_property(&connector->base,
> +							 false);
>  	if (ret)
>  		return ret;
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5e41942e5679..2d2c2d5e7681 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -556,6 +556,12 @@ struct drm_connector_state {
>  	 */
>  	unsigned int content_type;
>  
> +	/**
> +	 * @hdcp_content_type: Connector property to pass the type of
> +	 * protected content. This is most commonly used for HDCP.
> +	 */
> +	unsigned int hdcp_content_type;
> +
>  	/**
>  	 * @scaling_mode: Connector property to control the
>  	 * upscaling, mostly used for built-in panels.
> @@ -1326,6 +1332,7 @@ 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);
> +const char *drm_get_hdcp_content_type_name(int val);
>  
>  int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>  int drm_mode_create_tv_margin_properties(struct drm_device *dev);
> @@ -1340,7 +1347,7 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  int drm_connector_attach_vrr_capable_property(
>  		struct drm_connector *connector);
>  int drm_connector_attach_content_protection_property(
> -		struct drm_connector *connector);
> +		struct drm_connector *connector, bool hdcp_content_type);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>  int drm_mode_create_colorspace_property(struct drm_connector *connector);
>  int drm_mode_create_content_type_property(struct drm_device *dev);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 5764ee3c7453..b359b5b71eb9 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -842,6 +842,12 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *content_protection_property;
>  
> +	/**
> +	 * @hdcp_content_type_property: DRM ENUM property for type of
> +	 * Protected Content.
> +	 */
> +	struct drm_property *hdcp_content_type_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;
>  
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 83cd1636b9be..8ac03351fdee 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -209,6 +209,10 @@ extern "C" {
>  #define DRM_MODE_CONTENT_PROTECTION_DESIRED     1
>  #define DRM_MODE_CONTENT_PROTECTION_ENABLED     2
>  
> +/* Content Type classification for HDCP2.2 vs others */
> +#define DRM_MODE_HDCP_CONTENT_TYPE0		0
> +#define DRM_MODE_HDCP_CONTENT_TYPE1		1
> +

With the kernel doc nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  struct drm_mode_modeinfo {
>  	__u32 clock;
>  	__u16 hdisplay;
> -- 
> 2.19.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx