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

[Ramalingam C]

On Monday 24 July 2017 06:53 PM, Sean Paul wrote:
> 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
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 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
>>
>>