On Tue,  7 May 2019 21:57:41 +0530
Ramalingam C <ramalinga...@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 <ramalinga...@intel.com>
> Reviewed-by: Daniel Vetter <daniel.vet...@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

Attachment: pgpNI0kyBUlhJ.pgp
Description: OpenPGP digital signature

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

Reply via email to