[v9,10/39] drm/i915: Implement HDCP2.2 receiver authentication

Submitted by Ramalingam C on Dec. 13, 2018, 4:01 a.m.

Details

Message ID 1544673701-6353-11-git-send-email-ramalingam.c@intel.com
State New
Headers show
Series "drm/i915: Implement HDCP2.2" ( rev: 12 11 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ramalingam C Dec. 13, 2018, 4:01 a.m.
Implements HDCP2.2 authentication for hdcp2.2 receivers, with
following steps:
	Authentication and Key exchange (AKE).
	Locality Check (LC).
	Session Key Exchange(SKE).
	DP Errata for stream type configuration for receivers.

At AKE, the HDCP Receiver’s public key certificate is verified by the
HDCP Transmitter. A Master Key k m is exchanged.

At LC, the HDCP Transmitter enforces locality on the content by
requiring that the Round Trip Time (RTT) between a pair of messages
is not more than 20 ms.

At SKE, The HDCP Transmitter exchanges Session Key ks with
the HDCP Receiver.

In DP HDCP2.2 encryption and decryption logics use the stream type as
one of the parameter. So Before enabling the Encryption DP HDCP2.2
receiver needs to be communicated with stream type. This is added to
spec as ERRATA.

This generic implementation is complete only with the hdcp2 specific
functions defined at hdcp_shim.

v2: Rebased.
v3:
  %s/PARING/PAIRING
  Coding style fixing [Uma]
v4:
  Rebased as part of patch reordering.
  Defined the functions for mei services. [Daniel]
v5:
  Redefined the mei service functions as per comp redesign.
  Required intel_hdcp members are defined [Sean Paul]
v6:
  Typo of cipher is Fixed [Uma]
  %s/uintxx_t/uxx
  Check for comp_master is removed.
v7:
  Adjust to the new interface.
  Avoid using bool structure members. [Tomas]
v8: Rebased.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |  34 ++++++
 drivers/gpu/drm/i915/intel_hdcp.c | 211 +++++++++++++++++++++++++++++++++++---
 2 files changed, 230 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6d5361616ca3..a6b632d71490 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -387,6 +387,22 @@  struct intel_hdcp_shim {
 	/* Detects whether Panel is HDCP2.2 capable */
 	int (*hdcp_2_2_capable)(struct intel_digital_port *intel_dig_port,
 				bool *capable);
+
+	/* Write HDCP2.2 messages */
+	int (*write_2_2_msg)(struct intel_digital_port *intel_dig_port,
+			     void *buf, size_t size);
+
+	/* Read HDCP2.2 messages */
+	int (*read_2_2_msg)(struct intel_digital_port *intel_dig_port,
+			    u8 msg_id, void *buf, size_t size);
+
+	/*
+	 * Implementation of DP HDCP2.2 Errata for the communication of stream
+	 * type to Receivers. In DP HDCP2.2 Stream type is one of the input to
+	 * the HDCP2.2 Cipher for En/De-Cryption. Not applicable for HDMI.
+	 */
+	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
+				  void *buf, size_t size);
 };
 
 struct intel_hdcp {
@@ -414,6 +430,24 @@  struct intel_hdcp {
 	 */
 	u8 content_type;
 	struct hdcp_port_data port_data;
+
+	u8 is_paired;
+	u8 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.
+	 */
+	u32 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.
+	 */
+	u32 seq_num_m;
 };
 
 struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index f0ee448e6546..f1f0ef294e20 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -18,6 +18,7 @@ 
 
 #define KEY_LOAD_TRIES	5
 #define ENCRYPT_STATUS_CHANGE_TIMEOUT_MS	50
+#define HDCP2_LC_RETRY_CNT			3
 
 static
 bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -854,7 +855,7 @@  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
 	return INTEL_GEN(dev_priv) >= 9 && port < PORT_E;
 }
 
-static __attribute__((unused)) int
+static int
 hdcp2_prepare_ake_init(struct intel_connector *connector,
 		       struct hdcp2_ake_init *ake_data)
 {
@@ -875,7 +876,7 @@  hdcp2_prepare_ake_init(struct intel_connector *connector,
 	return ret;
 }
 
-static __attribute__((unused)) int
+static int
 hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
 				struct hdcp2_ake_send_cert *rx_cert,
 				bool *paired,
@@ -897,9 +898,8 @@  hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
 	return ret;
 }
 
-static __attribute__((unused)) int
-hdcp2_verify_hprime(struct intel_connector *connector,
-		    struct hdcp2_ake_send_hprime *rx_hprime)
+static int hdcp2_verify_hprime(struct intel_connector *connector,
+			       struct hdcp2_ake_send_hprime *rx_hprime)
 {
 	struct hdcp_port_data *data = &connector->hdcp.port_data;
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -913,7 +913,7 @@  hdcp2_verify_hprime(struct intel_connector *connector,
 	return ret;
 }
 
-static __attribute__((unused)) int
+static int
 hdcp2_store_pairing_info(struct intel_connector *connector,
 			 struct hdcp2_ake_send_pairing_info *pairing_info)
 {
@@ -930,7 +930,7 @@  hdcp2_store_pairing_info(struct intel_connector *connector,
 	return ret;
 }
 
-static __attribute__((unused)) int
+static int
 hdcp2_prepare_lc_init(struct intel_connector *connector,
 		      struct hdcp2_lc_init *lc_init)
 {
@@ -947,7 +947,7 @@  hdcp2_prepare_lc_init(struct intel_connector *connector,
 	return ret;
 }
 
-static __attribute__((unused)) int
+static int
 hdcp2_verify_lprime(struct intel_connector *connector,
 		    struct hdcp2_lc_send_lprime *rx_lprime)
 {
@@ -963,9 +963,8 @@  hdcp2_verify_lprime(struct intel_connector *connector,
 	return ret;
 }
 
-static __attribute__((unused))
-int hdcp2_prepare_skey(struct intel_connector *connector,
-		       struct hdcp2_ske_send_eks *ske_data)
+static int hdcp2_prepare_skey(struct intel_connector *connector,
+			      struct hdcp2_ske_send_eks *ske_data)
 {
 	struct hdcp_port_data *data = &connector->hdcp.port_data;
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1016,8 +1015,7 @@  hdcp2_verify_mprime(struct intel_connector *connector,
 	return ret;
 }
 
-static __attribute__((unused))
-int hdcp2_authenticate_port(struct intel_connector *connector)
+static int hdcp2_authenticate_port(struct intel_connector *connector)
 {
 	struct hdcp_port_data *data = &connector->hdcp.port_data;
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
@@ -1045,11 +1043,194 @@  static int hdcp2_deauthenticate_port(struct intel_connector *connector)
 	return hdcp2_close_mei_session(connector);
 }
 
+/* Authentication flow starts from here */
+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;
+	union {
+		struct hdcp2_ake_init ake_init;
+		struct hdcp2_ake_send_cert send_cert;
+		struct hdcp2_ake_no_stored_km no_stored_km;
+		struct hdcp2_ake_send_hprime send_hprime;
+		struct hdcp2_ake_send_pairing_info pairing_info;
+	} msgs;
+	const struct intel_hdcp_shim *shim = hdcp->shim;
+	size_t size;
+	int ret;
+	bool is_paired;
+
+	/* Init for seq_num */
+	hdcp->seq_num_v = 0;
+	hdcp->seq_num_m = 0;
+
+	ret = hdcp2_prepare_ake_init(connector, &msgs.ake_init);
+	if (ret < 0)
+		return ret;
+
+	ret = shim->write_2_2_msg(intel_dig_port, &msgs.ake_init,
+				  sizeof(msgs.ake_init));
+	if (ret < 0)
+		return ret;
+
+	ret = shim->read_2_2_msg(intel_dig_port, HDCP_2_2_AKE_SEND_CERT,
+				 &msgs.send_cert, sizeof(msgs.send_cert));
+	if (ret < 0)
+		return ret;
+
+	if (msgs.send_cert.rx_caps[0] != HDCP_2_2_RX_CAPS_VERSION_VAL)
+		return -EINVAL;
+
+	hdcp->is_repeater = HDCP_2_2_RX_REPEATER(msgs.send_cert.rx_caps[2]) ?
+			    1 : 0;
+
+	/*
+	 * Here msgs.no_stored_km will hold msgs corresponding to the km
+	 * stored also.
+	 */
+	ret = hdcp2_verify_rx_cert_prepare_km(connector, &msgs.send_cert,
+					      &is_paired,
+					      &msgs.no_stored_km, &size);
+	if (ret < 0)
+		return ret;
+
+	hdcp->is_paired = is_paired ? 1 : 0;
+
+	ret = shim->write_2_2_msg(intel_dig_port, &msgs.no_stored_km, size);
+	if (ret < 0)
+		return ret;
+
+	ret = shim->read_2_2_msg(intel_dig_port, HDCP_2_2_AKE_SEND_HPRIME,
+				 &msgs.send_hprime, sizeof(msgs.send_hprime));
+	if (ret < 0)
+		return ret;
+
+	ret = hdcp2_verify_hprime(connector, &msgs.send_hprime);
+	if (ret < 0)
+		return ret;
+
+	if (!hdcp->is_paired) {
+		/* Pairing is required */
+		ret = shim->read_2_2_msg(intel_dig_port,
+					 HDCP_2_2_AKE_SEND_PAIRING_INFO,
+					 &msgs.pairing_info,
+					 sizeof(msgs.pairing_info));
+		if (ret < 0)
+			return ret;
+
+		ret = hdcp2_store_pairing_info(connector, &msgs.pairing_info);
+		if (ret < 0)
+			return ret;
+		hdcp->is_paired = 1;
+	}
+
+	return 0;
+}
+
+static int hdcp2_locality_check(struct intel_connector *connector)
+{
+	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
+	struct intel_hdcp *hdcp = &connector->hdcp;
+	union {
+		struct hdcp2_lc_init lc_init;
+		struct hdcp2_lc_send_lprime send_lprime;
+	} msgs;
+	const struct intel_hdcp_shim *shim = hdcp->shim;
+	int tries = HDCP2_LC_RETRY_CNT, ret, i;
+
+	for (i = 0; i < tries; i++) {
+		ret = hdcp2_prepare_lc_init(connector, &msgs.lc_init);
+		if (ret < 0)
+			continue;
+
+		ret = shim->write_2_2_msg(intel_dig_port, &msgs.lc_init,
+				      sizeof(msgs.lc_init));
+		if (ret < 0)
+			continue;
+
+		ret = shim->read_2_2_msg(intel_dig_port,
+					 HDCP_2_2_LC_SEND_LPRIME,
+					 &msgs.send_lprime,
+					 sizeof(msgs.send_lprime));
+		if (ret < 0)
+			continue;
+
+		ret = hdcp2_verify_lprime(connector, &msgs.send_lprime);
+		if (!ret)
+			break;
+	}
+
+	return ret;
+}
+
+static int hdcp2_session_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 hdcp2_ske_send_eks send_eks;
+	int ret;
+
+	ret = hdcp2_prepare_skey(connector, &send_eks);
+	if (ret < 0)
+		return ret;
+
+	ret = hdcp->shim->write_2_2_msg(intel_dig_port, &send_eks,
+					sizeof(send_eks));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int hdcp2_authenticate_sink(struct intel_connector *connector)
 {
-	DRM_ERROR("Sink authentication is done in subsequent patches\n");
+	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
+	struct intel_hdcp *hdcp = &connector->hdcp;
+	const struct intel_hdcp_shim *shim = hdcp->shim;
+	struct hdcp2_dp_errata_stream_type stream_type_msg;
+	int ret;
 
-	return -EINVAL;
+	ret = hdcp2_authentication_key_exchange(connector);
+	if (ret < 0) {
+		DRM_DEBUG_KMS("AKE Failed. Err : %d\n", ret);
+		return ret;
+	}
+
+	ret = hdcp2_locality_check(connector);
+	if (ret < 0) {
+		DRM_DEBUG_KMS("Locality Check failed. Err : %d\n", ret);
+		return ret;
+	}
+
+	ret = hdcp2_session_key_exchange(connector);
+	if (ret < 0) {
+		DRM_DEBUG_KMS("SKE Failed. Err : %d\n", ret);
+		return ret;
+	}
+
+	if (!hdcp->is_repeater && shim->config_stream_type) {
+		/*
+		 * Errata for DP: As Stream type is used for encryption,
+		 * Receiver should be communicated with stream type for the
+		 * decryption of the content.
+		 * Repeater will be communicated with stream type as a
+		 * part of it's auth later in time.
+		 */
+		stream_type_msg.msg_id = HDCP_2_2_ERRATA_DP_STREAM_TYPE;
+		stream_type_msg.stream_type = hdcp->content_type;
+
+		ret = shim->config_stream_type(intel_dig_port, &stream_type_msg,
+					       sizeof(stream_type_msg));
+		if (ret < 0)
+			return ret;
+	}
+
+	hdcp->port_data.streams[0].stream_type = hdcp->content_type;
+	ret = hdcp2_authenticate_port(connector);
+	if (ret < 0)
+		return ret;
+
+	return ret;
 }
 
 static int hdcp2_enable_encryption(struct intel_connector *connector)

Comments

On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> Implements HDCP2.2 authentication for hdcp2.2 receivers, with
> following steps:
> 	Authentication and Key exchange (AKE).
> 	Locality Check (LC).
> 	Session Key Exchange(SKE).
> 	DP Errata for stream type configuration for receivers.
> 
> At AKE, the HDCP Receiver’s public key certificate is verified by the
> HDCP Transmitter. A Master Key k m is exchanged.
> 
> At LC, the HDCP Transmitter enforces locality on the content by
> requiring that the Round Trip Time (RTT) between a pair of messages
> is not more than 20 ms.
> 
> At SKE, The HDCP Transmitter exchanges Session Key ks with
> the HDCP Receiver.
> 
> In DP HDCP2.2 encryption and decryption logics use the stream type as
> one of the parameter. So Before enabling the Encryption DP HDCP2.2
> receiver needs to be communicated with stream type. This is added to
> spec as ERRATA.
> 
> This generic implementation is complete only with the hdcp2 specific
> functions defined at hdcp_shim.
> 
> v2: Rebased.
> v3:
>   %s/PARING/PAIRING
>   Coding style fixing [Uma]
> v4:
>   Rebased as part of patch reordering.
>   Defined the functions for mei services. [Daniel]
> v5:
>   Redefined the mei service functions as per comp redesign.
>   Required intel_hdcp members are defined [Sean Paul]
> v6:
>   Typo of cipher is Fixed [Uma]
>   %s/uintxx_t/uxx
>   Check for comp_master is removed.
> v7:
>   Adjust to the new interface.
>   Avoid using bool structure members. [Tomas]
> v8: Rebased.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  34 ++++++
>  drivers/gpu/drm/i915/intel_hdcp.c | 211 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 230 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6d5361616ca3..a6b632d71490 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -387,6 +387,22 @@ struct intel_hdcp_shim {
>  	/* Detects whether Panel is HDCP2.2 capable */
>  	int (*hdcp_2_2_capable)(struct intel_digital_port *intel_dig_port,
>  				bool *capable);
> +
> +	/* Write HDCP2.2 messages */
> +	int (*write_2_2_msg)(struct intel_digital_port *intel_dig_port,
> +			     void *buf, size_t size);
> +
> +	/* Read HDCP2.2 messages */
> +	int (*read_2_2_msg)(struct intel_digital_port *intel_dig_port,
> +			    u8 msg_id, void *buf, size_t size);
> +
> +	/*
> +	 * Implementation of DP HDCP2.2 Errata for the communication of stream
> +	 * type to Receivers. In DP HDCP2.2 Stream type is one of the input to
> +	 * the HDCP2.2 Cipher for En/De-Cryption. Not applicable for HDMI.
> +	 */
> +	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
> +				  void *buf, size_t size);
>  };
>  
>  struct intel_hdcp {
> @@ -414,6 +430,24 @@ struct intel_hdcp {
>  	 */
>  	u8 content_type;
>  	struct hdcp_port_data port_data;
> +
> +	u8 is_paired;
> +	u8 is_repeater;

Make these two bool, will simplify the code a bunch.

> +
> +	/*
> +	 * 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.
> +	 */
> +	u32 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.
> +	 */
> +	u32 seq_num_m;
>  };
>  
>  struct intel_connector {
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index f0ee448e6546..f1f0ef294e20 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -18,6 +18,7 @@
>  
>  #define KEY_LOAD_TRIES	5
>  #define ENCRYPT_STATUS_CHANGE_TIMEOUT_MS	50
> +#define HDCP2_LC_RETRY_CNT			3
>  
>  static
>  bool intel_hdcp_is_ksv_valid(u8 *ksv)
> @@ -854,7 +855,7 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>  	return INTEL_GEN(dev_priv) >= 9 && port < PORT_E;
>  }
>  
> -static __attribute__((unused)) int
> +static int
>  hdcp2_prepare_ake_init(struct intel_connector *connector,
>  		       struct hdcp2_ake_init *ake_data)
>  {
> @@ -875,7 +876,7 @@ hdcp2_prepare_ake_init(struct intel_connector *connector,
>  	return ret;
>  }
>  
> -static __attribute__((unused)) int
> +static int
>  hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>  				struct hdcp2_ake_send_cert *rx_cert,
>  				bool *paired,
> @@ -897,9 +898,8 @@ hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>  	return ret;
>  }
>  
> -static __attribute__((unused)) int
> -hdcp2_verify_hprime(struct intel_connector *connector,
> -		    struct hdcp2_ake_send_hprime *rx_hprime)
> +static int hdcp2_verify_hprime(struct intel_connector *connector,
> +			       struct hdcp2_ake_send_hprime *rx_hprime)
>  {
>  	struct hdcp_port_data *data = &connector->hdcp.port_data;
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -913,7 +913,7 @@ hdcp2_verify_hprime(struct intel_connector *connector,
>  	return ret;
>  }
>  
> -static __attribute__((unused)) int
> +static int
>  hdcp2_store_pairing_info(struct intel_connector *connector,
>  			 struct hdcp2_ake_send_pairing_info *pairing_info)
>  {
> @@ -930,7 +930,7 @@ hdcp2_store_pairing_info(struct intel_connector *connector,
>  	return ret;
>  }
>  
> -static __attribute__((unused)) int
> +static int
>  hdcp2_prepare_lc_init(struct intel_connector *connector,
>  		      struct hdcp2_lc_init *lc_init)
>  {
> @@ -947,7 +947,7 @@ hdcp2_prepare_lc_init(struct intel_connector *connector,
>  	return ret;
>  }
>  
> -static __attribute__((unused)) int
> +static int
>  hdcp2_verify_lprime(struct intel_connector *connector,
>  		    struct hdcp2_lc_send_lprime *rx_lprime)
>  {
> @@ -963,9 +963,8 @@ hdcp2_verify_lprime(struct intel_connector *connector,
>  	return ret;
>  }
>  
> -static __attribute__((unused))
> -int hdcp2_prepare_skey(struct intel_connector *connector,
> -		       struct hdcp2_ske_send_eks *ske_data)
> +static int hdcp2_prepare_skey(struct intel_connector *connector,
> +			      struct hdcp2_ske_send_eks *ske_data)
>  {
>  	struct hdcp_port_data *data = &connector->hdcp.port_data;
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1016,8 +1015,7 @@ hdcp2_verify_mprime(struct intel_connector *connector,
>  	return ret;
>  }
>  
> -static __attribute__((unused))
> -int hdcp2_authenticate_port(struct intel_connector *connector)
> +static int hdcp2_authenticate_port(struct intel_connector *connector)
>  {
>  	struct hdcp_port_data *data = &connector->hdcp.port_data;
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> @@ -1045,11 +1043,194 @@ static int hdcp2_deauthenticate_port(struct intel_connector *connector)
>  	return hdcp2_close_mei_session(connector);
>  }
>  
> +/* Authentication flow starts from here */
> +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;
> +	union {
> +		struct hdcp2_ake_init ake_init;
> +		struct hdcp2_ake_send_cert send_cert;
> +		struct hdcp2_ake_no_stored_km no_stored_km;
> +		struct hdcp2_ake_send_hprime send_hprime;
> +		struct hdcp2_ake_send_pairing_info pairing_info;
> +	} msgs;
> +	const struct intel_hdcp_shim *shim = hdcp->shim;
> +	size_t size;
> +	int ret;
> +	bool is_paired;
> +
> +	/* Init for seq_num */
> +	hdcp->seq_num_v = 0;
> +	hdcp->seq_num_m = 0;
> +
> +	ret = hdcp2_prepare_ake_init(connector, &msgs.ake_init);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = shim->write_2_2_msg(intel_dig_port, &msgs.ake_init,
> +				  sizeof(msgs.ake_init));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = shim->read_2_2_msg(intel_dig_port, HDCP_2_2_AKE_SEND_CERT,
> +				 &msgs.send_cert, sizeof(msgs.send_cert));
> +	if (ret < 0)
> +		return ret;

sink should give us the initial key with 100ms. I guess there's going to
be a patch later on to check that?

> +
> +	if (msgs.send_cert.rx_caps[0] != HDCP_2_2_RX_CAPS_VERSION_VAL)
> +		return -EINVAL;
> +
> +	hdcp->is_repeater = HDCP_2_2_RX_REPEATER(msgs.send_cert.rx_caps[2]) ?
> +			    1 : 0;

If you make is_repeater of type bool you can simplify this.

> +
> +	/*
> +	 * Here msgs.no_stored_km will hold msgs corresponding to the km
> +	 * stored also.
> +	 */
> +	ret = hdcp2_verify_rx_cert_prepare_km(connector, &msgs.send_cert,
> +					      &is_paired,
> +					      &msgs.no_stored_km, &size);
> +	if (ret < 0)
> +		return ret;
> +
> +	hdcp->is_paired = is_paired ? 1 : 0;

The ? 1 : 0 is redundant if you make is_paired type bool.
> +
> +	ret = shim->write_2_2_msg(intel_dig_port, &msgs.no_stored_km, size);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = shim->read_2_2_msg(intel_dig_port, HDCP_2_2_AKE_SEND_HPRIME,
> +				 &msgs.send_hprime, sizeof(msgs.send_hprime));
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hdcp2_verify_hprime(connector, &msgs.send_hprime);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!hdcp->is_paired) {
> +		/* Pairing is required */
> +		ret = shim->read_2_2_msg(intel_dig_port,
> +					 HDCP_2_2_AKE_SEND_PAIRING_INFO,
> +					 &msgs.pairing_info,
> +					 sizeof(msgs.pairing_info));
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = hdcp2_store_pairing_info(connector, &msgs.pairing_info);
> +		if (ret < 0)
> +			return ret;
> +		hdcp->is_paired = 1;

s/1/true/

> +	}
> +
> +	return 0;
> +}
> +
> +static int hdcp2_locality_check(struct intel_connector *connector)
> +{
> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> +	struct intel_hdcp *hdcp = &connector->hdcp;
> +	union {
> +		struct hdcp2_lc_init lc_init;
> +		struct hdcp2_lc_send_lprime send_lprime;
> +	} msgs;
> +	const struct intel_hdcp_shim *shim = hdcp->shim;
> +	int tries = HDCP2_LC_RETRY_CNT, ret, i;
> +
> +	for (i = 0; i < tries; i++) {
> +		ret = hdcp2_prepare_lc_init(connector, &msgs.lc_init);
> +		if (ret < 0)
> +			continue;
> +
> +		ret = shim->write_2_2_msg(intel_dig_port, &msgs.lc_init,
> +				      sizeof(msgs.lc_init));
> +		if (ret < 0)
> +			continue;
> +
> +		ret = shim->read_2_2_msg(intel_dig_port,
> +					 HDCP_2_2_LC_SEND_LPRIME,
> +					 &msgs.send_lprime,
> +					 sizeof(msgs.send_lprime));
> +		if (ret < 0)
> +			continue;
> +
> +		ret = hdcp2_verify_lprime(connector, &msgs.send_lprime);
> +		if (!ret)
> +			break;

We're not checking that the sink replies quickly enough here. Sink needs
to reply in 20ms. Can be a followup I guess.

Also just noticed that we seem to lack all these timing checks for hdcp1
too, at least we don't fail auth if the sink takes too long.

> +	}
> +
> +	return ret;
> +}
> +
> +static int hdcp2_session_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 hdcp2_ske_send_eks send_eks;
> +	int ret;
> +
> +	ret = hdcp2_prepare_skey(connector, &send_eks);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = hdcp->shim->write_2_2_msg(intel_dig_port, &send_eks,
> +					sizeof(send_eks));
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int hdcp2_authenticate_sink(struct intel_connector *connector)
>  {
> -	DRM_ERROR("Sink authentication is done in subsequent patches\n");
> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> +	struct intel_hdcp *hdcp = &connector->hdcp;
> +	const struct intel_hdcp_shim *shim = hdcp->shim;
> +	struct hdcp2_dp_errata_stream_type stream_type_msg;
> +	int ret;
>  
> -	return -EINVAL;
> +	ret = hdcp2_authentication_key_exchange(connector);
> +	if (ret < 0) {
> +		DRM_DEBUG_KMS("AKE Failed. Err : %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hdcp2_locality_check(connector);
> +	if (ret < 0) {
> +		DRM_DEBUG_KMS("Locality Check failed. Err : %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hdcp2_session_key_exchange(connector);
> +	if (ret < 0) {
> +		DRM_DEBUG_KMS("SKE Failed. Err : %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!hdcp->is_repeater && shim->config_stream_type) {
> +		/*
> +		 * Errata for DP: As Stream type is used for encryption,
> +		 * Receiver should be communicated with stream type for the
> +		 * decryption of the content.
> +		 * Repeater will be communicated with stream type as a
> +		 * part of it's auth later in time.
> +		 */

I'm not following what you want to say with this comment, and haven't
found anything in the hdcp2 dp spec about this either.

> +		stream_type_msg.msg_id = HDCP_2_2_ERRATA_DP_STREAM_TYP:E;
> +		stream_type_msg.stream_type = hdcp->content_type;
> +
> +		ret = shim->config_stream_type(intel_dig_port, &stream_type_msg,
> +					       sizeof(stream_type_msg));
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	hdcp->port_data.streams[0].stream_type = hdcp->content_type;
> +	ret = hdcp2_authenticate_port(connector);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret;
>  }

With thy types changed to bool and the dp errata cleared up somehow:

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

>  
>  static int hdcp2_enable_encryption(struct intel_connector *connector)
> -- 
> 2.7.4
>
On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
>> Implements HDCP2.2 authentication for hdcp2.2 receivers, with
>> following steps:
>> 	Authentication and Key exchange (AKE).
>> 	Locality Check (LC).
>> 	Session Key Exchange(SKE).
>> 	DP Errata for stream type configuration for receivers.
>>
>> At AKE, the HDCP Receiver’s public key certificate is verified by the
>> HDCP Transmitter. A Master Key k m is exchanged.
>>
>> At LC, the HDCP Transmitter enforces locality on the content by
>> requiring that the Round Trip Time (RTT) between a pair of messages
>> is not more than 20 ms.
>>
>> At SKE, The HDCP Transmitter exchanges Session Key ks with
>> the HDCP Receiver.
>>
>> In DP HDCP2.2 encryption and decryption logics use the stream type as
>> one of the parameter. So Before enabling the Encryption DP HDCP2.2
>> receiver needs to be communicated with stream type. This is added to
>> spec as ERRATA.
>>
>> This generic implementation is complete only with the hdcp2 specific
>> functions defined at hdcp_shim.
>>
>> v2: Rebased.
>> v3:
>>    %s/PARING/PAIRING
>>    Coding style fixing [Uma]
>> v4:
>>    Rebased as part of patch reordering.
>>    Defined the functions for mei services. [Daniel]
>> v5:
>>    Redefined the mei service functions as per comp redesign.
>>    Required intel_hdcp members are defined [Sean Paul]
>> v6:
>>    Typo of cipher is Fixed [Uma]
>>    %s/uintxx_t/uxx
>>    Check for comp_master is removed.
>> v7:
>>    Adjust to the new interface.
>>    Avoid using bool structure members. [Tomas]
>> v8: Rebased.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_drv.h  |  34 ++++++
>>   drivers/gpu/drm/i915/intel_hdcp.c | 211 +++++++++++++++++++++++++++++++++++---
>>   2 files changed, 230 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 6d5361616ca3..a6b632d71490 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -387,6 +387,22 @@ struct intel_hdcp_shim {
>>   	/* Detects whether Panel is HDCP2.2 capable */
>>   	int (*hdcp_2_2_capable)(struct intel_digital_port *intel_dig_port,
>>   				bool *capable);
>> +
>> +	/* Write HDCP2.2 messages */
>> +	int (*write_2_2_msg)(struct intel_digital_port *intel_dig_port,
>> +			     void *buf, size_t size);
>> +
>> +	/* Read HDCP2.2 messages */
>> +	int (*read_2_2_msg)(struct intel_digital_port *intel_dig_port,
>> +			    u8 msg_id, void *buf, size_t size);
>> +
>> +	/*
>> +	 * Implementation of DP HDCP2.2 Errata for the communication of stream
>> +	 * type to Receivers. In DP HDCP2.2 Stream type is one of the input to
>> +	 * the HDCP2.2 Cipher for En/De-Cryption. Not applicable for HDMI.
>> +	 */
>> +	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
>> +				  void *buf, size_t size);
>>   };
>>   
>>   struct intel_hdcp {
>> @@ -414,6 +430,24 @@ struct intel_hdcp {
>>   	 */
>>   	u8 content_type;
>>   	struct hdcp_port_data port_data;
>> +
>> +	u8 is_paired;
>> +	u8 is_repeater;
> Make these two bool, will simplify the code a bunch.

Seems there is a movement for not to use the bool in structures.
Thats why I have changed these from bool to u8 from v8 onwards. Checkpatch also complains on this

>
>> +
>> +	/*
>> +	 * 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.
>> +	 */
>> +	u32 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.
>> +	 */
>> +	u32 seq_num_m;
>>   };
>>   
>>   struct intel_connector {
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index f0ee448e6546..f1f0ef294e20 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -18,6 +18,7 @@
>>   
>>   #define KEY_LOAD_TRIES	5
>>   #define ENCRYPT_STATUS_CHANGE_TIMEOUT_MS	50
>> +#define HDCP2_LC_RETRY_CNT			3
>>   
>>   static
>>   bool intel_hdcp_is_ksv_valid(u8 *ksv)
>> @@ -854,7 +855,7 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>>   	return INTEL_GEN(dev_priv) >= 9 && port < PORT_E;
>>   }
>>   
>> -static __attribute__((unused)) int
>> +static int
>>   hdcp2_prepare_ake_init(struct intel_connector *connector,
>>   		       struct hdcp2_ake_init *ake_data)
>>   {
>> @@ -875,7 +876,7 @@ hdcp2_prepare_ake_init(struct intel_connector *connector,
>>   	return ret;
>>   }
>>   
>> -static __attribute__((unused)) int
>> +static int
>>   hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>>   				struct hdcp2_ake_send_cert *rx_cert,
>>   				bool *paired,
>> @@ -897,9 +898,8 @@ hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>>   	return ret;
>>   }
>>   
>> -static __attribute__((unused)) int
>> -hdcp2_verify_hprime(struct intel_connector *connector,
>> -		    struct hdcp2_ake_send_hprime *rx_hprime)
>> +static int hdcp2_verify_hprime(struct intel_connector *connector,
>> +			       struct hdcp2_ake_send_hprime *rx_hprime)
>>   {
>>   	struct hdcp_port_data *data = &connector->hdcp.port_data;
>>   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> @@ -913,7 +913,7 @@ hdcp2_verify_hprime(struct intel_connector *connector,
>>   	return ret;
>>   }
>>   
>> -static __attribute__((unused)) int
>> +static int
>>   hdcp2_store_pairing_info(struct intel_connector *connector,
>>   			 struct hdcp2_ake_send_pairing_info *pairing_info)
>>   {
>> @@ -930,7 +930,7 @@ hdcp2_store_pairing_info(struct intel_connector *connector,
>>   	return ret;
>>   }
>>   
>> -static __attribute__((unused)) int
>> +static int
>>   hdcp2_prepare_lc_init(struct intel_connector *connector,
>>   		      struct hdcp2_lc_init *lc_init)
>>   {
>> @@ -947,7 +947,7 @@ hdcp2_prepare_lc_init(struct intel_connector *connector,
>>   	return ret;
>>   }
>>   
>> -static __attribute__((unused)) int
>> +static int
>>   hdcp2_verify_lprime(struct intel_connector *connector,
>>   		    struct hdcp2_lc_send_lprime *rx_lprime)
>>   {
>> @@ -963,9 +963,8 @@ hdcp2_verify_lprime(struct intel_connector *connector,
>>   	return ret;
>>   }
>>   
>> -static __attribute__((unused))
>> -int hdcp2_prepare_skey(struct intel_connector *connector,
>> -		       struct hdcp2_ske_send_eks *ske_data)
>> +static int hdcp2_prepare_skey(struct intel_connector *connector,
>> +			      struct hdcp2_ske_send_eks *ske_data)
>>   {
>>   	struct hdcp_port_data *data = &connector->hdcp.port_data;
>>   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> @@ -1016,8 +1015,7 @@ hdcp2_verify_mprime(struct intel_connector *connector,
>>   	return ret;
>>   }
>>   
>> -static __attribute__((unused))
>> -int hdcp2_authenticate_port(struct intel_connector *connector)
>> +static int hdcp2_authenticate_port(struct intel_connector *connector)
>>   {
>>   	struct hdcp_port_data *data = &connector->hdcp.port_data;
>>   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> @@ -1045,11 +1043,194 @@ static int hdcp2_deauthenticate_port(struct intel_connector *connector)
>>   	return hdcp2_close_mei_session(connector);
>>   }
>>   
>> +/* Authentication flow starts from here */
>> +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;
>> +	union {
>> +		struct hdcp2_ake_init ake_init;
>> +		struct hdcp2_ake_send_cert send_cert;
>> +		struct hdcp2_ake_no_stored_km no_stored_km;
>> +		struct hdcp2_ake_send_hprime send_hprime;
>> +		struct hdcp2_ake_send_pairing_info pairing_info;
>> +	} msgs;
>> +	const struct intel_hdcp_shim *shim = hdcp->shim;
>> +	size_t size;
>> +	int ret;
>> +	bool is_paired;
>> +
>> +	/* Init for seq_num */
>> +	hdcp->seq_num_v = 0;
>> +	hdcp->seq_num_m = 0;
>> +
>> +	ret = hdcp2_prepare_ake_init(connector, &msgs.ake_init);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = shim->write_2_2_msg(intel_dig_port, &msgs.ake_init,
>> +				  sizeof(msgs.ake_init));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = shim->read_2_2_msg(intel_dig_port, HDCP_2_2_AKE_SEND_CERT,
>> +				 &msgs.send_cert, sizeof(msgs.send_cert));
>> +	if (ret < 0)
>> +		return ret;
> sink should give us the initial key with 100ms. I guess there's going to
> be a patch later on to check that?
Timings are checked at read_2_2_msg as per the msg_id
>
>> +
>> +	if (msgs.send_cert.rx_caps[0] != HDCP_2_2_RX_CAPS_VERSION_VAL)
>> +		return -EINVAL;
>> +
>> +	hdcp->is_repeater = HDCP_2_2_RX_REPEATER(msgs.send_cert.rx_caps[2]) ?
>> +			    1 : 0;
> If you make is_repeater of type bool you can simplify this.
Yes. But it is advised to not to use bool in struct
>
>> +
>> +	/*
>> +	 * Here msgs.no_stored_km will hold msgs corresponding to the km
>> +	 * stored also.
>> +	 */
>> +	ret = hdcp2_verify_rx_cert_prepare_km(connector, &msgs.send_cert,
>> +					      &is_paired,
>> +					      &msgs.no_stored_km, &size);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	hdcp->is_paired = is_paired ? 1 : 0;
> The ? 1 : 0 is redundant if you make is_paired type bool.
>> +
>> +	ret = shim->write_2_2_msg(intel_dig_port, &msgs.no_stored_km, size);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = shim->read_2_2_msg(intel_dig_port, HDCP_2_2_AKE_SEND_HPRIME,
>> +				 &msgs.send_hprime, sizeof(msgs.send_hprime));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = hdcp2_verify_hprime(connector, &msgs.send_hprime);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (!hdcp->is_paired) {
>> +		/* Pairing is required */
>> +		ret = shim->read_2_2_msg(intel_dig_port,
>> +					 HDCP_2_2_AKE_SEND_PAIRING_INFO,
>> +					 &msgs.pairing_info,
>> +					 sizeof(msgs.pairing_info));
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = hdcp2_store_pairing_info(connector, &msgs.pairing_info);
>> +		if (ret < 0)
>> +			return ret;
>> +		hdcp->is_paired = 1;
> s/1/true/
>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int hdcp2_locality_check(struct intel_connector *connector)
>> +{
>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>> +	struct intel_hdcp *hdcp = &connector->hdcp;
>> +	union {
>> +		struct hdcp2_lc_init lc_init;
>> +		struct hdcp2_lc_send_lprime send_lprime;
>> +	} msgs;
>> +	const struct intel_hdcp_shim *shim = hdcp->shim;
>> +	int tries = HDCP2_LC_RETRY_CNT, ret, i;
>> +
>> +	for (i = 0; i < tries; i++) {
>> +		ret = hdcp2_prepare_lc_init(connector, &msgs.lc_init);
>> +		if (ret < 0)
>> +			continue;
>> +
>> +		ret = shim->write_2_2_msg(intel_dig_port, &msgs.lc_init,
>> +				      sizeof(msgs.lc_init));
>> +		if (ret < 0)
>> +			continue;
>> +
>> +		ret = shim->read_2_2_msg(intel_dig_port,
>> +					 HDCP_2_2_LC_SEND_LPRIME,
>> +					 &msgs.send_lprime,
>> +					 sizeof(msgs.send_lprime));
>> +		if (ret < 0)
>> +			continue;
>> +
>> +		ret = hdcp2_verify_lprime(connector, &msgs.send_lprime);
>> +		if (!ret)
>> +			break;
> We're not checking that the sink replies quickly enough here. Sink needs
> to reply in 20ms. Can be a followup I guess.
>
> Also just noticed that we seem to lack all these timing checks for hdcp1
> too, at least we don't fail auth if the sink takes too long.
Shim takes care of it. It we dont compliance will fail.
>
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int hdcp2_session_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 hdcp2_ske_send_eks send_eks;
>> +	int ret;
>> +
>> +	ret = hdcp2_prepare_skey(connector, &send_eks);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = hdcp->shim->write_2_2_msg(intel_dig_port, &send_eks,
>> +					sizeof(send_eks));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>>   static int hdcp2_authenticate_sink(struct intel_connector *connector)
>>   {
>> -	DRM_ERROR("Sink authentication is done in subsequent patches\n");
>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>> +	struct intel_hdcp *hdcp = &connector->hdcp;
>> +	const struct intel_hdcp_shim *shim = hdcp->shim;
>> +	struct hdcp2_dp_errata_stream_type stream_type_msg;
>> +	int ret;
>>   
>> -	return -EINVAL;
>> +	ret = hdcp2_authentication_key_exchange(connector);
>> +	if (ret < 0) {
>> +		DRM_DEBUG_KMS("AKE Failed. Err : %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = hdcp2_locality_check(connector);
>> +	if (ret < 0) {
>> +		DRM_DEBUG_KMS("Locality Check failed. Err : %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = hdcp2_session_key_exchange(connector);
>> +	if (ret < 0) {
>> +		DRM_DEBUG_KMS("SKE Failed. Err : %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (!hdcp->is_repeater && shim->config_stream_type) {
>> +		/*
>> +		 * Errata for DP: As Stream type is used for encryption,
>> +		 * Receiver should be communicated with stream type for the
>> +		 * decryption of the content.
>> +		 * Repeater will be communicated with stream type as a
>> +		 * part of it's auth later in time.
>> +		 */
> I'm not following what you want to say with this comment, and haven't
> found anything in the hdcp2 dp spec about this either.

this is there in the errata of DP HDCP2.2 spec.
hdcp2.2 encryption algo involves the stream type as a parameter.
And as part of hdcp auth mentioned in base spec DP repeaters receive that details to decrypt the content.
But DP receivers dont get it. So errata adds the missing piece for decryption.

-Ram

>
>> +		stream_type_msg.msg_id = HDCP_2_2_ERRATA_DP_STREAM_TYP:E;
>> +		stream_type_msg.stream_type = hdcp->content_type;
>> +
>> +		ret = shim->config_stream_type(intel_dig_port, &stream_type_msg,
>> +					       sizeof(stream_type_msg));
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	hdcp->port_data.streams[0].stream_type = hdcp->content_type;
>> +	ret = hdcp2_authenticate_port(connector);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return ret;
>>   }
> With thy types changed to bool and the dp errata cleared up somehow:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>>   
>>   static int hdcp2_enable_encryption(struct intel_connector *connector)
>> -- 
>> 2.7.4
>>
On Wed, Dec 19, 2018 at 08:35:48PM +0530, C, Ramalingam wrote:
> On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> > On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> > > +	if (!hdcp->is_repeater && shim->config_stream_type) {
> > > +		/*
> > > +		 * Errata for DP: As Stream type is used for encryption,
> > > +		 * Receiver should be communicated with stream type for the
> > > +		 * decryption of the content.
> > > +		 * Repeater will be communicated with stream type as a
> > > +		 * part of it's auth later in time.
> > > +		 */
> > I'm not following what you want to say with this comment, and haven't
> > found anything in the hdcp2 dp spec about this either.
> 
> this is there in the errata of DP HDCP2.2 spec.
> hdcp2.2 encryption algo involves the stream type as a parameter.
> And as part of hdcp auth mentioned in base spec DP repeaters receive that details to decrypt the content.
> But DP receivers dont get it. So errata adds the missing piece for decryption.

Ok I found it, but the fake HDCP_2_2_ERRATA_DP_STREAM_TYPE define is kinda
annoying. It doesn't exist in the spec, but we put it into the drm_hdcp.h
header. Since you have a special ->config_stream_type hook for this
anyway, and it's only needed for DP, please move that into the shim
implementation.

And then a follow-up patch to remove the fake #define from drm_hdcp.h.
-Daniel
On 12/19/2018 9:05 PM, Daniel Vetter wrote:
> On Wed, Dec 19, 2018 at 08:35:48PM +0530, C, Ramalingam wrote:
>> On 12/19/2018 8:05 PM, Daniel Vetter wrote:
>>> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
>>>> +	if (!hdcp->is_repeater && shim->config_stream_type) {
>>>> +		/*
>>>> +		 * Errata for DP: As Stream type is used for encryption,
>>>> +		 * Receiver should be communicated with stream type for the
>>>> +		 * decryption of the content.
>>>> +		 * Repeater will be communicated with stream type as a
>>>> +		 * part of it's auth later in time.
>>>> +		 */
>>> I'm not following what you want to say with this comment, and haven't
>>> found anything in the hdcp2 dp spec about this either.
>> this is there in the errata of DP HDCP2.2 spec.
>> hdcp2.2 encryption algo involves the stream type as a parameter.
>> And as part of hdcp auth mentioned in base spec DP repeaters receive that details to decrypt the content.
>> But DP receivers dont get it. So errata adds the missing piece for decryption.
> Ok I found it, but the fake HDCP_2_2_ERRATA_DP_STREAM_TYPE define is kinda
> annoying. It doesn't exist in the spec, but we put it into the drm_hdcp.h
> header. Since you have a special ->config_stream_type hook for this
> anyway, and it's only needed for DP, please move that into the shim
> implementation.
>
> And then a follow-up patch to remove the fake #define from drm_hdcp.h.

This is defined to make the hdcp_shim interface to look common between DP and HDMI.
We can move this errata msg definition and its msg_id into intel_drv.h just for the intel's consumption.

-Ram

> -Daniel
On Wed, 19 Dec 2018, "C, Ramalingam" <ramalingam.c@intel.com> wrote:
> On 12/19/2018 8:05 PM, Daniel Vetter wrote:
>> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
>>>   struct intel_hdcp {
>>> @@ -414,6 +430,24 @@ struct intel_hdcp {
>>>   	 */
>>>   	u8 content_type;
>>>   	struct hdcp_port_data port_data;
>>> +
>>> +	u8 is_paired;
>>> +	u8 is_repeater;
>> Make these two bool, will simplify the code a bunch.
>
> Seems there is a movement for not to use the bool in structures.

No. Please use bools in structs when it makes sense. Avoid bools in
structs when you need to care about memory footprint or alignment or
packing or the like. This is not one of those cases.

> Thats why I have changed these from bool to u8 from v8
> onwards. Checkpatch also complains on this

Sorry to say, checkpatch is not the authority although we do send out
automated checkpatch results.

BR,
Jani.
> 
> On Wed, 19 Dec 2018, "C, Ramalingam" <ramalingam.c@intel.com> wrote:
> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> >>>   struct intel_hdcp {
> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> >>>   	 */
> >>>   	u8 content_type;
> >>>   	struct hdcp_port_data port_data;
> >>> +
> >>> +	u8 is_paired;
> >>> +	u8 is_repeater;
> >> Make these two bool, will simplify the code a bunch.
> >
> > Seems there is a movement for not to use the bool in structures.
> 
> No. Please use bools in structs when it makes sense. Avoid bools in structs
> when you need to care about memory footprint or alignment or packing or
> the like. This is not one of those cases.
> 
> > Thats why I have changed these from bool to u8 from v8 onwards.
> > Checkpatch also complains on this
> 
> Sorry to say, checkpatch is not the authority although we do send out
> automated checkpatch results.

I believe it was Linus' call to not use  bool in structs at all
https://lkml.org/lkml/2017/11/21/384

Thanks
Tomas
On Wed, 19 Dec 2018, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
>> 
>> On Wed, 19 Dec 2018, "C, Ramalingam" <ramalingam.c@intel.com> wrote:
>> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
>> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
>> >>>   struct intel_hdcp {
>> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
>> >>>   	 */
>> >>>   	u8 content_type;
>> >>>   	struct hdcp_port_data port_data;
>> >>> +
>> >>> +	u8 is_paired;
>> >>> +	u8 is_repeater;
>> >> Make these two bool, will simplify the code a bunch.
>> >
>> > Seems there is a movement for not to use the bool in structures.
>> 
>> No. Please use bools in structs when it makes sense. Avoid bools in structs
>> when you need to care about memory footprint or alignment or packing or
>> the like. This is not one of those cases.
>> 
>> > Thats why I have changed these from bool to u8 from v8 onwards.
>> > Checkpatch also complains on this
>> 
>> Sorry to say, checkpatch is not the authority although we do send out
>> automated checkpatch results.
>
> I believe it was Linus' call to not use  bool in structs at all
> https://lkml.org/lkml/2017/11/21/384

I don't care. That's a valid judgement in the context referenced, but
the conclusion "no bools in structs at all" isn't. In this case, I think
bools are the better option, and anything else makes the code worse.

BR,
Jani.
> On Wed, 19 Dec 2018, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
> >>
> >> On Wed, 19 Dec 2018, "C, Ramalingam" <ramalingam.c@intel.com> wrote:
> >> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> >> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> >> >>>   struct intel_hdcp {
> >> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> >> >>>   	 */
> >> >>>   	u8 content_type;
> >> >>>   	struct hdcp_port_data port_data;
> >> >>> +
> >> >>> +	u8 is_paired;
> >> >>> +	u8 is_repeater;
> >> >> Make these two bool, will simplify the code a bunch.
> >> >
> >> > Seems there is a movement for not to use the bool in structures.
> >>
> >> No. Please use bools in structs when it makes sense. Avoid bools in
> >> structs when you need to care about memory footprint or alignment or
> >> packing or the like. This is not one of those cases.
> >>
> >> > Thats why I have changed these from bool to u8 from v8 onwards.
> >> > Checkpatch also complains on this
> >>
> >> Sorry to say, checkpatch is not the authority although we do send out
> >> automated checkpatch results.
> >
> > I believe it was Linus' call to not use  bool in structs at all
> > https://lkml.org/lkml/2017/11/21/384
> 
> I don't care. That's a valid judgement in the context referenced, but the
> conclusion "no bools in structs at all" isn't. In this case, I think bools are the
> better option, and anything else makes the code worse.

The solution was to use bit fields, 
 unsinged int is_paired:1;
unsinged int is_repeter:1
There is a strong point in consistency so there are no mistakes.

But frankly I don't really have strong feelings about it.

Thanks
Tomas
On Thu, Dec 20, 2018 at 02:28:55PM +0000, Winkler, Tomas wrote:
> 
> 
> > On Wed, 19 Dec 2018, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
> > >>
> > >> On Wed, 19 Dec 2018, "C, Ramalingam" <ramalingam.c@intel.com> wrote:
> > >> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> > >> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> > >> >>>   struct intel_hdcp {
> > >> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> > >> >>>   	 */
> > >> >>>   	u8 content_type;
> > >> >>>   	struct hdcp_port_data port_data;
> > >> >>> +
> > >> >>> +	u8 is_paired;
> > >> >>> +	u8 is_repeater;
> > >> >> Make these two bool, will simplify the code a bunch.
> > >> >
> > >> > Seems there is a movement for not to use the bool in structures.
> > >>
> > >> No. Please use bools in structs when it makes sense. Avoid bools in
> > >> structs when you need to care about memory footprint or alignment or
> > >> packing or the like. This is not one of those cases.
> > >>
> > >> > Thats why I have changed these from bool to u8 from v8 onwards.
> > >> > Checkpatch also complains on this
> > >>
> > >> Sorry to say, checkpatch is not the authority although we do send out
> > >> automated checkpatch results.
> > >
> > > I believe it was Linus' call to not use  bool in structs at all
> > > https://lkml.org/lkml/2017/11/21/384
> > 
> > I don't care. That's a valid judgement in the context referenced, but the
> > conclusion "no bools in structs at all" isn't. In this case, I think bools are the
> > better option, and anything else makes the code worse.
> 
> The solution was to use bit fields, 
>  unsinged int is_paired:1;
> unsinged int is_repeter:1

This doesn't work with READ_ONCE/WRITE_ONCE, and it generates terrible
assembly (at least gcc is well known for struggling with these, compared
to open-coded bitops). So depending upon what you want to do, and where
youre space/performance tradeoff lies, doing this unconditionally is just
wrong.

It was the right thing for the patch Linus commented on though.
-Daniel

> There is a strong point in consistency so there are no mistakes.
> 
> But frankly I don't really have strong feelings about it.
> 
> Thanks
> Tomas
>
On Thu, Dec 20, 2018 at 03:55:27PM +0100, Daniel Vetter wrote:
> On Thu, Dec 20, 2018 at 02:28:55PM +0000, Winkler, Tomas wrote:
> > 
> > 
> > > On Wed, 19 Dec 2018, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
> > > >>
> > > >> On Wed, 19 Dec 2018, "C, Ramalingam" <ramalingam.c@intel.com> wrote:
> > > >> > On 12/19/2018 8:05 PM, Daniel Vetter wrote:
> > > >> >> On Thu, Dec 13, 2018 at 09:31:12AM +0530, Ramalingam C wrote:
> > > >> >>>   struct intel_hdcp {
> > > >> >>> @@ -414,6 +430,24 @@ struct intel_hdcp {
> > > >> >>>   	 */
> > > >> >>>   	u8 content_type;
> > > >> >>>   	struct hdcp_port_data port_data;
> > > >> >>> +
> > > >> >>> +	u8 is_paired;
> > > >> >>> +	u8 is_repeater;
> > > >> >> Make these two bool, will simplify the code a bunch.
> > > >> >
> > > >> > Seems there is a movement for not to use the bool in structures.
> > > >>
> > > >> No. Please use bools in structs when it makes sense. Avoid bools in
> > > >> structs when you need to care about memory footprint or alignment or
> > > >> packing or the like. This is not one of those cases.
> > > >>
> > > >> > Thats why I have changed these from bool to u8 from v8 onwards.
> > > >> > Checkpatch also complains on this
> > > >>
> > > >> Sorry to say, checkpatch is not the authority although we do send out
> > > >> automated checkpatch results.
> > > >
> > > > I believe it was Linus' call to not use  bool in structs at all
> > > > https://lkml.org/lkml/2017/11/21/384
> > > 
> > > I don't care. That's a valid judgement in the context referenced, but the
> > > conclusion "no bools in structs at all" isn't. In this case, I think bools are the
> > > better option, and anything else makes the code worse.
> > 
> > The solution was to use bit fields, 
> >  unsinged int is_paired:1;
> > unsinged int is_repeter:1
> 
> This doesn't work with READ_ONCE/WRITE_ONCE, and it generates terrible
> assembly (at least gcc is well known for struggling with these, compared
> to open-coded bitops). So depending upon what you want to do, and where
> youre space/performance tradeoff lies, doing this unconditionally is just
> wrong.

Another annoying downside with non-bool bitfields:

unsigned int foo:1;
foo = 2;
-> foo == 0

So you'll need to remmber to convert everything to
0/1 before the assignment.