On Thu, Apr 18, 2019 at 02:27:54PM +0530, Ramalingam C wrote:

Content protection property is created once and stored in drm_mode_config. And attached to all HDCP capable connectors.

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 | 13 +++++++------ include/drm/drm_connector.h | 6 ------ include/drm/drm_mode_config.h | 6 ++++++ 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index ea797d4c82ee..002dcede7915 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -727,7 +727,7 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, state->content_type = val; } else if (property == connector->scaling_mode_property) { state->scaling_mode = val;

• } else if (property == connector->content_protection_property) {

• } else if (property == config->content_protection_property) { if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) { DRM_DEBUG_KMS("only drivers can set CP Enabled\n"); return -EINVAL;

@@ -807,7 +807,7 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->colorspace; } else if (property == connector->scaling_mode_property) { *val = state->scaling_mode;

• } else if (property == connector->content_protection_property) {

• } else if (property == config->content_protection_property) { *val = state->content_protection; } else if (property == config->writeback_fb_id_property) { /* Writeback framebuffer is one-shot, write and forget */

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 2355124849db..7c0eda9cca60 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1534,18 +1534,19 @@ int drm_connector_attach_content_protection_property( struct drm_connector *connector) { struct drm_device *dev = connector->dev;

• struct drm_property *prop;

• struct drm_property *prop =

• 	dev->mode_config.content_protection_property;
  
  

• prop = drm_property_create_enum(dev, 0, "Content Protection",

• 			drm_cp_enum_list,
  

• 			ARRAY_SIZE(drm_cp_enum_list));
  

• if (!prop)

• prop = drm_property_create_enum(dev, 0, "Content Protection",
  

• 				drm_cp_enum_list,
  

• 				ARRAY_SIZE(drm_cp_enum_list));
  

  if (!prop) return -ENOMEM;

  drm_object_attach_property(&connector->base, prop, DRM_MODE_CONTENT_PROTECTION_UNDESIRED);

• connector->content_protection_property = prop;

• dev->mode_config.content_protection_property = prop;

  return 0;

} diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 02a131202add..5e41942e5679 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1061,12 +1061,6 @@ struct drm_connector { */ struct drm_property *vrr_capable_property;

• /**

• * @content_protection_property: DRM ENUM property for content
  

• * protection. See drm_connector_attach_content_protection_property().
  

• */
  

• struct drm_property *content_protection_property;
• /**
  • @colorspace_property: Connector property to set the suitable
  • colorspace supported by the sink.

diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 7f60e8eb269a..5764ee3c7453 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -836,6 +836,12 @@ struct drm_mode_config { */ struct drm_property *writeback_out_fence_ptr_property;

• /**

• * @content_protection_property: DRM ENUM property for content
  

• * protection. See drm_connector_attach_content_protection_property().
  

• */
  

• struct drm_property *content_protection_property;
• /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow;

-- 2.19.1

On 2019-04-23 at 10:11:48 +0200, Daniel Vetter wrote:

On Thu, Apr 18, 2019 at 02:27:57PM +0530, Ramalingam C wrote:

...

Attaches the content type property for HDCP2.2 capable connectors.

Implements the update of content type from property and apply the restriction on HDCP version selection.

v2: s/cp_content_type/content_protection_type [daniel] disable at hdcp_atomic_check to avoid check at atomic_set_property [Maarten] v3: s/content_protection_type/hdcp_content_type [Pekka] v4: hdcp disable incase of type change is moved into commit [daniel].

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/i915/intel_ddi.c | 38 ++++++++++++++++++++++++--- drivers/gpu/drm/i915/intel_hdcp.c | 43 ++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_hdcp.h | 2 +- 3 files changed, 63 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 24f9106efcc6..dd9bea840937 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3476,7 +3476,8 @@ static void intel_enable_ddi(struct intel_encoder *encoder, /* Enable hdcp if it's desired */ if (conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED)

• intel_hdcp_enable(to_intel_connector(conn_state->connector));
  

• intel_hdcp_enable(to_intel_connector(conn_state->connector),
  

• 		  (u8)conn_state->hdcp_content_type);
  

}

static void intel_disable_ddi_dp(struct intel_encoder *encoder, @@ -3545,15 +3546,44 @@ static void intel_ddi_update_pipe(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state, const struct drm_connector_state *conn_state) {

• struct intel_connector *connector =

• 		to_intel_connector(conn_state->connector);
  

• struct intel_hdcp *hdcp = &connector->hdcp;

• bool re_enable_hdcp = false;

• int ret = -EINVAL;

• if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) intel_ddi_update_pipe_dp(encoder, crtc_state, conn_state);

• /*

• * During the HDCP encryption session if Type change is requested,
  

• * disable the HDCP and reenable it with new TYPE value.
  

• */
  

  if (conn_state->content_protection ==

•    DRM_MODE_CONTENT_PROTECTION_DESIRED)
  

• intel_hdcp_enable(to_intel_connector(conn_state->connector));
  

•    DRM_MODE_CONTENT_PROTECTION_ENABLED &&
  

•    conn_state->hdcp_content_type != hdcp->content_type) {
  

• intel_hdcp_disable(to_intel_connector(conn_state->connector));
  

• re_enable_hdcp = true;
  

• }
• if (conn_state->content_protection ==

•    DRM_MODE_CONTENT_PROTECTION_DESIRED || re_enable_hdcp)
  

• ret = intel_hdcp_enable(connector,
  

• 			(u8)conn_state->hdcp_content_type);
  

  else if (conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)

• intel_hdcp_disable(to_intel_connector(conn_state->connector));
  

• intel_hdcp_disable(connector);
  

• /*

• * During Type change handling re-enabling of the HDCP failed. Hence
  

• * changes state as ENABLED->DESIRED.
  

• */
  

• if (ret && re_enable_hdcp) {

• mutex_lock(&hdcp->mutex);
  

• hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
  

• schedule_work(&hdcp->prop_work);
  

• mutex_unlock(&hdcp->mutex);
  

• }

I'm unhappy with the convoluted logic here. It's correct I think but fairly hard to follow. What about this instead?

unsigned int content_protection = conn_state->content_protection; bool content_protection_type_changed = conn_state->hdcp_content_type != hdcp->content_type;

if (conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_UNDESIRED || content_protection_type_changed) intel_hdcp_disable(to_intel_connector(conn_state->connector));

/* Make sure we pick up type changes */ if (content_protection_type_changed) { mutex_lock(&hdcp->mutex); hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED; schedule_work(&hdcp->prop_work); mutex_unlock(&hdcp->mutex); }

if (conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED || content_protection_type_changed) intel_hdcp_enable(to_intel_connector(conn_state->connector));

Idea is to only have one disable and one enable call, and make sure the enable picks up our content type change no matter what. But not sure that it's really clearer. Thoughts?

Sure. More simplified. I will add your reviewed-by with this change to the patch. Thanks.

- Ram

Aside from this lgtm. -Daniel

...

}

static void intel_ddi_set_fia_lane_count(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index a61391d374e8..772974697b5a 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -1717,14 +1717,15 @@ static const struct component_ops i915_hdcp_component_ops = { .unbind = i915_hdcp_component_unbind, };

-static inline int initialize_hdcp_port_data(struct intel_connector *connector) +static inline int initialize_hdcp_port_data(struct intel_connector *connector,

• 			    const struct intel_hdcp_shim *shim)
  

{ struct intel_hdcp *hdcp = &connector->hdcp; struct hdcp_port_data *data = &hdcp->port_data;

data->port = connector->encoder->port; data->port_type = (u8)HDCP_PORT_TYPE_INTEGRATED;

• data->protocol = (u8)hdcp->shim->protocol;

• data->protocol = (u8)shim->protocol;

  data->k = 1; if (!data->streams)

@@ -1774,12 +1775,13 @@ void intel_hdcp_component_init(struct drm_i915_private *dev_priv) } }

-static void intel_hdcp2_init(struct intel_connector *connector) +static void intel_hdcp2_init(struct intel_connector *connector,

• 	     const struct intel_hdcp_shim *shim)
  

{ struct intel_hdcp *hdcp = &connector->hdcp; int ret;

• ret = initialize_hdcp_port_data(connector);

• ret = initialize_hdcp_port_data(connector, shim); if (ret) { DRM_DEBUG_KMS("Mei hdcp data init failed\n"); return;

@@ -1798,25 +1800,28 @@ int intel_hdcp_init(struct intel_connector *connector, if (!shim) return -EINVAL;

• if (is_hdcp2_supported(dev_priv))

• intel_hdcp2_init(connector, shim);
  

• ret = drm_connector_attach_content_protection_property(&connector->base,

• 					 false);
  

• if (ret)

• 					 hdcp->hdcp2_supported);
  

• if (ret) {

• hdcp->hdcp2_supported = false;
  

• kfree(hdcp->port_data.streams);
  

  return ret;

• }

  hdcp->shim = shim; mutex_init(&hdcp->mutex); INIT_DELAYED_WORK(&hdcp->check_work, intel_hdcp_check_work); INIT_WORK(&hdcp->prop_work, intel_hdcp_prop_work);

• if (is_hdcp2_supported(dev_priv))

• intel_hdcp2_init(connector);
  

  init_waitqueue_head(&hdcp->cp_irq_queue);

  return 0;

}

-int intel_hdcp_enable(struct intel_connector *connector) +int intel_hdcp_enable(struct intel_connector *connector, u8 content_type) { struct intel_hdcp *hdcp = &connector->hdcp; unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS; @@ -1827,6 +1832,7 @@ int intel_hdcp_enable(struct intel_connector *connector)

mutex_lock(&hdcp->mutex); WARN_ON(hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);

• hdcp->content_type = content_type;

  /*

  • Considering that HDCP2.2 is more secure than HDCP1.4, If the setup

@@ -1838,8 +1844,12 @@ int intel_hdcp_enable(struct intel_connector *connector) check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS; }

• /* When HDCP2.2 fails, HDCP1.4 will be attempted */
• if (ret && intel_hdcp_capable(connector)) {

• /*

• * When HDCP2.2 fails and Content Type is not Type1, HDCP1.4 will
  

• * be attempted.
  

• */
  

• if (ret && intel_hdcp_capable(connector) &&

•    hdcp->content_type != DRM_MODE_HDCP_CONTENT_TYPE1) {
  

  ret = _intel_hdcp_enable(connector); }

@@ -1921,12 +1931,15 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,

/* * Nothing to do if the state didn't change, or HDCP was activated since

• * the last commit
  

• * the last commit. And also no change in hdcp content type.
  

  */ if (old_cp == new_cp || (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&

•     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED))
  

• return;
  

•     new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)) {
  

• if (old_state->hdcp_content_type ==
  

• 		new_state->hdcp_content_type)
  

• 	return;
  

• }

  crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc);

diff --git a/drivers/gpu/drm/i915/intel_hdcp.h b/drivers/gpu/drm/i915/intel_hdcp.h index be8da85c866a..13555b054930 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.h +++ b/drivers/gpu/drm/i915/intel_hdcp.h @@ -21,7 +21,7 @@ void intel_hdcp_atomic_check(struct drm_connector *connector, struct drm_connector_state *new_state); int intel_hdcp_init(struct intel_connector *connector, const struct intel_hdcp_shim *hdcp_shim); -int intel_hdcp_enable(struct intel_connector *connector); +int intel_hdcp_enable(struct intel_connector *connector, u8 content_type); int intel_hdcp_disable(struct intel_connector *connector); bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port); bool intel_hdcp_capable(struct intel_connector *connector); -- 2.19.1

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Mon, Apr 29, 2019 at 09:26:07AM +0200, Daniel Vetter wrote:

On Thu, Apr 18, 2019 at 02:27:58PM +0530, Ramalingam C wrote:

...

On every hdcp revocation check request SRM is read from fw file /lib/firmware/display_hdcp_srm.bin

SRM table is parsed and stored at drm_hdcp.c, with functions exported for the services for revocation check from drivers (which implements the HDCP authentication)

This patch handles the HDCP1.4 and 2.2 versions of SRM table.

v2: moved the uAPI to request_firmware_direct() [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Suggested-by: Daniel Vetter daniel@ffwll.ch

---

drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/drm_hdcp.c | 336 +++++++++++++++++++++++++++++++++

Please add an include stanza for this new file to Documentation/gpu/drm-kms-helpers.rst, somewhere near the other dp/hdcp related sink helpers.

...

drivers/gpu/drm/drm_internal.h | 4 + drivers/gpu/drm/drm_sysfs.c | 2 + include/drm/drm_hdcp.h | 36 ++++ 5 files changed, 379 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_hdcp.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 3d0c75cd687c..fe8400af2426 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -19,7 +19,7 @@ drm-y := drm_auth.o drm_bufs.o drm_cache.o \ drm_plane.o drm_color_mgmt.o drm_print.o \ drm_dumb_buffers.o drm_mode_config.o drm_vblank.o \ drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \

• drm_atomic_uapi.o
  

• drm_atomic_uapi.o drm_hdcp.o
  
  

drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o drm-$(CONFIG_DRM_VM) += drm_vm.o diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c new file mode 100644 index 000000000000..78b043c8195e --- /dev/null +++ b/drivers/gpu/drm/drm_hdcp.c @@ -0,0 +1,336 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 Intel Corporation.
• • Authors:
• • Ramalingam C ramalingam.c@intel.com
• */

+#include <linux/device.h> +#include <linux/err.h> +#include <linux/gfp.h> +#include <linux/export.h> +#include <linux/slab.h> +#include <linux/firmware.h>

+#include <drm/drm_hdcp.h> +#include <drm/drm_sysfs.h> +#include <drm/drm_print.h> +#include <drm/drm_device.h>

+struct hdcp_srm {

• u8 *srm_buf;
• size_t received_srm_sz;
• u32 revocated_ksv_cnt;
• u8 *revocated_ksv_list;

So not sure this is from the standard or not, but my dictionary says the verb is revoke, not revocate. Please change everywhere from revocated to revoked.

So drm_hdcp_check_ksvs_revoked() for the one exported function. -Daniel

...

• /* Mutex to protect above struct member */
• struct mutex mutex;

+} *srm_data;

+static inline void drm_hdcp_print_ksv(const char *ksv) +{

• DRM_DEBUG("\t%#04x, %#04x, %#04x, %#04x, %#04x\n", *ksv & 0xff,

Why 04? 8 bit only needs 02 I think.

...

•   *(ksv + 1) & 0xff, *(ksv + 2) & 0xff, *(ksv + 3) & 0xff,
  

•   *(ksv + 4) & 0xff);
  

The 0xff is redundant, char is always only 8 bits. You could also simplify the array derefe using ksv[0], ksv[1], ... it's the same expression.

Another one: There's a bit a confusion between const char * and u8* for parsing the buffer. I think it'd be good to standardize on const u8* for everything. I think that should also remove the need for 0xff completely, because no more sign extensions to the full int.

...

+}

+static u32 drm_hdcp_get_revocated_ksv_count(const char *buf, u32 vrls_length) +{

• u32 parsed_bytes = 0, ksv_count = 0, vrl_ksv_cnt, vrl_sz;
• do {

• vrl_ksv_cnt = *buf;
  

• ksv_count += vrl_ksv_cnt;
  

• vrl_sz = (vrl_ksv_cnt * DRM_HDCP_KSV_LEN) + 1;
  

• buf += vrl_sz;
  

• parsed_bytes += vrl_sz;
  

• } while (parsed_bytes < vrls_length);

Hm, if we overflow here (i.e. parsed_bytes > vrls_lenght) then we return garbage, since we already incremented ksv_count. Plus there's no error checking of this. Kernel shouldn't trust root this much, and if the vrls_length and our ksv parsing don't agree, we should abort the srm load. So maybe switch the return value to int, and on error return -EINVAL and abort?

...

• return ksv_count;

+}

+static u32 drm_hdcp_get_revocated_ksvs(const char *buf, u8 *revocated_ksv_list,

• 		       u32 vrls_length)
  

This function here doesn't need to be paranoid, since we already checked the SRM by this point.

...

+{

• u32 parsed_bytes = 0, ksv_count = 0;
• u32 vrl_ksv_cnt, vrl_ksv_sz, vrl_idx = 0;
• do {

• vrl_ksv_cnt = *buf;
  

• vrl_ksv_sz = vrl_ksv_cnt * DRM_HDCP_KSV_LEN;
  

• buf++;
  

• DRM_DEBUG("vrl: %d, Revoked KSVs: %d\n", vrl_idx++,
  

• 	  vrl_ksv_cnt);
  

• memcpy(revocated_ksv_list, buf, vrl_ksv_sz);
  

• ksv_count += vrl_ksv_cnt;
  

• revocated_ksv_list += vrl_ksv_sz;
  

• buf += vrl_ksv_sz;
  

• parsed_bytes += (vrl_ksv_sz + 1);
  

• } while (parsed_bytes < vrls_length);
• return ksv_count;

+}

+static int drm_hdcp_parse_hdcp1_srm(const char *buf, size_t count) +{

• struct hdcp_srm_header *header;
• u32 vrl_length, ksv_count;
• if (count < (sizeof(struct hdcp_srm_header) +

•    DRM_HDCP_1_4_VRL_LENGTH_SIZE + DRM_HDCP_1_4_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length\n");
  

• return -EINVAL;
  

• }
• header = (struct hdcp_srm_header *)buf;
• mutex_lock(&srm_data->mutex);
• DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",

•   header->spec_indicator.srm_id,
  

•   __swab16(header->srm_version), header->srm_gen_no);
  

• WARN_ON(header->spec_indicator.reserved_hi ||

• header->spec_indicator.reserved_lo);
  

• if (header->spec_indicator.srm_id != DRM_HDCP_1_4_SRM_ID) {

• DRM_ERROR("Invalid srm_id\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• buf = buf + sizeof(*header);
• vrl_length = (*buf << 16 | *(buf + 1) << 8 | *(buf + 2));

Maybe a static inline to implement this and use in the hdcp2 version below too. Also I think buf[0], buf[1] is easier to read (as a bikeshed again).

...

• if (count < (sizeof(struct hdcp_srm_header) + vrl_length) ||

•    vrl_length < (DRM_HDCP_1_4_VRL_LENGTH_SIZE +
  

• 	  DRM_HDCP_1_4_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length or vrl length\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• /* Length of the all vrls combined */
• vrl_length -= (DRM_HDCP_1_4_VRL_LENGTH_SIZE +

•        DRM_HDCP_1_4_DCP_SIG_SIZE);
  

• if (!vrl_length) {

• DRM_ERROR("No vrl found\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• buf += DRM_HDCP_1_4_VRL_LENGTH_SIZE;
• ksv_count = drm_hdcp_get_revocated_ksv_count(buf, vrl_length);
• if (!ksv_count) {

• DRM_DEBUG("Revocated KSV count is 0\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return count;
  

• }
• kfree(srm_data->revocated_ksv_list);
• srm_data->revocated_ksv_list = kzalloc(ksv_count * DRM_HDCP_KSV_LEN,

• 			       GFP_KERNEL);
  

kcalloc()

...

• if (!srm_data->revocated_ksv_list) {

• DRM_ERROR("Out of Memory\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -ENOMEM;
  

• }
• if (drm_hdcp_get_revocated_ksvs(buf, srm_data->revocated_ksv_list,

• 			vrl_length) != ksv_count) {
  

• srm_data->revocated_ksv_cnt = 0;
  

• kfree(srm_data->revocated_ksv_list);
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• srm_data->revocated_ksv_cnt = ksv_count;
• mutex_unlock(&srm_data->mutex);
• return count;

+}

+static int drm_hdcp_parse_hdcp2_srm(const char *buf, size_t count) +{

• struct hdcp2_srm_header *header;
• u32 vrl_length, ksv_count, ksv_sz;
• mutex_lock(&srm_data->mutex);
• if (count < (sizeof(struct hdcp2_srm_header) +

•    DRM_HDCP_2_VRL_LENGTH_SIZE + DRM_HDCP_2_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• header = (struct hdcp2_srm_header *)buf;
• DRM_DEBUG("SRM ID: 0x%x, SRM Ver: 0x%x, SRM Gen No: 0x%x\n",

•   header->spec_indicator.srm_id,
  

•   __swab16(header->srm_version), header->srm_gen_no);
  

• if (header->spec_indicator.reserved)

• return -EINVAL;
  

• buf = buf + sizeof(*header);
• vrl_length = (*buf << 16 | *(buf + 1) << 8 | *(buf + 2));
• if (count < (sizeof(struct hdcp2_srm_header) + vrl_length) ||

•    vrl_length < (DRM_HDCP_2_VRL_LENGTH_SIZE +
  

•    DRM_HDCP_2_DCP_SIG_SIZE)) {
  

• DRM_ERROR("Invalid blob length or vrl length\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• /* Length of the all vrls combined */
• vrl_length -= (DRM_HDCP_2_VRL_LENGTH_SIZE +

•        DRM_HDCP_2_DCP_SIG_SIZE);
  

• if (!vrl_length) {

• DRM_ERROR("No vrl found\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -EINVAL;
  

• }
• buf += DRM_HDCP_2_VRL_LENGTH_SIZE;
• ksv_count = (*buf << 2) | DRM_HDCP_2_KSV_COUNT_2_LSBITS(*(buf + 1));
• if (!ksv_count) {

• DRM_DEBUG("Revocated KSV count is 0\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return count;
  

• }
• kfree(srm_data->revocated_ksv_list);
• srm_data->revocated_ksv_list = kzalloc(ksv_count * DRM_HDCP_KSV_LEN,

• 			       GFP_KERNEL);
  

• if (!srm_data->revocated_ksv_list) {

• DRM_ERROR("Out of Memory\n");
  

• mutex_unlock(&srm_data->mutex);
  

• return -ENOMEM;
  

• }
• ksv_sz = ksv_count * DRM_HDCP_KSV_LEN;
• buf += DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ;
• DRM_DEBUG("Revoked KSVs: %d\n", ksv_count);
• memcpy(srm_data->revocated_ksv_list, buf, ksv_sz);
• srm_data->revocated_ksv_cnt = ksv_count;
• mutex_unlock(&srm_data->mutex);
• return count;

+}

+static inline bool is_srm_version_hdcp1(const char *buf) +{

• return ((u8)*buf) == DRM_HDCP_1_4_SRM_ID << 4;

+}

+static inline bool is_srm_version_hdcp2(const char *buf) +{

• return ((u8)*buf) == (DRM_HDCP_2_SRM_ID << 4 |

• 	     DRM_HDCP_2_INDICATOR);
  

+}

+static ssize_t drm_hdcp_srm_update(const char *buf, size_t count) +{

• if (is_srm_version_hdcp1(buf))

• return (ssize_t)drm_hdcp_parse_hdcp1_srm(buf, count);
  

• else if (is_srm_version_hdcp2(buf))

• return (ssize_t)drm_hdcp_parse_hdcp2_srm(buf, count);
  

• return (ssize_t)-EINVAL;

+}

+void drm_hdcp_request_srm(struct drm_device *drm_dev) +{

• char fw_name[36] = "display_hdcp_srm.bin";
• const struct firmware *fw;
• int ret;
• ret = request_firmware_direct(&fw, (const char *)fw_name,

• 		      drm_dev->dev);
  

We need an ack from Matt Roper or someont else from iotg on this.

...

• if (ret < 0)

• goto exit;
  

• if (fw->size && fw->data)

• drm_hdcp_srm_update((const char *)fw->data, fw->size);
  

+exit:

• release_firmware(fw);

+}

+/* Check if any of the KSV is revocated by DCP LLC through SRM table */ +bool drm_hdcp_ksvs_revocated(struct drm_device *drm_dev, u8 *ksvs,

Since comment says "check", maybe put that into the function name? I like to have verbs in function names that do something (instead of simple helper functions to extract a computation to make the code a bit more readable).

Also needs some nice kerneldoc.

...

• 	     u32 ksv_count)
  

+{

• u32 rev_ksv_cnt, cnt, i, j;
• u8 *rev_ksv_list;
• if (!srm_data)

• return false;
  

• drm_hdcp_request_srm(drm_dev);
• mutex_lock(&srm_data->mutex);

I'd have wrapped the locking around the entire function, should simplify things a lot. Or does that anger lockdep when we call into request_firmware()?

...

• rev_ksv_cnt = srm_data->revocated_ksv_cnt;
• rev_ksv_list = srm_data->revocated_ksv_list;
• /* If the Revocated ksv list is empty */
• if (!rev_ksv_cnt || !rev_ksv_list) {

• mutex_unlock(&srm_data->mutex);
  

• return false;
  

• }
• for (cnt = 0; cnt < ksv_count; cnt++) {

• rev_ksv_list = srm_data->revocated_ksv_list;
  

• for (i = 0; i < rev_ksv_cnt; i++) {
  

• 	for (j = 0; j < DRM_HDCP_KSV_LEN; j++)
  

• 		if (*(ksvs + j) != *(rev_ksv_list + j)) {
  

• 			break;
  

• 		} else if (j == (DRM_HDCP_KSV_LEN - 1)) {
  

• 			DRM_DEBUG("Revocated KSV is ");
  

• 			drm_hdcp_print_ksv(ksvs);
  

• 			mutex_unlock(&srm_data->mutex);
  

• 			return true;
  

• 		}
  

• 	/* Move the offset to next KSV in the revocated list */
  

• 	rev_ksv_list += DRM_HDCP_KSV_LEN;
  

• }
  

• /* Iterate to next ksv_offset */
  

• ksvs += DRM_HDCP_KSV_LEN;
  

• }
• mutex_unlock(&srm_data->mutex);
• return false;

+} +EXPORT_SYMBOL_GPL(drm_hdcp_ksvs_revocated);

+int drm_setup_hdcp_srm(struct class *drm_class) +{

• srm_data = kzalloc(sizeof(*srm_data), GFP_KERNEL);
• if (!srm_data)

• return -ENOMEM;
  

• srm_data->srm_buf = kcalloc(DRM_HDCP_SRM_GEN1_MAX_BYTES,

• 		    sizeof(u8), GFP_KERNEL);
  

• if (!srm_data->srm_buf) {

• kfree(srm_data);
  

• return -ENOMEM;
  

• }
• mutex_init(&srm_data->mutex);
• return 0;

+}

+void drm_teardown_hdcp_srm(struct class *drm_class) +{

• if (srm_data) {

• kfree(srm_data->srm_buf);
  

• kfree(srm_data->revocated_ksv_list);
  

• kfree(srm_data);
  

• }

+} diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index d9a483a5fce0..ef3180443870 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -199,3 +199,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, const struct drm_framebuffer *fb); int drm_framebuffer_debugfs_init(struct drm_minor *minor);

+/* drm_hdcp.c */ +int drm_setup_hdcp_srm(struct class *drm_class); +void drm_teardown_hdcp_srm(struct class *drm_class); diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index ecb7b33002bb..18b1ac442997 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -78,6 +78,7 @@ int drm_sysfs_init(void) }

drm_class->devnode = drm_devnode;

• drm_setup_hdcp_srm(drm_class); return 0;

}

@@ -90,6 +91,7 @@ void drm_sysfs_destroy(void) { if (IS_ERR_OR_NULL(drm_class)) return;

• drm_teardown_hdcp_srm(drm_class); class_remove_file(drm_class, &class_attr_version.attr); class_destroy(drm_class); drm_class = NULL;

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index f243408ecf26..ff2bcfc1ecef 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -265,4 +265,40 @@ void drm_hdcp2_u32_to_seq_num(u8 seq_num[HDCP_2_2_SEQ_NUM_LEN], u32 val) seq_num[2] = val; }

+#define DRM_HDCP_SRM_GEN1_MAX_BYTES (5 * 1024) +#define DRM_HDCP_1_4_SRM_ID 0x8 +#define DRM_HDCP_1_4_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_1_4_DCP_SIG_SIZE 40

+struct hdcp_srm_header {

• struct {

• u8 reserved_hi:4;
  

• u8 srm_id:4;
  

bitfields in binary structures are heavily discouraged. I think since this is only u8 it should be fine, but explaining that is more work than just having the usual _MASK/_SHIFT #defines like we do with registers.

...

• u8 reserved_lo;
  

• } spec_indicator;

If you wannt __packed, you also need to list this to sub-structs.

...

• u16 srm_version;
• u8 srm_gen_no;

+} __packed;

+#define DRM_HDCP_2_SRM_ID 0x9 +#define DRM_HDCP_2_INDICATOR 0x1 +#define DRM_HDCP_2_VRL_LENGTH_SIZE 3 +#define DRM_HDCP_2_DCP_SIG_SIZE 384 +#define DRM_HDCP_2_NO_OF_DEV_PLUS_RESERVED_SZ 4

+#define DRM_HDCP_2_KSV_COUNT_2_LSBITS(byte) (((byte) & 0xC) >> 6)

+struct hdcp2_srm_header {

• struct {

• u8 hdcp2_indicator:4;
  

Since hdcp1/2 match except for this I think better to merge them.

...

• u8 srm_id:4;
  

• u8 reserved;
  

• } spec_indicator;
• u16 srm_version;
• u8 srm_gen_no;

+} __packed;

+struct drm_device;

+bool drm_hdcp_ksvs_revocated(struct drm_device *dev, u8 *ksvs, u32 ksv_count); #endif

Cheers, Daniel

...

-- 2.19.1

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Thu, Apr 18, 2019 at 02:27:59PM +0530, Ramalingam C wrote:

DRM HDCP SRM revocation check services are used from I915 for HDCP1.4 and 2.2 revocation check during the respective authentication flow.

v2: Rebased.

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/i915/intel_hdcp.c | 57 ++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 772974697b5a..de634f3fbd1b 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -492,9 +492,11 @@ int intel_hdcp_validate_v_prime(struct intel_digital_port *intel_dig_port,

/* Implements Part 2 of the HDCP authorization procedure */ static -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,

• 	       const struct intel_hdcp_shim *shim)
  

+int intel_hdcp_auth_downstream(struct intel_connector *connector) {

• struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
• const struct intel_hdcp_shim *shim = connector->hdcp.shim;
• struct drm_device *dev = connector->base.dev; u8 bstatus[2], num_downstream, *ksv_fifo; int ret, i, tries = 3;

@@ -533,6 +535,11 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, if (ret) goto err;

• if (drm_hdcp_ksvs_revocated(dev, ksv_fifo, num_downstream)) {

• DRM_ERROR("Revocated Ksv(s) in ksv_fifo\n");
  

• return -EPERM;
  

• }
• /*
  • When V prime mismatches, DP Spec mandates re-read of
  • V prime atleast twice.

@@ -559,9 +566,12 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, }

/* Implements Part 1 of the HDCP authorization procedure */ -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,

• 	   const struct intel_hdcp_shim *shim)
  

+static int intel_hdcp_auth(struct intel_connector *connector) {

• struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
• struct intel_hdcp *hdcp = &connector->hdcp;
• struct drm_device *dev = connector->base.dev;
• const struct intel_hdcp_shim *shim = hdcp->shim; struct drm_i915_private *dev_priv; enum port port; unsigned long r0_prime_gen_start;

@@ -627,6 +637,11 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, if (ret < 0) return ret;

• if (drm_hdcp_ksvs_revocated(dev, bksv.shim, 1)) {

• DRM_ERROR("BKSV is revocated\n");
  

• return -EPERM;
  

• }
• I915_WRITE(PORT_HDCP_BKSVLO(port), bksv.reg[0]); I915_WRITE(PORT_HDCP_BKSVHI(port), bksv.reg[1]);

@@ -700,7 +715,7 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, */

if (repeater_present)

• return intel_hdcp_auth_downstream(intel_dig_port, shim);
  

• return intel_hdcp_auth_downstream(connector);
  
  

  DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); return 0;

@@ -763,7 +778,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)

/* Incase of authentication failures, HDCP spec expects reauth. */ for (i = 0; i < tries; i++) {

• ret = intel_hdcp_auth(conn_to_dig_port(connector), hdcp->shim);
  

• ret = intel_hdcp_auth(connector);
  

  if (!ret) { hdcp->hdcp_encrypted = true; return 0;

@@ -779,12 +794,6 @@ static int _intel_hdcp_enable(struct intel_connector *connector) return ret; }

-static inline -struct intel_connector *intel_hdcp_to_connector(struct intel_hdcp *hdcp) -{

• return container_of(hdcp, struct intel_connector, hdcp);

-}

/* Implements Part 3 of the HDCP authorization procedure */ static int intel_hdcp_check_link(struct intel_connector *connector) { @@ -845,6 +854,12 @@ static int intel_hdcp_check_link(struct intel_connector *connector) return ret; }

+static inline +struct intel_connector *intel_hdcp_to_connector(struct intel_hdcp *hdcp) +{

• return container_of(hdcp, struct intel_connector, hdcp);

+}

Moving intel_hdcp_to_connector around doesn't seem to be necessary?

static void intel_hdcp_prop_work(struct work_struct *work) { struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp, @@ -1162,6 +1177,7 @@ static int hdcp2_authentication_key_exchange(struct intel_connector *connector) { struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); struct intel_hdcp *hdcp = &connector->hdcp;

• struct drm_device *dev = connector->base.dev; union { struct hdcp2_ake_init ake_init; struct hdcp2_ake_send_cert send_cert;

@@ -1196,6 +1212,12 @@ static int hdcp2_authentication_key_exchange(struct intel_connector *connector)

hdcp->is_repeater = HDCP_2_2_RX_REPEATER(msgs.send_cert.rx_caps[2]);

• if (drm_hdcp_ksvs_revocated(dev,

• 		    msgs.send_cert.cert_rx.receiver_id, 1)) {
  

• DRM_ERROR("Receiver ID is revocated\n");
  

• return -EPERM;
  

• }
• /*
  • Here msgs.no_stored_km will hold msgs corresponding to the km
  • stored also.

@@ -1348,13 +1370,14 @@ int hdcp2_authenticate_repeater_topology(struct intel_connector *connector) { struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); struct intel_hdcp *hdcp = &connector->hdcp;

• struct drm_device *dev = connector->base.dev; union { struct hdcp2_rep_send_receiverid_list recvid_list; struct hdcp2_rep_send_ack rep_ack; } msgs; const struct intel_hdcp_shim *shim = hdcp->shim;
• u32 seq_num_v, device_cnt; u8 *rx_info;

• u32 seq_num_v; int ret;

  ret = shim->read_2_2_msg(intel_dig_port, HDCP_2_2_REP_SEND_RECVID_LIST,

@@ -1379,6 +1402,14 @@ int hdcp2_authenticate_repeater_topology(struct intel_connector *connector) return -EINVAL; }

• device_cnt = HDCP_2_2_DEV_COUNT_HI(rx_info[0]) << 4 ||

• 	HDCP_2_2_DEV_COUNT_LO(rx_info[1]);
  

• if (drm_hdcp_ksvs_revocated(dev, msgs.recvid_list.receiver_ids,

• 		    device_cnt)) {
  

• DRM_ERROR("Revoked receiver ID(s) is in list\n");
  

• return -EPERM;
  

• }
• ret = hdcp2_verify_rep_topology_prepare_ack(connector, &msgs.recvid_list, &msgs.rep_ack);

-- 2.19.1

Aside from the nit lgtm, with that hunk removed:

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

Plus might need to adjust to drm_hdcp_check_ksvs_revoked. -Daniel

On Thu, Apr 18, 2019 at 02:28:00PM +0530, Ramalingam C wrote:

Considering the significant size of hdcp related code in drm, all hdcp related codes are moved into separate file called drm_hdcp.c.

v2: Rebased.

Signed-off-by: Ramalingam C ramalingam.c@intel.com Suggested-by: Daniel Vetter daniel@ffwll.ch

---

drivers/gpu/drm/drm_connector.c | 78 ------------------------------- drivers/gpu/drm/drm_hdcp.c | 81 +++++++++++++++++++++++++++++++++

Need to make sure this is still included into Documenation/gpu/drm-kms-helpers.rst. But an earlier patch should take care of that, so

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

include/drm/drm_connector.h | 2 - include/drm/drm_hdcp.h | 3 ++ 4 files changed, 84 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 03907d13ef66..436cf8e764cc 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -823,13 +823,6 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = { DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name, drm_tv_subconnector_enum_list)

-static struct drm_prop_enum_list drm_cp_enum_list[] = {

• { DRM_MODE_CONTENT_PROTECTION_UNDESIRED, "Undesired" },
• { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
• { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },

-}; -DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)

static const struct drm_prop_enum_list hdmi_colorspaces[] = { /* For Default case, driver will set the colorspace */ { DRM_MODE_COLORIMETRY_DEFAULT, "Default" }, @@ -857,13 +850,6 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = { { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" }, };

-static struct drm_prop_enum_list drm_hdcp_content_type_enum_list[] = {

• { DRM_MODE_HDCP_CONTENT_TYPE0, "HDCP Type0" },
• { DRM_MODE_HDCP_CONTENT_TYPE1, "HDCP Type1" },

-}; -DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,

•  drm_hdcp_content_type_enum_list)
  

/**

• DOC: standard connector properties

@@ -1539,70 +1525,6 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);

-/**

• • drm_connector_attach_content_protection_property - attach content protection
• • property
• • @connector: connector to attach CP property on.
• • @hdcp_content_type: is HDCP Content Type property needed for connector
• • This is used to add support for content protection on select connectors.
• • Content Protection is intentionally vague to allow for different underlying
• • technologies, however it is most implemented by HDCP.
• • When hdcp_content_type is true enum property called HDCP Content Type is
• • created (if it is not already) and attached to the connector.
• • This property is used for sending the protected content's stream type
• • from userspace to kernel on selected connectors. Protected content provider
• • will decide their type of their content and declare the same to kernel.
• • Content type will be used during the HDCP 2.2 authentication.
• • Content type will be set to &drm_connector_state.hdcp_content_type.
• • The content protection will be set to &drm_connector_state.content_protection
• • Returns:
• • Zero on success, negative errno on failure.
• */

-int drm_connector_attach_content_protection_property(

• struct drm_connector *connector, bool hdcp_content_type)
  

-{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop =

• 	dev->mode_config.content_protection_property;
  

• if (!prop)

• prop = drm_property_create_enum(dev, 0, "Content Protection",
  

• 				drm_cp_enum_list,
  

• 				ARRAY_SIZE(drm_cp_enum_list));
  

• if (!prop)

• return -ENOMEM;
  

• drm_object_attach_property(&connector->base, prop,

• 		   DRM_MODE_CONTENT_PROTECTION_UNDESIRED);
  

• dev->mode_config.content_protection_property = prop;
• if (!hdcp_content_type)

• return 0;
  

• prop = dev->mode_config.hdcp_content_type_property;
• if (!prop)

• prop = drm_property_create_enum(dev, 0, "HDCP Content Type",
  

• 			drm_hdcp_content_type_enum_list,
  

• 			ARRAY_SIZE(
  

• 			drm_hdcp_content_type_enum_list));
  

• if (!prop)

• return -ENOMEM;
  

• drm_object_attach_property(&connector->base, prop,

• 		   DRM_MODE_HDCP_CONTENT_TYPE0);
  

• dev->mode_config.hdcp_content_type_property = prop;
• return 0;

-} -EXPORT_SYMBOL(drm_connector_attach_content_protection_property);

/**

• drm_mode_create_aspect_ratio_property - create aspect ratio property
• @dev: DRM device

diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 78b043c8195e..a0960507e4ff 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -17,6 +17,9 @@ #include <drm/drm_sysfs.h> #include <drm/drm_print.h> #include <drm/drm_device.h> +#include <drm/drm_property.h> +#include <drm/drm_mode_object.h> +#include <drm/drm_connector.h>

struct hdcp_srm { u8 *srm_buf; @@ -334,3 +337,81 @@ void drm_teardown_hdcp_srm(struct class *drm_class) kfree(srm_data); } }

+static struct drm_prop_enum_list drm_cp_enum_list[] = {

• { DRM_MODE_CONTENT_PROTECTION_UNDESIRED, "Undesired" },
• { DRM_MODE_CONTENT_PROTECTION_DESIRED, "Desired" },
• { DRM_MODE_CONTENT_PROTECTION_ENABLED, "Enabled" },

+}; +DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)

+static struct drm_prop_enum_list drm_hdcp_content_type_enum_list[] = {

• { DRM_MODE_HDCP_CONTENT_TYPE0, "HDCP Type0" },
• { DRM_MODE_HDCP_CONTENT_TYPE1, "HDCP Type1" },

+}; +DRM_ENUM_NAME_FN(drm_get_hdcp_content_type_name,

•  drm_hdcp_content_type_enum_list)
  

+/**

• • drm_connector_attach_content_protection_property - attach content protection
• • property
• • @connector: connector to attach CP property on.
• • @hdcp_content_type: is HDCP Content Type property needed for connector
• • This is used to add support for content protection on select connectors.
• • Content Protection is intentionally vague to allow for different underlying
• • technologies, however it is most implemented by HDCP.
• • When hdcp_content_type is true enum property called HDCP Content Type is
• • created (if it is not already) and attached to the connector.
• • This property is used for sending the protected content's stream type
• • from userspace to kernel on selected connectors. Protected content provider
• • will decide their type of their content and declare the same to kernel.
• • Content type will be used during the HDCP 2.2 authentication.
• • Content type will be set to &drm_connector_state.hdcp_content_type.
• • The content protection will be set to &drm_connector_state.content_protection
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_content_protection_property(

• struct drm_connector *connector, bool hdcp_content_type)
  

+{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop =

• 	dev->mode_config.content_protection_property;
  

• if (!prop)

• prop = drm_property_create_enum(dev, 0, "Content Protection",
  

• 				drm_cp_enum_list,
  

• 				ARRAY_SIZE(drm_cp_enum_list));
  

• if (!prop)

• return -ENOMEM;
  

• drm_object_attach_property(&connector->base, prop,

• 		   DRM_MODE_CONTENT_PROTECTION_UNDESIRED);
  

• dev->mode_config.content_protection_property = prop;
• if (!hdcp_content_type)

• return 0;
  

• prop = dev->mode_config.hdcp_content_type_property;
• if (!prop)

• prop = drm_property_create_enum(dev, 0, "HDCP Content Type",
  

• 			drm_hdcp_content_type_enum_list,
  

• 			ARRAY_SIZE(
  

• 			drm_hdcp_content_type_enum_list));
  

• if (!prop)

• return -ENOMEM;
  

• drm_object_attach_property(&connector->base, prop,

• 		   DRM_MODE_HDCP_CONTENT_TYPE0);
  

• dev->mode_config.hdcp_content_type_property = prop;
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_content_protection_property); diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 2d2c2d5e7681..9e2f1a9de2a0 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1346,8 +1346,6 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector, u32 scaling_mode_mask); int drm_connector_attach_vrr_capable_property( struct drm_connector *connector); -int drm_connector_attach_content_protection_property(

• struct drm_connector *connector, bool hdcp_content_type);
  

int drm_mode_create_aspect_ratio_property(struct drm_device *dev); int drm_mode_create_colorspace_property(struct drm_connector *connector); int drm_mode_create_content_type_property(struct drm_device *dev); diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index ff2bcfc1ecef..145c81ba1e09 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -299,6 +299,9 @@ struct hdcp2_srm_header { } __packed;

struct drm_device; +struct drm_connector;

bool drm_hdcp_ksvs_revocated(struct drm_device *dev, u8 *ksvs, u32 ksv_count); +int drm_connector_attach_content_protection_property(

• struct drm_connector *connector, bool hdcp_content_type);
  

#endif

2.19.1

On Thu, Apr 18, 2019 at 02:28:01PM +0530, Ramalingam C wrote:

DRM API for generating uevent for a status changes of connector's property.

This uevent will have following details related to the status change:

HOTPLUG=1, CONNECTOR=<connector_id> and PROPERTY=<property_id> v2: Minor fixes at KDoc comments [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/drm_sysfs.c | 31 +++++++++++++++++++++++++++++++ include/drm/drm_sysfs.h | 5 ++++- 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 18b1ac442997..e8f1fd73677f 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -320,6 +320,9 @@ void drm_sysfs_lease_event(struct drm_device *dev)

• Send a uevent for the DRM device specified by @dev. Currently we only
• set HOTPLUG=1 in the uevent environment, but this could be expanded to
• deal with other types of events.

• • Any new uapi should be using the drm_sysfs_connector_status_event()
• • for uevents on connector status change.
  */

void drm_sysfs_hotplug_event(struct drm_device *dev) { @@ -332,6 +335,34 @@ void drm_sysfs_hotplug_event(struct drm_device *dev) } EXPORT_SYMBOL(drm_sysfs_hotplug_event);

+/**

• • drm_sysfs_connector_status_event - generate a DRM uevent for connector
• • property status change
• • @connector: connector on which property status changed
• • @property: connector property whoes status changed.
• • Send a uevent for the DRM device specified by @dev. Currently we
• • set HOTPLUG=1 and connector id along with the attached property id
• • related to the status change.
• */

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

• 		      struct drm_property *property)
  

+{

• struct drm_device *dev = connector->dev;
• char hotplug_str[] = "HOTPLUG=1", conn_id[30], prop_id[30];
• char *envp[4] = { hotplug_str, conn_id, prop_id, NULL };
• snprintf(conn_id, ARRAY_SIZE(conn_id),

•  "CONNECTOR=%u", connector->base.id);
  

• snprintf(prop_id, ARRAY_SIZE(prop_id),

•  "PROPERTY=%u", property->base.id);
  

Maybe double-check that property is attached to connector, and WARN if not? Just to catch mistakes in driver programming.

With that: Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

• DRM_DEBUG("generating connector status event\n");
• kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE, envp);

+} +EXPORT_SYMBOL(drm_sysfs_connector_status_event);

static void drm_sysfs_release(struct device *dev) { kfree(dev); diff --git a/include/drm/drm_sysfs.h b/include/drm/drm_sysfs.h index 4f311e836cdc..d454ef617b2c 100644 --- a/include/drm/drm_sysfs.h +++ b/include/drm/drm_sysfs.h @@ -4,10 +4,13 @@

struct drm_device; struct device; +struct drm_connector; +struct drm_property;

int drm_class_device_register(struct device *dev); void drm_class_device_unregister(struct device *dev);

void drm_sysfs_hotplug_event(struct drm_device *dev);

+void drm_sysfs_connector_status_event(struct drm_connector *connector,

• 		      struct drm_property *property);
  

#endif

2.19.1

On Thu, Apr 18, 2019 at 02:28:02PM +0530, Ramalingam C wrote:

drm function is defined and exported to update a connector's content protection property state and to generate a uevent along with it.

v2: Update only when state is different from old one.

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/drm_hdcp.c | 16 ++++++++++++++++ include/drm/drm_hdcp.h | 2 ++ 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index a0960507e4ff..5640c4e744fe 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -415,3 +415,19 @@ int drm_connector_attach_content_protection_property( return 0; } EXPORT_SYMBOL(drm_connector_attach_content_protection_property);

Needs kerneldoc. Plus I think a note in the attach function that sets up hdcp for a connector that it should be used.

Plus we need an ack from userspace people (weston?) that they're using this new uapi and are happy with it.

+void drm_hdcp_update_content_protection(struct drm_connector *connector,

• 			u64 val)
  

+{

• struct drm_device *dev = connector->dev;
• struct drm_connector_state *state = connector->state;
• WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
• if (state->content_protection == val)

• return;
  

• state->content_protection = val;
• drm_sysfs_connector_status_event(connector,

• 		 dev->mode_config.content_protection_property);
  

+} +EXPORT_SYMBOL(drm_hdcp_update_content_protection);

With all that addressed has my Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 145c81ba1e09..9457c7ec0d37 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -304,4 +304,6 @@ struct drm_connector; bool drm_hdcp_ksvs_revocated(struct drm_device *dev, u8 *ksvs, u32 ksv_count); int drm_connector_attach_content_protection_property( struct drm_connector *connector, bool hdcp_content_type); +void drm_hdcp_update_content_protection(struct drm_connector *connector,

• 			u64 val);
  

#endif

2.19.1

On Thu, Apr 18, 2019 at 02:28:03PM +0530, Ramalingam C wrote:

drm function to update the content protection property state and to generate a uevent is invoked from the intel hdcp property work.

Hence whenever kernel changes the property state, userspace will be updated with a uevent.

v2: state update is moved into drm function [daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/intel_hdcp.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index de634f3fbd1b..84ac23ea75af 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -866,7 +866,6 @@ static void intel_hdcp_prop_work(struct work_struct *work) prop_work); struct intel_connector *connector = intel_hdcp_to_connector(hdcp); struct drm_device *dev = connector->base.dev;

• struct drm_connector_state *state;

  drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); mutex_lock(&hdcp->mutex);

@@ -876,10 +875,9 @@ static void intel_hdcp_prop_work(struct work_struct *work) * those to UNDESIRED is handled by core. If value == UNDESIRED, * we're running just after hdcp has been disabled, so just exit */

• if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {

• state = connector->base.state;
  

• state->content_protection = hdcp->value;
  

• }

• if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)

• drm_hdcp_update_content_protection(&connector->base,
  

• 				   hdcp->value);
  
  

  mutex_unlock(&hdcp->mutex); drm_modeset_unlock(&dev->mode_config.connection_mutex);

-- 2.19.1

On Thu, Apr 18, 2019 at 02:28:04PM +0530, Ramalingam C wrote:

This patch adds a optional CP downstream info blob property to the connectors. This enables the Userspace to read the information of HDCP authenticated downstream topology.

Driver will update this blob with all downstream information at the end of the authentication.

In case userspace configures this platform as repeater, then this information is needed for the authentication with upstream HDCP transmitter.

v2: s/cp_downstream/content_protection_downstream [daniel] v3: s/content_protection_downstream/hdcp_topology [daniel] v4: hdcp_topology_info struct is added with explicit padding [Daniel] v5: Rebased.

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/drm_atomic_uapi.c | 3 ++ drivers/gpu/drm/drm_connector.c | 20 ++++++++++ drivers/gpu/drm/drm_hdcp.c | 65 +++++++++++++++++++++++++++++++ include/drm/drm_connector.h | 6 +++ include/drm/drm_hdcp.h | 6 +++ include/drm/drm_mode_config.h | 6 +++ include/uapi/drm/drm_mode.h | 37 ++++++++++++++++++ 7 files changed, 143 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 0b0747869963..1c9e1ab0d536 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -813,6 +813,9 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = state->content_protection; } else if (property == config->hdcp_content_type_property) { *val = state->hdcp_content_type;

• } else if (property == config->hdcp_topology_property) {

• *val = connector->hdcp_topology_blob_ptr ?
  

• 	connector->hdcp_topology_blob_ptr->base.id : 0;
  

  } 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 436cf8e764cc..033ced774d37 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -246,6 +246,7 @@ int drm_connector_init(struct drm_device *dev, mutex_init(&connector->mutex); connector->edid_blob_ptr = NULL; connector->tile_blob_ptr = NULL;

• connector->hdcp_topology_blob_ptr = NULL; connector->status = connector_status_unknown; connector->display_info.panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;

@@ -972,6 +973,25 @@ static const struct drm_prop_enum_list hdmi_colorspaces[] = {

• authentication process. If content type is changed when
• content_protection is not UNDESIRED, then kernel will disable the HDCP
• and re-enable with new type in the same atomic commit

• • HDCP Topology:
• • This blob property is used to pass the HDCP downstream topology details
• • of a HDCP encrypted connector, from kernel to userspace.
• • This provides all required information to userspace, so that userspace
• • can implement the HDCP repeater using the kernel as downstream ports of
• • the repeater. as illustrated below:

• •                      HDCP Repeaters
    

• • +--------------------------------------------------------------+
• • | |
• • | | |
• • | Userspace HDCP Receiver +-----> KMD HDCP transmitters |
• • | (Upstream Port) <------+ (Downstream Ports) |
• • | | |
• • | |
• • +--------------------------------------------------------------+

Hm I think this misrenders, since the table isn't indented or marked as fixed with. Did you check the output of $make htmldocs?

• • Kernel will populate this blob only when the HDCP authentication is
• • successful.
  • max bpc:
  • This range property is used by userspace to limit the bit depth. When

diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c index 5640c4e744fe..c0d3fc93fc50 100644 --- a/drivers/gpu/drm/drm_hdcp.c +++ b/drivers/gpu/drm/drm_hdcp.c @@ -431,3 +431,68 @@ void drm_hdcp_update_content_protection(struct drm_connector *connector, dev->mode_config.content_protection_property); } EXPORT_SYMBOL(drm_hdcp_update_content_protection);

+/**

• • drm_connector_attach_hdcp_topology_property - attach hdcp topology property
• • @connector: connector to attach hdcp topology property with.
• • This is used to add support for hdcp topology support on select connectors.
• • When Intel platform is configured as repeater, this downstream info is used
• • by userspace, to complete the repeater authentication of HDCP specification
• • with upstream HDCP transmitter.
• • The blob_id of the hdcp topology info will be set to
• • &drm_connector_state.hdcp_topology
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int drm_connector_attach_hdcp_topology_property(struct drm_connector *connector) +{

• struct drm_device *dev = connector->dev;
• struct drm_property *prop = dev->mode_config.hdcp_topology_property;
• if (!prop)

• prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
  

• 			   DRM_MODE_PROP_IMMUTABLE,
  

• 			   "HDCP Topology", 0);
  

• if (!prop)

• return -ENOMEM;
  

• drm_object_attach_property(&connector->base, prop, 0);
• dev->mode_config.hdcp_topology_property = prop;
• return 0;

+} +EXPORT_SYMBOL(drm_connector_attach_hdcp_topology_property);

+/**

• • drm_connector_update_hdcp_topology_property - update the hdcp topology
• • property of a connector
• • @connector: drm connector, the topology is associated to
• • @hdcp_topology_info: new content for the blob of hdcp_topology_property
• • This function creates a new blob modeset object and assigns its id to the
• • connector's hdcp_topology_property.
• • Returns:
• • Zero on success, negative errno on failure.
• */

+int +drm_connector_update_hdcp_topology_property(struct drm_connector *connector,

• 			const struct hdcp_topology_info *info)
  

+{

• struct drm_device *dev = connector->dev;
• int ret;
• if (!info)

• return -EINVAL;
  

• ret = drm_property_replace_global_blob(dev,

• 	&connector->hdcp_topology_blob_ptr,
  

• 	sizeof(struct hdcp_topology_info),
  

• 	info, &connector->base,
  

• 	dev->mode_config.hdcp_topology_property);
  

• return ret;

+}

Hm, I'd put this into the previous function that also sends out the uevent. That way we can guarantee that we update everyting _before_ we send out the uevent to signal to userspace that it should take some action.

+EXPORT_SYMBOL(drm_connector_update_hdcp_topology_property); diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 9e2f1a9de2a0..180f8f4d4526 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1050,6 +1050,12 @@ struct drm_connector { /** @properties: property tracking for this connector */ struct drm_object_properties properties;

• /**

• * @hdcp_topology_blob_ptr: DRM BLOB pointer for hdcp downstream
  

• * topology information.
  

• */
  

• struct drm_property_blob *hdcp_topology_blob_ptr;
• /**
  • @scaling_mode_property: Optional atomic property to control the
  • upscaling. See drm_connector_attach_content_protection_property().

diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index 9457c7ec0d37..011f35e121a5 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -300,10 +300,16 @@ struct hdcp2_srm_header {

struct drm_device; struct drm_connector; +struct hdcp_topology_info;

bool drm_hdcp_ksvs_revocated(struct drm_device *dev, u8 *ksvs, u32 ksv_count); int drm_connector_attach_content_protection_property( struct drm_connector *connector, bool hdcp_content_type); void drm_hdcp_update_content_protection(struct drm_connector *connector, u64 val); +int drm_connector_attach_hdcp_topology_property(

• 		struct drm_connector *connector);
  

+int drm_connector_update_hdcp_topology_property(

• 		struct drm_connector *connector,
  

• 		const struct hdcp_topology_info *info);
  

#endif diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index b359b5b71eb9..4349b614fa15 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -848,6 +848,12 @@ struct drm_mode_config { */ struct drm_property *hdcp_content_type_property;

• /**

• * @hdcp_topology_property: DRM BLOB property for hdcp downstream
  

• * topology information.
  

• */
  

• struct drm_property *hdcp_topology_property;
• /* dumb ioctl parameters */ uint32_t preferred_depth, prefer_shadow;

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 8ac03351fdee..9d7cdfab4962 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -213,6 +213,43 @@ extern "C" { #define DRM_MODE_HDCP_CONTENT_TYPE0 0 #define DRM_MODE_HDCP_CONTENT_TYPE1 1

+#define DRM_MODE_HDCP_KSV_LEN 5 +#define DRM_MODE_HDCP_MAX_DEVICE_CNT 127 +#define DRM_MODE_HDCP14_IN_FORCE (1 << 0) +#define DRM_MODE_HDCP22_IN_FORCE (1 << 1)

+struct hdcp_topology_info {

• /* Version of HDCP authenticated (1.4/2.2) */
• __u32 ver_in_force;
• /* Applicable only for HDCP2.2 */
• __u32 content_type;
• /* KSV of immediate HDCP Sink. In Little-Endian Format. */
• __u8 bksv[DRM_MODE_HDCP_KSV_LEN];
• /* Whether Immediate HDCP sink is a repeater? */
• __u8 is_repeater;
• /* Depth received from immediate downstream repeater */
• __u8 depth;
• __u8 pad1;
• /* Device count received from immediate downstream repeater */
• __u32 device_count;
• /*

• * Max buffer required to hold ksv list received from immediate
  

• * repeater. In this array first device_count * DRM_MODE_HDCP_KSV_LEN
  

• * will hold the valid ksv bytes.
  

• * If authentication specification is
  

• *	HDCP1.4 - each KSV's Bytes will be in Little-Endian format.
  

• *	HDCP2.2 - each KSV's Bytes will be in Big-Endian format.
  

• */
  

• __u8 ksv_list[DRM_MODE_HDCP_KSV_LEN * DRM_MODE_HDCP_MAX_DEVICE_CNT];
• __u8 pad2[5];

+} __packed;

So I think we need userspace for this, but not sure where the discussions on that are right now. Is this feasible, or this and follow-up i915 patch more fyi and not for upstream? -Daniel

struct drm_mode_modeinfo { __u32 clock; __u16 hdisplay; -- 2.19.1