Re: [PATCH v3 02/10] drm: Add Content protection type property

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

On 2019-03-27 at 10:56:32 +0100, Daniel Vetter wrote:
> On Fri, Mar 22, 2019 at 06:14:40AM +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]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
> >  drivers/gpu/drm/drm_connector.c   | 63 +++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       | 15 ++++++++
> >  include/uapi/drm/drm_mode.h       |  4 ++
> >  4 files changed, 86 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 4eb81f10bc54..857ca6fa6fd0 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -746,6 +746,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> >  			return -EINVAL;
> >  		}
> >  		state->content_protection = val;
> > +	} else if (property == connector->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) {
> > @@ -822,6 +824,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >  		*val = state->scaling_mode;
> >  	} else if (property == connector->content_protection_property) {
> >  		*val = state->content_protection;
> > +	} else if (property == connector->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 2355124849db..ff61c3208307 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
> > + *	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. 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
> > @@ -1551,6 +1575,45 @@ int drm_connector_attach_content_protection_property(
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_content_protection_property);
> >  
> > +/**
> > + * drm_connector_attach_hdcp_content_type_property - attach HDCP
> > + * content type property
> > + *
> > + * @connector: connector to attach HDCP content type property on.
> > + *
> > + * This is used to add support 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.
> > + *
> > + * This information will be used during the HDCP 2.2 authentication.
> > + *
> > + * Content type will be set to &drm_connector_state.hdcp_content_type.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int
> > +drm_connector_attach_hdcp_content_type_property(struct drm_connector *
> > +						      connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_property *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 we always create the same property, then the usual pattern is to have a
> global one stored in dev->mode_config, created at init time, and only
> attach it. You can attach the same property to multiple objects, and all
> objects will have distinct values for that property.
> 
> drm_property = metadata describing the name/value range/..., _not_ the value itself
> 
> I guess would be good to address that with content protection property
> too.
Just aligned with existing content protection proeprty. I will address
the content protection property first and then I will add the property
for the HDCP content type and HDCP topology in the same fucntion.
> 
> Also, not sure we need 2 functions for this, I'd just add a bool
> enable_content_type to drm_connector_attach_content_protection_property(),
> for a simpler interface in drivers.
> 
> 
> > +	if (!prop)
> > +		return -ENOMEM;
> > +
> > +	drm_object_attach_property(&connector->base, prop,
> > +				   DRM_MODE_HDCP_CONTENT_TYPE0);
> > +	connector->hdcp_content_type_property = prop;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_hdcp_content_type_property);
> > +
> >  /**
> >   * drm_mode_create_aspect_ratio_property - create aspect ratio property
> >   * @dev: DRM device
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index c8061992d6cb..f0830985367f 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -518,6 +518,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.
> > @@ -1035,6 +1041,12 @@ struct drm_connector {
> >  	 */
> >  	struct drm_property *colorspace_property;
> >  
> > +	/**
> > +	 * @hdcp_content_type_property: DRM ENUM property for type of
> > +	 * Protected Content.
> > +	 */
> > +	struct drm_property *hdcp_content_type_property;
> 
> Please move these two next to the existing content protection stuff.

some rebasing mistakes. Will address it.
> 
> > +
> >  	/**
> >  	 * @path_blob_ptr:
> >  	 *
> > @@ -1294,6 +1306,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);
> > @@ -1309,6 +1322,8 @@ int drm_connector_attach_vrr_capable_property(
> >  		struct drm_connector *connector);
> >  int drm_connector_attach_content_protection_property(
> >  		struct drm_connector *connector);
> > +int drm_connector_attach_hdcp_content_type_property(
> > +		struct drm_connector *connector);
> >  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/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index a439c2e67896..44412e8b77cd 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -210,6 +210,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
> > +
> >  struct drm_mode_modeinfo {
> >  	__u32 clock;
> >  	__u16 hdisplay;
> 
> Aside from the nits looks all good to me. Ofc we need an r-b/ack from
> userspace people on the interface itself.
Sure. Thanks a lot. Hopefully we will get it soon in weston.

-Ram
> -Daniel
> -- 
> 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