On 2019-07-08 at 12:59:59 +0300, Pekka Paalanen wrote:
> On Mon, 8 Jul 2019 12:52:17 +0300
> Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote:
>
> > On Fri, 5 Jul 2019 06:16:37 +0530
> > Ramalingam C <ramalingam.c@xxxxxxxxx> 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.
> > > v6:
> > > Kernel docs are modified [pekka]
> > >
> > > Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > ---
> > > drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
> > > drivers/gpu/drm/drm_connector.c | 22 ++++++++++++++
> > > drivers/gpu/drm/drm_hdcp.c | 36 ++++++++++++++++++++++-
> > > drivers/gpu/drm/i915/display/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, 82 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index abe38bdf85ae..19ae119f1a5d 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -747,6 +747,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) {
> > > @@ -831,6 +833,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> > > state->hdr_output_metadata->base.id : 0;
> > > } 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 068d4b05f1be..17aef88c03a6 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -951,6 +951,28 @@ 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:
> > > + * - "HDCP Type0": DRM_MODE_HDCP_CONTENT_TYPE0 = 0
> > > + * HDCP Type0 streams can be transmitted on a link which is
> > > + * encrypted with HDCP 1.4 or higher versions of HDCP(i.e HDCP2.2
> > > + * and more).
> > > + * - "HDCP Type1": DRM_MODE_HDCP_CONTENT_TYPE1 = 1
> > > + * HDCP Type1 streams can be transmitted on a link which is
> > > + * encrypted only with HDCP 2.2. In future higher versions also
> > > + * might support Type1 based on their spec.
> > > + *
> > > + * Note that the HDCP Content Type property is introduced at HDCP 2.2, and
> > > + * defaults to type 0. It is only exposed by drivers supporting HDCP 2.2.
> > > + * Based on how next versions of HDCP specs are defined content Type could
> > > + * be used for higher versions too.
> > > + * 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
> >
> > Hi,
> >
> > I think this doc covers all my previous comments on this patch now. One
> > more thing, the wording here reads as:
> > - userspace configures the content type
> > - the kernel transmits the content if the link satisfies the type
> > requirement
> > - if the link does not satisfy the type requirement, the kernel will
> > not transmit the content
> >
> > This is of course false, the kernel transmits anyway, but that is how
> > the text reads from the "stream's content type" and "can be transmitted
> > on". The problem is, that a userspace developer will think the stream
> > is what he pushes into KMS, not what goes on the wire. The text also
> > magnifies that misconception because it sounds like the stream is
> > something different from the link. Actually, I don't understand what
> > "the stream" is supposed to be here.
Stream is nothing but the any display content that needs the hdcp
protection.
> >
> > Instead, I think you should explain how the content type property
> > guides the kernel driver's attempts in negotiating the link encryption
> > and how that then reflects in the content protection property (DESIRED
> > vs. ENABLED). It might be best to not say anything about any "stream".
I will explain in different and more words, so that this questions wont
raise.
>
> Btw. this UAPI has the following fundamental flaws:
>
> - userspace cannot know which encryption is used on the link
This claim is not true.
"HDCP content type" and "content protection" are used
together in
requesting the HDCP state
confirming that required state is achieved
For ex:
The state "HDCP content type" = "HDCP Type1" and "content protection" =
"DESIRED" stands for userspace has requested for HDCP Type 1 protection
from kernel.
When kernel changes the "content protection" to "ENABLED" when "HDCP
content type" is "HDCP Type1", kernel indicates the userspace that HDCP
authentication for Type 1 is successful.
I am not sure why do you think that userspace is not knowing link's
authentication state w.r.t type.
> - userspace cannot force Type0 if the driver succeeds enabling Type1
When you set Type 0, kernel will authenticate the link for Type 0 only.
Not for Type 1. I guess you are trying to say HDCP1.4 is not possible
over HDCP2.2.
I dont see any reason why anyone want this except for testing.
Type 0 can be achieved through HDCP2.2 and HDCP1.4. And HDCP2.2 is attempted
first and HDCP1.4 will be attempted only if HDCP2.2 is failed for some
reasons. This is done because HDCP2.2 is preferred over 1.4 due to its
better crypto algo.
Thanks for pointing it out. There is a scope to add all these
information in the documentation. Which I will do it in the next version
today.
Thanks,
-Ram.
>
> To me this seems like poor UAPI design. Why was this designed like this?
>
>
> Thanks,
> pq
>
> > > *
> > > * HDR_OUTPUT_METADATA:
> > > * Connector property to enable userspace to send HDR Metadata to
> > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
> > > index cd837bd409f7..ce235fd1c844 100644
> > > --- a/drivers/gpu/drm/drm_hdcp.c
> > > +++ b/drivers/gpu/drm/drm_hdcp.c
> > > @@ -344,23 +344,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 =
> > > @@ -377,6 +395,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/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > index bc3a94d491c4..2a4d10952b74 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/display/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 4c30d751487a..d6432967a2a9 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -601,6 +601,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.
> > > @@ -1484,6 +1490,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 759d462d028b..6c4b5bc85951 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -849,6 +849,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 5ab331e5dc23..5c954394093f 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -218,6 +218,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;
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
