[Intel-gfx] [RFC v2] drm/hdcp: drm enum property for HDCP State

Mon Jul 24 13:23:07 UTC 2017

On Fri, Jul 21, 2017 at 9:02 AM, Ramalingam C <ramalingam.c at intel.com> wrote:
> Sorry for late response.
>
>
> On Friday 14 July 2017 07:25 PM, Sean Paul wrote:
>
> On Fri, Jul 14, 2017 at 04:51:43PM +0530, Ramalingam C wrote:
>
> DRM connector property is created to represent the content protection
> state of the connector and to configure the same.
>
> CP States defined:
> DRM_CP_UNSUPPORTED - CP is not supported
> DRM_CP_DISABLE - CP is disabled
> DRM_CP_ENABLE - CP is enabled
>
> Why are we changing the names from the original version (that's used in
> CrOS)? It's not
> obvious what "CP" stands for when looking at the name.
>
> Sean,
>
> Considering that we want to test this interface for CrOS stack as a
> consumer, I will try to align the property names.
> Meanwhile got few questions with respect to designing this CP interface
> aligned with existing CrOS stack.
>
> I want to understand what all are the bare minimal content protection
> interface required for existing CrOS Userspace stack.
> Whether this drm enum property will be sufficient for CrOS Content
> protection needs of HDCP1.4.?

Yep, just the property. Userspace sets it to desired and polls it
until it's enabled. Here are the code pointers, I sent this to Marc
Herbert, so perhaps it's already gotten to you.

Threads to set/query hdcp state:
https://cs.chromium.org/chromium/src/ui/display/manager/chromeos/apply_content_protection_task.cc
https://cs.chromium.org/chromium/src/ui/display/manager/chromeos/query_content_protection_task.cc
Code which actually tweaks the drm props:
https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_display.cc

> Could you please help me on that direction?
>
> When i refer your RFC at
> https://lists.freedesktop.org/archives/dri-devel/2014-December/073418.html
> there is a drm range property called  "Content Protection KSV", claimed to
> stand for content protection state.
> Is this used by current CrOS userspace stack?
>

No, it's not.

> As per the design I am proposing, SRM is passed to KMD and revocation check
> is done in kernel space.

I'm not completely against this, however I'm more partial to having
userspace handle SRM/revocation since it's not really a hardware
feature.

> How is this done in CrOS? CrOS uses the above mentioned ksv property for
> that purpose, to pass the ksv to UMD for revocation check?

We don't have SRM or revocation lists, but that was the idea should we
have needed it.

Sean

>
>
>
> V2: Redesigned the property to match with CP needs of CrOS.
>
> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 14 +++++++++++++
>  include/drm/drm_hdcp.h          | 44
> +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_mode_config.h   |  5 +++++
>  3 files changed, 63 insertions(+)
>  create mode 100644 include/drm/drm_hdcp.h
>
> diff --git a/drivers/gpu/drm/drm_connector.c
> b/drivers/gpu/drm/drm_connector.c
> index 5cd61af..64895fb 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -24,6 +24,7 @@
>  #include <drm/drm_connector.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_hdcp.h>
>
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
> @@ -617,6 +618,13 @@ static const struct drm_prop_enum_list
> drm_link_status_enum_list[] = {
>  };
>  DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list)
>
> +static const struct drm_prop_enum_list drm_cp_enum_list[] = {
> + { DRM_CP_UNSUPPORTED, "CP Not Supported" },
> + { DRM_CP_DISABLE, "Disable CP" },
> + { DRM_CP_ENABLE, "Enable CP for Type0 content" },
>
> Type0 has no meaning in this case. The whole point of using "Content
> Protection"
> is to abstract the means of protection from userspace. The names are also
> much
> more verbose than they need be. "Unsupported", "Disabled", "Enabled" are
> fine.
>
> Looks like i was still trying to reflect that "Type1 is coming" ;). I will
> rename these in next revision. Thanks
>
>
> +};
> +DRM_ENUM_NAME_FN(drm_get_cp_status_name, drm_cp_enum_list)
> +
>  /**
>   * drm_display_info_set_bus_formats - set the supported bus formats
>   * @info: display info to store bus formats in
> @@ -789,6 +797,12 @@ int drm_connector_create_standard_properties(struct
> drm_device *dev)
>   return -ENOMEM;
>   dev->mode_config.link_status_property = prop;
>
> + prop = drm_property_create_enum(dev, 0, "cp", drm_cp_enum_list,
> +   ARRAY_SIZE(drm_cp_enum_list));
>
> Your property name here, on the other hand, is not descriptive enough.
> Please
> expand to something meaningful.
>
> Will change it to "content protection"
>
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.cp_property = prop;
> +
>   return 0;
>  }
>
> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
>
> Why create a new header for this? That seems a little excessive.
>
> But I am planning to use this header for all hdcp protocol specific
> definitions like HDCP2.2 messages and Panel's HDCP2.2 register definitions
> etc.
> With that I felt it justifies a header on it own.
> And this will grow further when multiple spec versions are supported in
> future.
>
> -Ram
>
>
> Sean
>
> new file mode 100644
> index 0000000..f6d0160
> --- /dev/null
> +++ b/include/drm/drm_hdcp.h
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> its
> + * documentation for any purpose is hereby granted without fee, provided
> that
> + * the above copyright notice appear in all copies and that both that
> copyright
> + * notice and this permission notice appear in supporting documentation,
> and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no representations
> + * about the suitability of this software for any purpose.  It is provided
> "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
> USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
> PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +/*
> + * Header for HDCP specific data
> + */
> +
> +#ifndef __DRM_HDCP_H__
> +#define __DRM_HDCP_H__
> +
> +/**
> + * CP property related information
> + */
> +enum drm_cp_state {
> +
> + /* HDCP sink and Src dont have any common HDCP ver supported */
> + DRM_CP_UNSUPPORTED,
> +
> + /* Disable Content Protection */
> + DRM_CP_DISABLE,
> +
> + /* Enable Content Protection */
> + DRM_CP_ENABLE,
> +};
> +#endif /* __DRM_HDCP_H__ */
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 4298171..7acb8b2 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -538,6 +538,11 @@ struct drm_mode_config {
>   */
>   struct drm_property *link_status_property;
>   /**
> + * @cp_property: Default connector property for CP
> + * of a connector
> + */
> + struct drm_property *cp_property;
> + /**
>   * @plane_type_property: Default plane property to differentiate
>   * CURSOR, PRIMARY and OVERLAY legacy uses of planes.
>   */
> --
> 2.7.4
>
>