Re: [PATCH v3 07/10] drm/i915: Populate downstream info for HDCP1.4

Daniel Vetter
2019-03-27T10:28:43Z

On Fri, Mar 22, 2019 at 06:14:45AM +0530, Ramalingam C wrote:
> Implements drm blob property content_protection_downstream_info
> property on HDCP capable connectors.
>
> Downstream topology info is gathered across authentication stages
> and stored in intel_hdcp. When HDCP authentication is complete,
> new blob with latest downstream topology information is updated to
> content_protection_downstream_info property.
>
> v2:
> %s/cp_downstream/content_protection_downstream [daniel]
> v3:
> %s/content_protection_downstream/hdcp_topology [daniel]
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_hdcp.c | 30 +++++++++++++++++++++++++++++-
> include/drm/drm_hdcp.h | 1 +
> include/uapi/drm/drm_mode.h | 5 +++++
> 4 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4db15ee81042..46325fb33507 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -482,6 +482,8 @@ struct intel_hdcp {
> wait_queue_head_t cp_irq_queue;
> atomic_t cp_irq_count;
> int cp_irq_count_cached;
> +
> + struct hdcp_topology_info *topology_info;
> };
>
> struct intel_connector {
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 921f07c64350..0e3d7afecd5a 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -580,6 +580,9 @@ int intel_hdcp_auth_downstream(struct intel_hdcp *hdcp,
> if (num_downstream == 0)
> return -EINVAL;
>
> + hdcp->topology_info->device_count = num_downstream;
> + hdcp->topology_info->depth = DRM_HDCP_DEPTH(bstatus[1]);
> +
> ksv_fifo = kcalloc(DRM_HDCP_KSV_LEN, num_downstream, GFP_KERNEL);
> if (!ksv_fifo)
> return -ENOMEM;
> @@ -593,6 +596,8 @@ int intel_hdcp_auth_downstream(struct intel_hdcp *hdcp,
> return -EPERM;
> }
>
> + memcpy(hdcp->topology_info->ksv_list, ksv_fifo,
> + num_downstream * DRM_HDCP_KSV_LEN);
> /*
> * When V prime mismatches, DP Spec mandates re-read of
> * V prime atleast twice.
> @@ -694,15 +699,20 @@ static int intel_hdcp_auth(struct intel_connector *connector)
> return -EPERM;
> }
>
> + hdcp->topology_info->ver_in_force = DRM_MODE_HDCP14_IN_FORCE;
> + memcpy(hdcp->topology_info->bksv, bksv.shim, DRM_MODE_HDCP_KSV_LEN);
> +
> I915_WRITE(PORT_HDCP_BKSVLO(port), bksv.reg[0]);
> I915_WRITE(PORT_HDCP_BKSVHI(port), bksv.reg[1]);
>
> ret = shim->repeater_present(intel_dig_port, &repeater_present);
> if (ret)
> return ret;
> - if (repeater_present)
> + if (repeater_present) {
> I915_WRITE(HDCP_REP_CTL,
> intel_hdcp_get_repeater_ctl(intel_dig_port));
> + hdcp->topology_info->is_repeater = true;
> + }
>
> ret = shim->toggle_signalling(intel_dig_port, true);
> if (ret)
> @@ -798,6 +808,12 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
> return ret;
> }
>
> + memset(hdcp->topology_info, 0, sizeof(struct hdcp_topology_info));
> +
> + if (drm_connector_update_hdcp_topology_property(&connector->base,
> + connector->hdcp.topology_info))
> + DRM_ERROR("Downstream_info update failed.\n");
> +
> DRM_DEBUG_KMS("HDCP is disabled\n");
> return 0;
> }
> @@ -831,6 +847,10 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
> ret = intel_hdcp_auth(connector);
> if (!ret) {
> connector->hdcp.hdcp_encrypted = true;
> + if (drm_connector_update_hdcp_topology_property(
> + &connector->base,
> + connector->hdcp.topology_info))
> + DRM_ERROR("Downstream_info update failed.\n");
> return 0;
> }
>
> @@ -1882,6 +1902,14 @@ int intel_hdcp_init(struct intel_connector *connector,
> if (ret)
> return ret;
>
> + ret = drm_connector_attach_hdcp_topology_property(&connector->base);
> + if (ret)
> + return ret;
> +
> + hdcp->topology_info = kzalloc(sizeof(*hdcp->topology_info), GFP_KERNEL);
> + if (!hdcp->topology_info)
> + return -ENOMEM;

Since you need to assemble the info struct manually from information from
all over: What if we make that the interface of the update function (i.e.
you pass it the list of ksv, bksv, and a bunch of flags), and the core
function bakes that into a structure and correct property? Would be a
slightly cleaner interface I think, and no risk that someone accidentally
forgets to set one of the values and leaves something stale behind.

> +
> hdcp->shim = shim;
> mutex_init(&hdcp->mutex);
> INIT_DELAYED_WORK(&hdcp->check_work, intel_hdcp_check_work);
> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> index 652aaf5d658e..654a170f03eb 100644
> --- a/include/drm/drm_hdcp.h
> +++ b/include/drm/drm_hdcp.h
> @@ -23,6 +23,7 @@
> #define DRM_HDCP_V_PRIME_PART_LEN 4
> #define DRM_HDCP_V_PRIME_NUM_PARTS 5
> #define DRM_HDCP_NUM_DOWNSTREAM(x) (x & 0x7f)
> +#define DRM_HDCP_DEPTH(x) ((x) & 0x7)
> #define DRM_HDCP_MAX_CASCADE_EXCEEDED(x) (x & BIT(3))
> #define DRM_HDCP_MAX_DEVICE_EXCEEDED(x) (x & BIT(7))
>
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 03d3aa2b1a49..733db7283e57 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -216,8 +216,13 @@ extern "C" {
>
> #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;

Looks like misplaced hunk. Maybe use one of the __u8 padding fields you
need anyway.
-Daniel

> +
> /* KSV of immediate HDCP Sink. In Little-Endian Format. */
> char bksv[DRM_MODE_HDCP_KSV_LEN];
>
> --
> 2.19.1
>

--
Daniel Vetter
Software Engineer, Intel Corporation