Re: [PATCH v3 06/10] drm: Add CP downstream_info property

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

On 2019-03-27 at 11:25:04 +0100, Daniel Vetter wrote:
> On Fri, Mar 22, 2019 at 06:14:44AM +0530, Ramalingam C wrote:
> > This patch adds a optional CP downstream info blob property to the
> > connectors. This enables the Userspace to read the information of HDCP
> > authenticated downstream topology.
> > 
> > Driver will updated this blob with all downstream information at the
> > end of the authentication.
> > 
> > In case userspace configures this platform as repeater, then this
> > information is needed for the authentication with upstream HDCP
> > transmitter.
> > 
> > v2:
> >   s/cp_downstream/content_protection_downstream [daniel]
> > v3:
> >   s/content_protection_downstream/hdcp_topology [daniel]
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++
> >  drivers/gpu/drm/drm_connector.c   | 86 +++++++++++++++++++++++++++++++
> >  include/drm/drm_connector.h       | 12 +++++
> >  include/uapi/drm/drm_mode.h       | 27 ++++++++++
> >  4 files changed, 129 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > index 857ca6fa6fd0..4246e8988c29 100644
> > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > @@ -826,6 +826,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> >  		*val = state->content_protection;
> >  	} else if (property == connector->hdcp_content_type_property) {
> >  		*val = state->hdcp_content_type;
> > +	} else if (property ==
> > +		   connector->hdcp_topology_property) {
> > +		*val = connector->hdcp_topology_blob_ptr ?
> > +			connector->hdcp_topology_blob_ptr->base.id : 0;
> >  	} 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 ff61c3208307..0de8b441a449 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -246,6 +246,7 @@ int drm_connector_init(struct drm_device *dev,
> >  	mutex_init(&connector->mutex);
> >  	connector->edid_blob_ptr = NULL;
> >  	connector->tile_blob_ptr = NULL;
> > +	connector->hdcp_topology_blob_ptr = NULL;
> >  	connector->status = connector_status_unknown;
> >  	connector->display_info.panel_orientation =
> >  		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> > @@ -986,6 +987,25 @@ DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,
> >   *	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
> > + * HDCP Topology:
> > + *	This blob property is used to pass the HDCP downstream topology details
> > + *	of a HDCP encrypted connector, from kernel to userspace.
> > + *	This provides all required information to userspace, so that userspace
> > + *	can implement the HDCP repeater using the kernel as downstream ports of
> > + *	the repeater. as illustrated below:
> > + *
> > + *                          HDCP Repeaters
> > + * +--------------------------------------------------------------+
> > + * |                                                              |
> > + * |                               |                              |
> > + * |   Userspace HDCP Receiver  +----->    KMD HDCP transmitters  |
> > + * |      (Upstream Port)      <------+     (Downstream Ports)    |
> > + * |                               |                              |
> > + * |                                                              |
> > + * +--------------------------------------------------------------+
> 
> I didn't check, but I think this doesn't format correctly in the html
> output. I think you need to indent, and start with :: to denote a fixed
> width font example.
Looks good at mutt and at
https://patchwork.freedesktop.org/patch/293490/?series=57232&rev=4
Should i still do something on it?
> 
> > + *
> > + *	Kernel will populate this blob only when the HDCP authentication is
> > + *	successful.
> >   *
> >   * max bpc:
> >   *	This range property is used by userspace to limit the bit depth. When
> > @@ -1614,6 +1634,72 @@ drm_connector_attach_hdcp_content_type_property(struct drm_connector *
> >  }
> >  EXPORT_SYMBOL(drm_connector_attach_hdcp_content_type_property);
> >  
> > +/**
> > + * drm_connector_attach_hdcp_topology_property - attach hdcp topology property
> > + *
> > + * @connector: connector to attach hdcp topology property with.
> > + *
> > + * This is used to add support for hdcp topology support on select connectors.
> > + * When Intel platform is configured as repeater, this downstream info is used
> > + * by userspace, to complete the repeater authentication of HDCP specification
> > + * with upstream HDCP transmitter.
> > + *
> > + * The blob_id of the hdcp topology info will be set to
> > + * &drm_connector_state.hdcp_topology
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int drm_connector_attach_hdcp_topology_property(struct drm_connector *connector)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	struct drm_property *prop;
> > +
> > +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> > +				   DRM_MODE_PROP_IMMUTABLE,
> > +				   "HDCP Topology", 0);
> 
> Again global prop in dev->mode_config, and I think just add a flag to the
> overall "attach content protection stuff to connnector" function.
Will be done in the next version :)

-Ram
> 
> > +	if (!prop)
> > +		return -ENOMEM;
> > +
> > +	drm_object_attach_property(&connector->base, prop, 0);
> > +
> > +	connector->hdcp_topology_property = prop;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_connector_attach_hdcp_topology_property);
> > +
> > +/**
> > + * drm_connector_update_hdcp_topology_property - update the hdcp topology
> > + * property of a connector
> > + * @connector: drm connector, the topology is associated to
> > + * @hdcp_topology_info: new content for the blob of hdcp_topology property
> > + *
> > + * This function creates a new blob modeset object and assigns its id to the
> > + * connector's hdcp_topology property.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int
> > +drm_connector_update_hdcp_topology_property(struct drm_connector *connector,
> > +					const struct hdcp_topology_info *info)
> > +{
> > +	struct drm_device *dev = connector->dev;
> > +	int ret;
> > +
> > +	if (!info)
> > +		return -EINVAL;
> > +
> > +	ret = drm_property_replace_global_blob(dev,
> > +			&connector->hdcp_topology_blob_ptr,
> > +			sizeof(struct hdcp_topology_info),
> > +			info, &connector->base,
> > +			connector->hdcp_topology_property);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_connector_update_hdcp_topology_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 f0830985367f..c016a0bcedac 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -1047,6 +1047,13 @@ struct drm_connector {
> >  	 */
> >  	struct drm_property *hdcp_content_type_property;
> >  
> > +	/**
> > +	 * @hdcp_topology_property: DRM BLOB property for hdcp downstream
> > +	 * topology information.
> > +	 */
> > +	struct drm_property *hdcp_topology_property;
> > +	struct drm_property_blob *hdcp_topology_blob_ptr;
> 
> Need kerneldoc for both or kerneldoc gets a bit unhappy.
Sure.
> 
> > +
> >  	/**
> >  	 * @path_blob_ptr:
> >  	 *
> > @@ -1324,6 +1331,11 @@ 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_connector_attach_hdcp_topology_property(
> > +		struct drm_connector *connector);
> > +int drm_connector_update_hdcp_topology_property(
> > +		struct drm_connector *connector,
> > +		const struct hdcp_topology_info *info);
> >  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 44412e8b77cd..03d3aa2b1a49 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -214,6 +214,33 @@ extern "C" {
> >  #define DRM_MODE_HDCP_CONTENT_TYPE0		0
> >  #define DRM_MODE_HDCP_CONTENT_TYPE1		1
> >  
> > +#define DRM_MODE_HDCP_KSV_LEN			5
> > +#define DRM_MODE_HDCP_MAX_DEVICE_CNT		127
> > +
> > +struct hdcp_topology_info {
> > +	/* KSV of immediate HDCP Sink. In Little-Endian Format. */
> > +	char bksv[DRM_MODE_HDCP_KSV_LEN];
> 
> This isn't aligned to __u32. Just make the length 128 bytes.
> 
> > +
> > +	/* Whether Immediate HDCP sink is a repeater? */
> > +	bool is_repeater;
> 
> no bool in uapi structs. Just go with __u8.
> 
> > +
> > +	/* Depth received from immediate downstream repeater */
> > +	__u8 depth;
> 
> Needs to be aligned with explicit padding.
not sure why do we need this explicit padding. currently this struct
works between IGT and kernel. Might be co-incident too!!


Will check on this.
> 
> > +
> > +	/* Device count received from immediate downstream repeater */
> > +	__u32 device_count;
> > +
> > +	/*
> > +	 * Max buffer required to hold ksv list received from immediate
> > +	 * repeater. In this array first device_count * DRM_MODE_HDCP_KSV_LEN
> > +	 * will hold the valid ksv bytes.
> > +	 * If authentication specification is
> > +	 *	HDCP1.4 - each KSV's Bytes will be in Little-Endian format.
> > +	 *	HDCP2.2 - each KSV's Bytes will be in Big-Endian format.
> 
> Why is this ksv list be for hdcp2.2, but bksv is le for both case? I'm
> confused.
I used the KSV and receiver ID interchangeably. receiver ID/KSV lists
from repeater changes with endianness. I guess we need not bother about
it at present.
> 
> > +	 */
> > +	char ksv_list[DRM_MODE_HDCP_KSV_LEN * DRM_MODE_HDCP_MAX_DEVICE_CNT];
> 
> Again better to align this. Also maybe make it a nested array (it's the
> same underlying layout, but easier to use for userspace.)
> 
> For uapi struct recommendations, see https://gitlab.com/TeeFirefly/linux-kernel/blob/7408b38cfdf9b0c6c3bda97402c75bd27ef69a85/Documentation/ioctl/botching-up-ioctls.txt
I will go through and align to it. Thanks for sharing.

-Ram
> 
> Cheers, Daniel
> > +};
> > +
> >  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