Re: [PATCH v7 07/11] drm: Add Content protection type property

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7105 bytes --]

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

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel