| Message ID | 20190507162745.25600-8-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | HDCP2.2 Phase II | expand |
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
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). > + * 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. > + * - 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. > + * > + * 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? > + * 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. *** Questions about the already existing "Content Protection" property: - What happens if userspace attempts to write "Enabled" into it? - 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
On 2019-07-05 at 16:00:37 +0300, Pekka Paalanen wrote: > On Thu, 4 Jul 2019 16:06:05 +0530 > Ramalingam C <ramalingam.c@intel.com> wrote: > > > 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. > > Hi, > > I don't really care what kernel internal APIs use, this may well be > correct for them, but the UAPI uses strings. > > Because the kernel internal and UAPI docs are mixed up, it will be hard > to write proper docs. I guess can't help it this time. > > It would be really good if the enum value strings were explicitly > presented in the docs, so userspace has something to hook on. Or if not > in docs, in the UAPI header, see further below. > > I do see the strings in the docs you wrote, but nothing really > highlights them as the literal strings to be used in the API. Even just > quotes "" around them would make them more discoverable, especially > "HDCP Type0" etc. In the next version I have added the string too in the kernel docuemntation. But when we read the property state we read the enum value which is matched agaist the string based on the enum property definition. So I feel we should have both detail matched against in the uAPI doc. > > > > > > > > + * 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. > > Right. All my three notes above are about future-proofing this for the > "trivial" case of adding one more content type. I think it would be > wise to do, because the cost is very low (no changes in code, just > trivial change in spec), and even if the likelyhood of adding more > types in future might be low, it's not zero. > > I think the Weston implementation under review in MR !48 is > future-proof as well, in the sense that enum values it does not know > will be interpreted as Type0. > > I suppose it is a reasonable assumption that HDCP spec will not add a > new type that would not be at least as secure as Type0, right? As per my understanding, HDCP spec will add new type if there is a betterment in next version of HDCP and hence userspace want to mandate only that spec for it's content. Say Content Type 2 which can be rendered only on HDCP3.x. Hopefully next version of this patch has the futureproofing you are looking for. -Ram > > > > > > > > + * 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. > > I suspect this is just the cycle of cargo-culting mistakes. Weston > MR !48 does not use these defines either. > > Unfortunately the bad examples that are already in the UAPI headers > cannot be removed. > > However, I think you could start a new good practice: > > #define DRM_MODE_HDCP_CONTENT_TYPE0_STR "HDCP Type0" > #define DRM_MODE_HDCP_CONTENT_TYPE1_STR "HDCP Type1" > > It would make a lot of sense for the UAPI header to spell out the > strings that userspace must be using. Even if userspace wouldn't use > these defines, they would have strong documentary value. > > Danvet, would you agree with me here? > > > > > > > *** > > > > > > 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 > > Good, I glossed over the last sentence when reading this before. > > > Thanks, > pq > > > > > -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 > > > > >
On Thu, 4 Jul 2019 16:06:05 +0530 Ramalingam C <ramalingam.c@intel.com> wrote: > 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. Hi, I don't really care what kernel internal APIs use, this may well be correct for them, but the UAPI uses strings. Because the kernel internal and UAPI docs are mixed up, it will be hard to write proper docs. I guess can't help it this time. It would be really good if the enum value strings were explicitly presented in the docs, so userspace has something to hook on. Or if not in docs, in the UAPI header, see further below. I do see the strings in the docs you wrote, but nothing really highlights them as the literal strings to be used in the API. Even just quotes "" around them would make them more discoverable, especially "HDCP Type0" etc. > > > > > + * 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. Right. All my three notes above are about future-proofing this for the "trivial" case of adding one more content type. I think it would be wise to do, because the cost is very low (no changes in code, just trivial change in spec), and even if the likelyhood of adding more types in future might be low, it's not zero. I think the Weston implementation under review in MR !48 is future-proof as well, in the sense that enum values it does not know will be interpreted as Type0. I suppose it is a reasonable assumption that HDCP spec will not add a new type that would not be at least as secure as Type0, right? > > > > > + * 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. I suspect this is just the cycle of cargo-culting mistakes. Weston MR !48 does not use these defines either. Unfortunately the bad examples that are already in the UAPI headers cannot be removed. However, I think you could start a new good practice: #define DRM_MODE_HDCP_CONTENT_TYPE0_STR "HDCP Type0" #define DRM_MODE_HDCP_CONTENT_TYPE1_STR "HDCP Type1" It would make a lot of sense for the UAPI header to spell out the strings that userspace must be using. Even if userspace wouldn't use these defines, they would have strong documentary value. Danvet, would you agree with me here? > > > > *** > > > > 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 Good, I glossed over the last sentence when reading this before. Thanks, pq > > -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 > >
On Fri, 5 Jul 2019 12:03:18 +0530 Ramalingam C <ramalingam.c@intel.com> wrote: > On 2019-07-05 at 16:00:37 +0300, Pekka Paalanen wrote: > > On Thu, 4 Jul 2019 16:06:05 +0530 > > Ramalingam C <ramalingam.c@intel.com> wrote: > > > > > 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. > > > > Hi, > > > > I don't really care what kernel internal APIs use, this may well be > > correct for them, but the UAPI uses strings. > > > > Because the kernel internal and UAPI docs are mixed up, it will be hard > > to write proper docs. I guess can't help it this time. > > > > It would be really good if the enum value strings were explicitly > > presented in the docs, so userspace has something to hook on. Or if not > > in docs, in the UAPI header, see further below. > > > > I do see the strings in the docs you wrote, but nothing really > > highlights them as the literal strings to be used in the API. Even just > > quotes "" around them would make them more discoverable, especially > > "HDCP Type0" etc. > In the next version I have added the string too in the kernel > docuemntation. > > But when we read the property state we read the enum > value which is matched agaist the string based on the enum property > definition. So I feel we should have both detail matched against in the uAPI > doc. Hi, I guess so. When userspace initializes, it will ask the kernel for the kernel integers corresponding to all of the name strings they both know about. When userspace sets an enum property to a value, it looks up the kernel integer from an internal name for the enum value and submits that to the kernel. When userspace reads an enum property, it receives a kernel integer, which it looks up in its value table to translate it into an internal name which it can handle. At no point is the kernel integer definition needed outside of the kernel. It is discovered through the UAPI. So from UAPI perspective, only the name string is appropriate. From kernel internal API perspective, I suppose the integer is appropriate. Another unfortunate confusion caused by mixed internal and UAPI docs. Thanks, pq
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 @@ -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 + * 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. + * + * 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 + * 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 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/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 53df2f2376e8..be6c81addca0 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -1829,7 +1829,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 e257b879b72c..65005240c561 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -560,6 +560,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. @@ -1330,6 +1336,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); diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 13771a496e2b..2970abdfaf12 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -291,5 +291,5 @@ struct drm_connector; bool drm_hdcp_check_ksvs_revoked(struct drm_device *dev, u8 *ksvs, u32 ksv_count); int drm_connector_attach_content_protection_property( - struct drm_connector *connector); + struct drm_connector *connector, bool hdcp_content_type); #endif 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 + struct drm_mode_modeinfo { __u32 clock; __u16 hdisplay;