Re: [PATCH v8 1/6] drm: Add Content protection type property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux