On Fri, Jul 21, 2017 at 9:02 AM, Ramalingam C <ramalingam.c@xxxxxxxxx> 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@xxxxxxxxx> > --- > 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 > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
