On Wed, Mar 27, 2019 at 2:58 PM Ramalingam C <ramalingam.c@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > > ---
> > > 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?
$ make htmldocs
Look at the generated output. These kerneldoc comments are parsed as
rest/sphinx formatted text, and I'm pretty sure the above won't parse.
The plaintext as plaintext looks fine.
> >
> > > + *
> > > + * 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!!
32bit vs 64bit, depending upon architecture. Since this is core code,
it needs to work everywhere. igt is 64bit like the kernel (at least in
our CI, and probably on your machine too), that case always works.
It's mixed mode where userspace and the kernel might disagree on
alignment, and where therefore must align manually.
The other bit is that for uapi structs you must _never_ leak kernel
data out. It's pretty hard to avoid that if you have padding bytes you
can't even access because there's no member for them :-)
-Daniel
>
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
