Re: [PATCH 25/43] drm/i915: Define HDCP2.2 related variables

Sean Paul
2018-02-22T14:59:03Z

On Wed, Feb 14, 2018 at 07:43:40PM +0530, Ramalingam C wrote:
> For upcoming implementation of HDCP2.2 in I915, all variable required
> for HDCP2.2 are defined.
>
> This includes a translation layer called hdcp2_shim for encoder
> specific HDCP2.2 spec deviations.
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 +++
> drivers/gpu/drm/i915/intel_drv.h | 91 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 96 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3f8d824dc313..09d31db90a38 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -44,6 +44,7 @@
> #include <linux/pm_qos.h>
> #include <linux/reservation.h>
> #include <linux/shmem_fs.h>
> +#include <linux/mei_hdcp.h>
>
> #include <drm/drmP.h>
> #include <drm/intel-gtt.h>
> @@ -2365,6 +2366,10 @@ struct drm_i915_private {
>
> struct i915_pmu pmu;
>
> + /* Mei interface handle */
> + struct mei_cl_device *mei_cldev;
> + int mei_ref_cnt;
> +
> /*
> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
> * will be rejected. Instead look for a better place.
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1c0d324c1044..9c05049c25d5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -301,6 +301,62 @@ struct intel_panel {
> } backlight;
> };
>
> +struct intel_hdcp;

Why?

> +
> +#define HDCP_CAPABILITY BIT(0)
> +#define HDCP2_CAPABILITY BIT(1)

These names are pretty vague, without a comment it's hard to know what/where
they're used.

It's really hard to review changes where things are just defined. Can you please
squash this patch into the patch where you actually use it?

> +
> +/*
> + * This structure serves as a translation layer between the generic HDCP2.2 code
> + * and the bus-specific code. What that means is that HDCP over HDMI differs
> + * from HDCP over DP, so to account for these differences, we need to
> + * communicate with the receiver through this shim.
> + *
> + * For completeness, the 2 buses differ in the following ways:
> + * - DP AUX vs. DDC
> + * HDCP registers on the receiver are set via DP AUX for DP, and
> + * they are set via DDC for HDMI.
> + * - Receiver register offsets
> + * The offsets of the registers are different for DP vs. HDMI
> + * - Receiver register masks/offsets
> + * Few register bit definitions are different between DP and HDMI.
> + * - HDCP signalling
> + * Needed for HDMI but not for DP.
> + * - Configure stream type onto receiver
> + * Needed only for DP but not for HDMI.
> + */
> +struct intel_hdcp2_shim {

Why don't you just use the same shim? No need to duplicate.

> +
> + /* Write HDCP2.2 messages */
> + int (*write_msg)(struct intel_digital_port *intel_dig_port, void *buf,
> + size_t size);
> +
> + /* Read HDCP2.2 messages */
> + int (*read_msg)(struct intel_digital_port *intel_dig_port,
> + uint8_t msg_id, void *buf, size_t size);
> +
> + /*
> + * Implementation of DP Errata for the communication of stream type to
> + * Receivers. Not applicable for HDMI.
> + */
> + int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
> + void *buf, size_t size);
> +
> + /* Enables HDCP signalling on the port. Not applicable for DP. */
> + int (*toggle_signalling)(struct intel_digital_port *intel_dig_port,
> + bool enable);
> +
> + /* Link Integrity Check */
> + int (*check_link)(struct intel_digital_port *intel_dig_port);
> +
> + /* Detects whether Panel is HDCP2.2 capable */
> + int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> + bool *capable);
> +
> + /* Detects the HDCP protocol(DP/HDMI) required on the port */
> + enum hdcp_protocol (*hdcp_protocol)(void);
> +};
> +
> /*
> * This structure serves as a translation layer between the generic HDCP code
> * and the bus-specific code. What that means is that HDCP over HDMI differs
> @@ -378,6 +434,10 @@ struct intel_hdcp_shim {
> struct intel_hdcp {
> struct intel_connector *connector;
>
> + /* Flags to represent the HDCP capability of the platform and panel */
> + u32 plat_cap;

I don't think you need this. A simple bool supports_hdcp2 would suffice here.

> + u32 panel_cap;

This should not be stored here. At best it will be complicated to keep
up-to-date and at worst, it will get stale.

> +
> const struct intel_hdcp_shim *hdcp_shim;
> struct mutex hdcp_mutex;
> uint64_t hdcp_value; /* protected by hdcp_mutex */
> @@ -387,6 +447,37 @@ struct intel_hdcp {
>
> /* To indicate the assertion of CP_IRQ */
> struct completion cp_irq_recved;
> +
> + /* HDCP2.2 related definitions */
> +
> + /* Content Stream Type defined by content owner */
> + u8 content_type;
> +
> + bool is_paired;
> + bool is_repeater;
> +
> + /*
> + * Count of ReceiverID_List received. Initialized to 0 at AKE_INIT.
> + * Incremented after processing the RepeaterAuth_Send_ReceiverID_List.
> + * When it rolls over re-auth has to be triggered.
> + */
> + uint32_t seq_num_v;
> +
> + /*
> + * Count of RepeaterAuth_Stream_Manage msg propagated.
> + * Initialized to 0 on AKE_INIT. Incremented after every successful
> + * transmission of RepeaterAuth_Stream_Manage message. When it rolls
> + * over re-Auth has to be triggered.
> + */
> + uint32_t seq_num_m;
> +
> + /* Translation layer for encoder specific functions */
> + const struct intel_hdcp2_shim *hdcp2_shim;
> +
> + /* mei interface related information */
> + struct mei_hdcp_data *mei_hdcp_data;
> +
> + struct delayed_work hdcp2_check_work;
> };
>
> struct intel_connector {
> --
> 2.7.4
>

--
Sean Paul, Software Engineer, Google / Chromium OS