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

[Sean Paul <seanpaul@chromium.org>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx