diff mbox

[RFC,v2] drm/hdcp: drm enum property for HDCP State

Message ID 1500031303-11797-1-git-send-email-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ramalingam C July 14, 2017, 11:21 a.m. UTC 
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

V2: Redesigned the property to match with CP needs of CrOS.

Signed-off-by: Ramalingam C <ramalingam.c@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

Comments

Sean Paul July 14, 2017, 1:55 p.m. UTC | #1 
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.

> 
> V2: Redesigned the property to match with CP needs of CrOS.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@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.


> +};
> +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.

> +	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.

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
>
Ramalingam C July 21, 2017, 1:02 p.m. UTC | #2 
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.?
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?

As per the design I am proposing, SRM is passed to KMD and revocation 
check is done in kernel space.
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?

>
>> V2: Redesigned the property to match with CP needs of CrOS.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@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
>>
Sean Paul July 24, 2017, 1:23 p.m. UTC | #3 
On Fri, Jul 21, 2017 at 9:02 AM, Ramalingam C <ramalingam.c@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@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
>
>
Ramalingam C July 24, 2017, 1:34 p.m. UTC | #4 
On Monday 24 July 2017 06:53 PM, Sean Paul wrote:
> On Fri, Jul 21, 2017 at 9:02 AM, Ramalingam C <ramalingam.c@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
Thank you sean. This clears my doubts. I will share the reworked patch soon.

--Ram
>
>>
>>
>> V2: Redesigned the property to match with CP needs of CrOS.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@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
>>
>>
diff mbox

Patch

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" },
+};
+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));
+	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
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.
 	 */