Re: [PATCH v7 07/11] drm: Add Content protection type property

[Ramalingam C <ramalingam.c@intel.com>]

On 2019-07-04 at 14:11:59 +0300, Pekka Paalanen wrote:
> On Tue,  7 May 2019 21:57:41 +0530
> Ramalingam C <ramalingam.c@intel.com> 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.
> > 
> > Need ACK for this new conenctor property from userspace consumer.
> > 
> > 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]
> > v5:
> >   kernel doc nits addressed [Daniel]
> >   Rebased as part of patch reordering.
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
> >  drivers/gpu/drm/drm_connector.c   | 18 ++++++++++++++++
> >  drivers/gpu/drm/drm_hdcp.c        | 36 ++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_hdcp.c |  4 +++-
> >  include/drm/drm_connector.h       |  7 ++++++
> >  include/drm/drm_hdcp.h            |  2 +-
> >  include/drm/drm_mode_config.h     |  6 ++++++
> >  include/uapi/drm/drm_mode.h       |  4 ++++
> >  8 files changed, 78 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 4131e669785a..a85f3ccfe699 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -738,6 +738,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) {
> > @@ -816,6 +818,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 764c7903edf6..de9e06583e8c 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> 
> Hi,
> 
> below I have some comments and questions before I can say whether
> https://gitlab.freedesktop.org/wayland/weston/merge_requests/48
> adheres to this specification.
> 
> > @@ -955,6 +955,24 @@ 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
> > + *	to be displayed stream's content type. Content Type of a stream is
> > + *	decided by 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
> 
> If this doc is meant as the UAPI doc, it needs to use the string names
> for enum property values, not the C language definitions (integers).
kernel documentation for all other properties followed this way. We
could add string associated to the enum state too for this property.
> 
> > + *		HDCP Type0 streams can be transmitted on a link which is
> > + *		encrypted with HDCP 1.4 or HDCP 2.2.
> 
> This wording forbids using any future HDCP version for type0.
We could change it to HDCP1.4 and higher version of HDCP.
> 
> > + *	  - DRM_MODE_HDCP_CONTENT_TYPE1 = 1
> > + *		HDCP Type1 streams can be transmitted on a link which is
> > + *		encrypted only with HDCP 2.2.
> 
> This wording forbids using any future HDCP version for type1.
As of now type 1 is only on HDCP2.2 encrypted link, as no higher version is
available. But as similar to Type 0 we could assume that Type 1 will be
supported on higher HDCP versions too and correct the sentence here.
> 
> > + *
> > + *	Note that the HDCP Content Type property is specific to HDCP 2.2, and
> > + *	defaults to type 0. It is only exposed by drivers supporting HDCP 2.2
> 
> Not possible with a future HDCP version greater than 2.2?
> 
> Is it intended to be possible to add more content types in the future?
Possible. As per HDCP2.2 spec, 8bits are reserved for the content Type.
> 
> > + *	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
> 
> Ok, very good to mention this.
> 
> >   *
> >   * max bpc:
> >   *	This range property is used by userspace to limit the bit depth. When
> > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > index 0da7b3718bad..75402463466b 100644
> > --- a/drivers/gpu/drm/drm_hdcp.c
> > +++ b/drivers/gpu/drm/drm_hdcp.c
> > @@ -342,23 +342,41 @@ static struct drm_prop_enum_list drm_cp_enum_list[] = {
> >  };
> >  DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
> >  
> > +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)
> > +
> >  /**
> >   * drm_connector_attach_content_protection_property - attach content protection
> >   * 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 =
> > @@ -375,6 +393,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/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
> 
> These should not be in a UAPI header. The C language definitions must
> never be exposed to userspace, or misguided userspace will start using
> them.
> 
> Instead, UAPI uses the string names to discover the integers at runtime.
This is done as per the usual practice for any existing enum property.
> 
> ***
> 
> Questions about the already existing "Content Protection" property:
> 
> - What happens if userspace attempts to write "Enabled" into it?
 *      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.
line number 936 @ drivers/gpu/drm/drm_connector.c

-Ram

> 
> - Where is the UAPI documentation that explains how userspace should be
>   using the property? (e.g. telling that you cannot set it to "Enabled")
> 
> 
> Thanks,
> pq


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx