On 2019-07-09 at 16:26:31 +0300, Pekka Paalanen wrote:
> On Mon, 8 Jul 2019 14:42:29 +0530
> Ramalingam C <ramalingam.c@xxxxxxxxx> wrote:
>
> > 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.
>
> Hi,
>
> if userspace asks for Type0, the kernel is free to use Type1 instead
> and switch "Content Protection" to "ENABLED". Userspace has no way of
> knowing that the link actually runs with Type1.
>
> You can argue that it does not matter, because Type1 protection is
> stronger than Type0, so everybody should be happy, but that does not
> change the fact that userspace thinks wrong of what the protection
> really is.
>
> > > - userspace cannot force Type0 if the driver succeeds enabling Type1
> > When you set Type 0, kernel will authenticate the link for Type 0 only.
>
> Really? Sorry, I have completely missed this. Please make it clear in
> UAPI and kAPI docs. This invalidates my comment above as well.
>
> But you are also conflicting with your own doc, which said:
>
> + * - "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).
>
> To me that reads as: if userspace asks for Type0, the kernel is free to
> use Type0 or Type1 or anything stronger to say it succeeded.
>
> Hence my confusion.
>
> > Not for Type 1. I guess you are trying to say HDCP1.4 is not possible
> > over HDCP2.2.
>
> I wouldn't know what that means.
>
> > 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.
>
> I'm running with the assumption that:
> - Type0 == HDCP 1.4
> - Type1 == HDCP 2.2
> and I have no idea why you are sometimes talking about Type and
> sometimes about HDCP version.
This is a wrong assumption. Type 0 is not attached to HDCP1.4. Even
HDCP2.2 support Type 0 along with Type 1. Thats why when Type 0 is
requested Type 0 through HDCP2.2 will be attempted first. When that
fails, we go for HDCP1.4(Type 0 only)
>
> Is there any reason to actually talk about HDCP versions at all in the
> UAPI doc? I'm starting to suspect that my assumptions are not right,
> and the use of dual-terminology in the UAPI doc confuses me.
In practical sense we dont need to mention the HDCP version in uAPI doc.
But I feel it is better to provide the understanding of the association
of the content type with HDCP version. May be we should try to avoid the
confusion :)
next version is closure in that direction i guess.
-Ram
>
> > 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.
>
> Awesome, let's see.
>
>
> Thanks,
> pq
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
