[v8,05/35] drm/i915: MEI interface definition

Submitted by Ramalingam C on Nov. 27, 2018, 10:43 a.m.

Details

Message ID 1543315413-24302-6-git-send-email-ramalingam.c@intel.com
State New
Headers show
Series "drm/i915: Implement HDCP2.2" ( rev: 10 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ramalingam C Nov. 27, 2018, 10:43 a.m.
Defining the mei-i915 interface functions and initialization of
the interface.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |   2 +
 drivers/gpu/drm/i915/intel_drv.h  |   7 +
 drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
 include/drm/i915_component.h      |  71 ++++++
 4 files changed, 521 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..b68bc980b7cd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2015,6 +2015,8 @@  struct drm_i915_private {
 
 	struct i915_pmu pmu;
 
+	struct i915_hdcp_component_master *hdcp_comp;
+
 	/*
 	 * 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 85a526598096..bde82f3ada85 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -29,6 +29,7 @@ 
 #include <linux/i2c.h>
 #include <linux/hdmi.h>
 #include <linux/sched/clock.h>
+#include <linux/mei_hdcp.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include <drm/drm_crtc.h>
@@ -379,6 +380,9 @@  struct intel_hdcp_shim {
 	/* Detects panel's hdcp capability. This is optional for HDMI. */
 	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
 			    bool *hdcp_capable);
+
+	/* Detects the HDCP protocol(DP/HDMI) required on the port */
+	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
 };
 
 struct intel_hdcp {
@@ -399,6 +403,9 @@  struct intel_hdcp {
 	 * content can flow only through a link protected by HDCP2.2.
 	 */
 	u8 content_type;
+
+	/* mei interface related information */
+	struct mei_hdcp_data mei_data;
 };
 
 struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 99dddb540958..760780f1105c 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -8,14 +8,20 @@ 
 
 #include <drm/drmP.h>
 #include <drm/drm_hdcp.h>
+#include <drm/i915_component.h>
 #include <linux/i2c.h>
 #include <linux/random.h>
+#include <linux/component.h>
 
 #include "intel_drv.h"
 #include "i915_reg.h"
 
 #define KEY_LOAD_TRIES	5
 #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
+#define GET_MEI_DDI_INDEX(p) ({                            \
+	typeof(p) __p = (p);                               \
+	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
+})
 
 static
 bool intel_hdcp_is_ksv_valid(u8 *ksv)
@@ -833,6 +839,417 @@  bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
 		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
 }
 
+static __attribute__((unused)) int
+hdcp2_prepare_ake_init(struct intel_connector *connector,
+		       struct hdcp2_ake_init *ake_data)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
+		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
+
+	/* Clear ME FW instance for the port, just incase */
+	comp->ops->close_hdcp_session(comp->dev, data);
+
+	ret = comp->ops->initiate_hdcp2_session(comp->dev,
+						data, ake_data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
+				struct hdcp2_ake_send_cert *rx_cert,
+				bool *paired,
+				struct hdcp2_ake_no_stored_km *ek_pub_km,
+				size_t *msg_sz)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
+							 rx_cert, paired,
+							 ek_pub_km, msg_sz);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_hprime(struct intel_connector *connector,
+		    struct hdcp2_ake_send_hprime *rx_hprime)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_store_pairing_info(struct intel_connector *connector,
+			 struct hdcp2_ake_send_pairing_info *pairing_info)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->store_pairing_info(comp->dev,
+					    data, pairing_info);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_prepare_lc_init(struct intel_connector *connector,
+		      struct hdcp2_lc_init *lc_init)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->initiate_locality_check(comp->dev,
+						 data, lc_init);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_lprime(struct intel_connector *connector,
+		    struct hdcp2_lc_send_lprime *rx_lprime)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_prepare_skey(struct intel_connector *connector,
+		       struct hdcp2_ske_send_eks *ske_data)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
+				      struct hdcp2_rep_send_receiverid_list
+								*rep_topology,
+				      struct hdcp2_rep_send_ack *rep_send_ack)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
+							 data, rep_topology,
+							 rep_send_ack);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused)) int
+hdcp2_verify_mprime(struct intel_connector *connector,
+		    struct hdcp2_rep_stream_ready *stream_ready)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_authenticate_port(struct intel_connector *connector)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
+	if (ret < 0)
+		comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+
+	return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_close_mei_session(struct intel_connector *connector)
+{
+	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
+	int ret;
+
+	if (!comp)
+		return -EINVAL;
+
+	mutex_lock(&comp->mutex);
+	if (!comp->ops || !comp->dev ||
+	    data->port == MEI_DDI_INVALID_PORT) {
+		mutex_unlock(&comp->mutex);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->close_hdcp_session(comp->dev, data);
+	mutex_unlock(&comp->mutex);
+	return ret;
+}
+
+static __attribute__((unused))
+int hdcp2_deauthenticate_port(struct intel_connector *connector)
+{
+	return hdcp2_close_mei_session(connector);
+}
+
+static int i915_hdcp_component_master_bind(struct device *dev)
+{
+	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
+
+	return component_bind_all(dev, dev_priv->hdcp_comp);
+}
+
+static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = &dev_priv->drm;
+	struct intel_connector *intel_connector;
+	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
+
+	drm_connector_list_iter_begin(dev, &conn_iter);
+	drm_for_each_connector_iter(connector, &conn_iter) {
+		intel_connector = to_intel_connector(connector);
+		if (!(intel_connector->hdcp.shim))
+			continue;
+
+		intel_hdcp_disable(intel_connector);
+	}
+	drm_connector_list_iter_end(&conn_iter);
+}
+
+static void i915_hdcp_component_master_unbind(struct device *dev)
+{
+	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
+
+	intel_connectors_hdcp_disable(dev_priv);
+	component_unbind_all(dev, dev_priv->hdcp_comp);
+}
+
+static const struct component_master_ops i915_hdcp_component_master_ops = {
+	.bind = i915_hdcp_component_master_bind,
+	.unbind = i915_hdcp_component_master_unbind,
+};
+
+static int i915_hdcp_component_match(struct device *dev, void *data)
+{
+	return !strcmp(dev->driver->name, "mei_hdcp");
+}
+
+static int intel_hdcp_component_init(struct intel_connector *connector)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct i915_hdcp_component_master *comp;
+	struct component_match *match = NULL;
+	int ret;
+
+	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
+	if (!comp)
+		return -ENOMEM;
+
+	mutex_init(&comp->mutex);
+	dev_priv->hdcp_comp = comp;
+
+	component_match_add(dev_priv->drm.dev, &match,
+			    i915_hdcp_component_match, dev_priv);
+	ret = component_master_add_with_match(dev_priv->drm.dev,
+					      &i915_hdcp_component_master_ops,
+					      match);
+	if (ret < 0)
+		goto out_err;
+
+	DRM_INFO("I915 hdcp component master added.\n");
+	return ret;
+
+out_err:
+	component_master_del(dev_priv->drm.dev,
+			     &i915_hdcp_component_master_ops);
+	kfree(comp);
+	dev_priv->hdcp_comp = NULL;
+	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
+
+	return ret;
+}
+
+static int initialize_mei_hdcp_data(struct intel_connector *connector)
+{
+	struct intel_hdcp *hdcp = &connector->hdcp;
+	struct mei_hdcp_data *data = &hdcp->mei_data;
+	enum port port;
+
+	if (connector->encoder) {
+		port = connector->encoder->port;
+		data->port = GET_MEI_DDI_INDEX(port);
+	}
+
+	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
+	data->protocol = hdcp->shim->hdcp_protocol();
+
+	data->k = 1;
+	if (!data->streams)
+		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
+					GFP_KERNEL);
+	if (!data->streams) {
+		DRM_ERROR("Out of Memory\n");
+		return -ENOMEM;
+	}
+
+	data->streams[0].stream_id = 0;
+	data->streams[0].stream_type = hdcp->content_type;
+
+	return 0;
+}
+
 bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
 {
 	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
@@ -843,10 +1260,25 @@  static void intel_hdcp2_init(struct intel_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
 	struct intel_hdcp *hdcp = &connector->hdcp;
+	int ret;
 
 	WARN_ON(!is_hdcp2_supported(dev_priv));
 
-	/* TODO: MEI interface needs to be initialized here */
+	ret = initialize_mei_hdcp_data(connector);
+	if (ret) {
+		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
+		return;
+	}
+
+	if (!dev_priv->hdcp_comp)
+		ret = intel_hdcp_component_init(connector);
+
+	if (ret) {
+		DRM_DEBUG_KMS("HDCP comp init failed\n");
+		kfree(hdcp->mei_data.streams);
+		return;
+	}
+
 	hdcp->hdcp2_supported = 1;
 }
 
@@ -894,9 +1326,17 @@  void intel_hdcp_exit(struct drm_i915_private *dev_priv)
 		mutex_lock(&intel_connector->hdcp.mutex);
 		intel_connector->hdcp.hdcp2_supported = 0;
 		intel_connector->hdcp.shim = NULL;
+		kfree(intel_connector->hdcp.mei_data.streams);
 		mutex_unlock(&intel_connector->hdcp.mutex);
 	}
 	drm_connector_list_iter_end(&conn_iter);
+
+	if (dev_priv->hdcp_comp) {
+		component_master_del(dev_priv->drm.dev,
+				     &i915_hdcp_component_master_ops);
+		kfree(dev_priv->hdcp_comp);
+		dev_priv->hdcp_comp = NULL;
+	}
 }
 
 int intel_hdcp_enable(struct intel_connector *connector)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index fca22d463e1b..12268228f4dc 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,12 @@ 
 #ifndef _I915_COMPONENT_H_
 #define _I915_COMPONENT_H_
 
+#include <linux/mutex.h>
+
 #include "drm_audio_component.h"
 
+#include <drm/drm_hdcp.h>
+
 /* MAX_PORT is the number of port
  * It must be sync with I915_MAX_PORTS defined i915_drv.h
  */
@@ -46,4 +50,71 @@  struct i915_audio_component {
 	int aud_sample_rate[MAX_PORTS];
 };
 
+struct i915_hdcp_component_ops {
+	/**
+	 * @owner: mei_hdcp module
+	 */
+	struct module *owner;
+
+	int (*initiate_hdcp2_session)(struct device *dev,
+				      void *hdcp_data,
+				      struct hdcp2_ake_init *ake_data);
+	int (*verify_receiver_cert_prepare_km)(struct device *dev,
+					       void *hdcp_data,
+					       struct hdcp2_ake_send_cert
+								*rx_cert,
+					       bool *km_stored,
+					       struct hdcp2_ake_no_stored_km
+								*ek_pub_km,
+					       size_t *msg_sz);
+	int (*verify_hprime)(struct device *dev,
+			     void *hdcp_data,
+			     struct hdcp2_ake_send_hprime *rx_hprime);
+	int (*store_pairing_info)(struct device *dev,
+				  void *hdcp_data,
+				  struct hdcp2_ake_send_pairing_info
+								*pairing_info);
+	int (*initiate_locality_check)(struct device *dev,
+				       void *hdcp_data,
+				       struct hdcp2_lc_init *lc_init_data);
+	int (*verify_lprime)(struct device *dev,
+			     void *hdcp_data,
+			     struct hdcp2_lc_send_lprime *rx_lprime);
+	int (*get_session_key)(struct device *dev,
+			       void *hdcp_data,
+			       struct hdcp2_ske_send_eks *ske_data);
+	int (*repeater_check_flow_prepare_ack)(struct device *dev,
+					       void *hdcp_data,
+					       struct hdcp2_rep_send_receiverid_list
+								*rep_topology,
+					       struct hdcp2_rep_send_ack
+								*rep_send_ack);
+	int (*verify_mprime)(struct device *dev,
+			     void *hdcp_data,
+			     struct hdcp2_rep_stream_ready *stream_ready);
+	int (*enable_hdcp_authentication)(struct device *dev,
+					  void *hdcp_data);
+	int (*close_hdcp_session)(struct device *dev,
+				  void *hdcp_data);
+};
+
+/**
+ * struct i915_hdcp_component_master - Used for communication between i915
+ * and mei_hdcp for HDCP2.2 services.
+ */
+struct i915_hdcp_component_master {
+	/**
+	 * @dev: a device providing hdcp
+	 */
+	struct device *dev;
+	/**
+	 * @mutex: Mutex to protect the state of dev
+	 */
+	struct mutex mutex;
+	/**
+	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
+	 */
+	const struct i915_hdcp_component_ops *ops;
+};
+
 #endif /* _I915_COMPONENT_H_ */

Comments

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> Defining the mei-i915 interface functions and initialization of
> the interface.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |   2 +
>  drivers/gpu/drm/i915/intel_drv.h  |   7 +
>  drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
>  include/drm/i915_component.h      |  71 ++++++
>  4 files changed, 521 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f763b30f98d9..b68bc980b7cd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2015,6 +2015,8 @@ struct drm_i915_private {
>  
>  	struct i915_pmu pmu;
>  
> +	struct i915_hdcp_component_master *hdcp_comp;
> +
>  	/*
>  	 * 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 85a526598096..bde82f3ada85 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -29,6 +29,7 @@
>  #include <linux/i2c.h>
>  #include <linux/hdmi.h>
>  #include <linux/sched/clock.h>
> +#include <linux/mei_hdcp.h>
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include <drm/drm_crtc.h>
> @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
>  	/* Detects panel's hdcp capability. This is optional for HDMI. */
>  	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
>  			    bool *hdcp_capable);
> +
> +	/* Detects the HDCP protocol(DP/HDMI) required on the port */
> +	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);

Looking ahead, this seems hardwired to constant return value? Or why do we
need a function here?

>  };
>  
>  struct intel_hdcp {
> @@ -399,6 +403,9 @@ struct intel_hdcp {
>  	 * content can flow only through a link protected by HDCP2.2.
>  	 */
>  	u8 content_type;
> +
> +	/* mei interface related information */
> +	struct mei_hdcp_data mei_data;
>  };
>  
>  struct intel_connector {
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 99dddb540958..760780f1105c 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -8,14 +8,20 @@
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_hdcp.h>
> +#include <drm/i915_component.h>
>  #include <linux/i2c.h>
>  #include <linux/random.h>
> +#include <linux/component.h>
>  
>  #include "intel_drv.h"
>  #include "i915_reg.h"
>  
>  #define KEY_LOAD_TRIES	5
>  #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
> +#define GET_MEI_DDI_INDEX(p) ({                            \
> +	typeof(p) __p = (p);                               \
> +	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
> +})
>  
>  static
>  bool intel_hdcp_is_ksv_valid(u8 *ksv)
> @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>  		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
>  }
>  
> +static __attribute__((unused)) int
> +hdcp2_prepare_ake_init(struct intel_connector *connector,
> +		       struct hdcp2_ake_init *ake_data)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
> +
> +	/* Clear ME FW instance for the port, just incase */
> +	comp->ops->close_hdcp_session(comp->dev, data);

Sounds like a bug somewhere if we need this? I'd put this code into the
->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.

"Just in case" papering over programming bugs of our own just makes
debugging harder.

> +
> +	ret = comp->ops->initiate_hdcp2_session(comp->dev,
> +						data, ake_data);

We should set the port only after successfully initializing this.

> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
> +				struct hdcp2_ake_send_cert *rx_cert,
> +				bool *paired,
> +				struct hdcp2_ake_no_stored_km *ek_pub_km,
> +				size_t *msg_sz)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {

These all look like programming mistakes that should result in a WARN_ON.

> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
> +							 rx_cert, paired,
> +							 ek_pub_km, msg_sz);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);

Error handling here seems a bit strange. You close the session, but don't
reset the port. So next op will be totally unaware that things have blown
up. Also no warning.

If we want to close the session, then I think that should be a decision
made higher up.

> +	mutex_unlock(&comp->mutex);

With component do we still need this mutex stuff here?

Exact same comments everywhere below.

> +
> +	return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_verify_hprime(struct intel_connector *connector,
> +		    struct hdcp2_ake_send_hprime *rx_hprime)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_store_pairing_info(struct intel_connector *connector,
> +			 struct hdcp2_ake_send_pairing_info *pairing_info)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->store_pairing_info(comp->dev,
> +					    data, pairing_info);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_prepare_lc_init(struct intel_connector *connector,
> +		      struct hdcp2_lc_init *lc_init)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->initiate_locality_check(comp->dev,
> +						 data, lc_init);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_verify_lprime(struct intel_connector *connector,
> +		    struct hdcp2_lc_send_lprime *rx_lprime)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused))
> +int hdcp2_prepare_skey(struct intel_connector *connector,
> +		       struct hdcp2_ske_send_eks *ske_data)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
> +				      struct hdcp2_rep_send_receiverid_list
> +								*rep_topology,
> +				      struct hdcp2_rep_send_ack *rep_send_ack)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
> +							 data, rep_topology,
> +							 rep_send_ack);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused)) int
> +hdcp2_verify_mprime(struct intel_connector *connector,
> +		    struct hdcp2_rep_stream_ready *stream_ready)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused))
> +int hdcp2_authenticate_port(struct intel_connector *connector)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
> +	if (ret < 0)
> +		comp->ops->close_hdcp_session(comp->dev, data);
> +	mutex_unlock(&comp->mutex);
> +
> +	return ret;
> +}
> +
> +static __attribute__((unused))
> +int hdcp2_close_mei_session(struct intel_connector *connector)
> +{
> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> +	int ret;
> +
> +	if (!comp)
> +		return -EINVAL;
> +
> +	mutex_lock(&comp->mutex);
> +	if (!comp->ops || !comp->dev ||
> +	    data->port == MEI_DDI_INVALID_PORT) {
> +		mutex_unlock(&comp->mutex);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->close_hdcp_session(comp->dev, data);

Need to reset the port here I think.

> +	mutex_unlock(&comp->mutex);
> +	return ret;
> +}
> +
> +static __attribute__((unused))
> +int hdcp2_deauthenticate_port(struct intel_connector *connector)
> +{
> +	return hdcp2_close_mei_session(connector);
> +}
> +
> +static int i915_hdcp_component_master_bind(struct device *dev)
> +{
> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> +
> +	return component_bind_all(dev, dev_priv->hdcp_comp);
> +}
> +
> +static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +	struct intel_connector *intel_connector;
> +	struct drm_connector *connector;
> +	struct drm_connector_list_iter conn_iter;
> +
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(connector, &conn_iter) {
> +		intel_connector = to_intel_connector(connector);
> +		if (!(intel_connector->hdcp.shim))
> +			continue;
> +
> +		intel_hdcp_disable(intel_connector);
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +}
> +
> +static void i915_hdcp_component_master_unbind(struct device *dev)
> +{
> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> +
> +	intel_connectors_hdcp_disable(dev_priv);

Why this code? Once we've unregistered the driver, we should have shut
down the hardware completely. Which should have shut down all the hdcp
users too.

> +	component_unbind_all(dev, dev_priv->hdcp_comp);
> +}
> +
> +static const struct component_master_ops i915_hdcp_component_master_ops = {
> +	.bind = i915_hdcp_component_master_bind,
> +	.unbind = i915_hdcp_component_master_unbind,
> +};
> +
> +static int i915_hdcp_component_match(struct device *dev, void *data)
> +{
> +	return !strcmp(dev->driver->name, "mei_hdcp");
> +}
> +
> +static int intel_hdcp_component_init(struct intel_connector *connector)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct i915_hdcp_component_master *comp;
> +	struct component_match *match = NULL;
> +	int ret;
> +
> +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
> +	if (!comp)
> +		return -ENOMEM;
> +
> +	mutex_init(&comp->mutex);
> +	dev_priv->hdcp_comp = comp;
> +
> +	component_match_add(dev_priv->drm.dev, &match,
> +			    i915_hdcp_component_match, dev_priv);
> +	ret = component_master_add_with_match(dev_priv->drm.dev,
> +					      &i915_hdcp_component_master_ops,
> +					      match);

So I'm not sure this will work out well, hiding the master_ops here in
hdcp. Definitely needs to be rewritten as soon as i915 needs another
component. And might make the load/unload split complicated.

> +	if (ret < 0)
> +		goto out_err;
> +
> +	DRM_INFO("I915 hdcp component master added.\n");

You add both the master and the hdcp component here. Output is a bit
confusing.

> +	return ret;
> +
> +out_err:
> +	component_master_del(dev_priv->drm.dev,
> +			     &i915_hdcp_component_master_ops);
> +	kfree(comp);
> +	dev_priv->hdcp_comp = NULL;
> +	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
> +
> +	return ret;
> +}
> +
> +static int initialize_mei_hdcp_data(struct intel_connector *connector)
> +{
> +	struct intel_hdcp *hdcp = &connector->hdcp;
> +	struct mei_hdcp_data *data = &hdcp->mei_data;
> +	enum port port;
> +
> +	if (connector->encoder) {
> +		port = connector->encoder->port;
> +		data->port = GET_MEI_DDI_INDEX(port);
> +	}
> +
> +	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
> +	data->protocol = hdcp->shim->hdcp_protocol();
> +
> +	data->k = 1;
> +	if (!data->streams)
> +		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
> +					GFP_KERNEL);
> +	if (!data->streams) {
> +		DRM_ERROR("Out of Memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	data->streams[0].stream_id = 0;
> +	data->streams[0].stream_type = hdcp->content_type;
> +
> +	return 0;
> +}
> +
>  bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
>  {
>  	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
> @@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>  	struct intel_hdcp *hdcp = &connector->hdcp;
> +	int ret;
>  
>  	WARN_ON(!is_hdcp2_supported(dev_priv));
>  
> -	/* TODO: MEI interface needs to be initialized here */
> +	ret = initialize_mei_hdcp_data(connector);
> +	if (ret) {
> +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
> +		return;
> +	}
> +
> +	if (!dev_priv->hdcp_comp)
> +		ret = intel_hdcp_component_init(connector);
> +
> +	if (ret) {
> +		DRM_DEBUG_KMS("HDCP comp init failed\n");
> +		kfree(hdcp->mei_data.streams);
> +		return;
> +	}
> +
>  	hdcp->hdcp2_supported = 1;
>  }
>  
> @@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
>  		mutex_lock(&intel_connector->hdcp.mutex);
>  		intel_connector->hdcp.hdcp2_supported = 0;
>  		intel_connector->hdcp.shim = NULL;
> +		kfree(intel_connector->hdcp.mei_data.streams);

We need to push this into a per-connector hdcp cleanup function. Or just
into the generic connector cleanup.

>  		mutex_unlock(&intel_connector->hdcp.mutex);
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> +
> +	if (dev_priv->hdcp_comp) {
> +		component_master_del(dev_priv->drm.dev,
> +				     &i915_hdcp_component_master_ops);
> +		kfree(dev_priv->hdcp_comp);
> +		dev_priv->hdcp_comp = NULL;
> +	}
>  }
>  
>  int intel_hdcp_enable(struct intel_connector *connector)
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index fca22d463e1b..12268228f4dc 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -24,8 +24,12 @@
>  #ifndef _I915_COMPONENT_H_
>  #define _I915_COMPONENT_H_
>  
> +#include <linux/mutex.h>
> +
>  #include "drm_audio_component.h"
>  
> +#include <drm/drm_hdcp.h>
> +
>  /* MAX_PORT is the number of port
>   * It must be sync with I915_MAX_PORTS defined i915_drv.h
>   */
> @@ -46,4 +50,71 @@ struct i915_audio_component {
>  	int aud_sample_rate[MAX_PORTS];
>  };
>  
> +struct i915_hdcp_component_ops {

Imo that should be called mei_hdcp_component_ops and put into the
linux/mei_hdcp.h header. Or was that Thomas' review comment?

Aside: Review here in public channels instead of in private would be much
better for coordination.

> +	/**
> +	 * @owner: mei_hdcp module
> +	 */
> +	struct module *owner;
> +
> +	int (*initiate_hdcp2_session)(struct device *dev,
> +				      void *hdcp_data,
> +				      struct hdcp2_ake_init *ake_data);
> +	int (*verify_receiver_cert_prepare_km)(struct device *dev,
> +					       void *hdcp_data,
> +					       struct hdcp2_ake_send_cert
> +								*rx_cert,
> +					       bool *km_stored,
> +					       struct hdcp2_ake_no_stored_km
> +								*ek_pub_km,
> +					       size_t *msg_sz);
> +	int (*verify_hprime)(struct device *dev,
> +			     void *hdcp_data,
> +			     struct hdcp2_ake_send_hprime *rx_hprime);
> +	int (*store_pairing_info)(struct device *dev,
> +				  void *hdcp_data,
> +				  struct hdcp2_ake_send_pairing_info
> +								*pairing_info);
> +	int (*initiate_locality_check)(struct device *dev,
> +				       void *hdcp_data,
> +				       struct hdcp2_lc_init *lc_init_data);
> +	int (*verify_lprime)(struct device *dev,
> +			     void *hdcp_data,
> +			     struct hdcp2_lc_send_lprime *rx_lprime);
> +	int (*get_session_key)(struct device *dev,
> +			       void *hdcp_data,
> +			       struct hdcp2_ske_send_eks *ske_data);
> +	int (*repeater_check_flow_prepare_ack)(struct device *dev,
> +					       void *hdcp_data,
> +					       struct hdcp2_rep_send_receiverid_list
> +								*rep_topology,
> +					       struct hdcp2_rep_send_ack
> +								*rep_send_ack);
> +	int (*verify_mprime)(struct device *dev,
> +			     void *hdcp_data,
> +			     struct hdcp2_rep_stream_ready *stream_ready);
> +	int (*enable_hdcp_authentication)(struct device *dev,
> +					  void *hdcp_data);
> +	int (*close_hdcp_session)(struct device *dev,
> +				  void *hdcp_data);
> +};
> +
> +/**
> + * struct i915_hdcp_component_master - Used for communication between i915
> + * and mei_hdcp for HDCP2.2 services.
> + */
> +struct i915_hdcp_component_master {
> +	/**
> +	 * @dev: a device providing hdcp
> +	 */
> +	struct device *dev;
> +	/**
> +	 * @mutex: Mutex to protect the state of dev
> +	 */
> +	struct mutex mutex;
> +	/**
> +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
> +	 */
> +	const struct i915_hdcp_component_ops *ops;
> +};
> +
>  #endif /* _I915_COMPONENT_H_ */
> -- 
> 2.7.4
>
On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
>> Defining the mei-i915 interface functions and initialization of
>> the interface.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h   |   2 +
>>   drivers/gpu/drm/i915/intel_drv.h  |   7 +
>>   drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
>>   include/drm/i915_component.h      |  71 ++++++
>>   4 files changed, 521 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index f763b30f98d9..b68bc980b7cd 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2015,6 +2015,8 @@ struct drm_i915_private {
>>   
>>   	struct i915_pmu pmu;
>>   
>> +	struct i915_hdcp_component_master *hdcp_comp;
>> +
>>   	/*
>>   	 * 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 85a526598096..bde82f3ada85 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -29,6 +29,7 @@
>>   #include <linux/i2c.h>
>>   #include <linux/hdmi.h>
>>   #include <linux/sched/clock.h>
>> +#include <linux/mei_hdcp.h>
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>>   #include <drm/drm_crtc.h>
>> @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
>>   	/* Detects panel's hdcp capability. This is optional for HDMI. */
>>   	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
>>   			    bool *hdcp_capable);
>> +
>> +	/* Detects the HDCP protocol(DP/HDMI) required on the port */
>> +	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
> Looking ahead, this seems hardwired to constant return value? Or why do we
> need a function here?

This is hardwired based on the connector type(DP/HDMI).
Since we have the shim for hdcp's connector based work, I have added this function.

Could have done this just with connector_type check, but in that way whole hdcp_shim
can be done in that way. So going with the larger design here.

>
>>   };
>>   
>>   struct intel_hdcp {
>> @@ -399,6 +403,9 @@ struct intel_hdcp {
>>   	 * content can flow only through a link protected by HDCP2.2.
>>   	 */
>>   	u8 content_type;
>> +
>> +	/* mei interface related information */
>> +	struct mei_hdcp_data mei_data;
>>   };
>>   
>>   struct intel_connector {
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index 99dddb540958..760780f1105c 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -8,14 +8,20 @@
>>   
>>   #include <drm/drmP.h>
>>   #include <drm/drm_hdcp.h>
>> +#include <drm/i915_component.h>
>>   #include <linux/i2c.h>
>>   #include <linux/random.h>
>> +#include <linux/component.h>
>>   
>>   #include "intel_drv.h"
>>   #include "i915_reg.h"
>>   
>>   #define KEY_LOAD_TRIES	5
>>   #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
>> +#define GET_MEI_DDI_INDEX(p) ({                            \
>> +	typeof(p) __p = (p);                               \
>> +	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
>> +})
>>   
>>   static
>>   bool intel_hdcp_is_ksv_valid(u8 *ksv)
>> @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>>   		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
>>   }
>>   
>> +static __attribute__((unused)) int
>> +hdcp2_prepare_ake_init(struct intel_connector *connector,
>> +		       struct hdcp2_ake_init *ake_data)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
>> +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
>> +
>> +	/* Clear ME FW instance for the port, just incase */
>> +	comp->ops->close_hdcp_session(comp->dev, data);
> Sounds like a bug somewhere if we need this? I'd put this code into the
> ->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.

Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth along with ME FW.
Now if authentication failed due to some reason, then the HDCP2.2 season created with
ME FW for that port is not closed yet.

So we need to call close_hdcp_session() explicitly on authentication failures.
Session has to be closed before restarting the auth on that port with initialite_hdcp_session.
If we are closing the hdcp session of the port on all auth errors, we dont need this just before
start of the hdcp session.

>
> "Just in case" papering over programming bugs of our own just makes
> debugging harder.
>
>> +
>> +	ret = comp->ops->initiate_hdcp2_session(comp->dev,
>> +						data, ake_data);
> We should set the port only after successfully initializing this.

hdcp2_session is created for each port at ME FW. Hence we need the port
initialized even before calling the initiate_hdcp2_session.

>
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused)) int
>> +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>> +				struct hdcp2_ake_send_cert *rx_cert,
>> +				bool *paired,
>> +				struct hdcp2_ake_no_stored_km *ek_pub_km,
>> +				size_t *msg_sz)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> These all look like programming mistakes that should result in a WARN_ON.

We are using the comp->mutex for protecting the interface during the interface
init, usage for mei communication and interface teardown.

But what if mei interface teardown happens between mei communications?
So when we try to access the mei interface after such possible tear down scenario,
we are checking the integrity of the interface.

Possible alternate solution is hold the comp->mutex across the authentication steps.
But consequence is that mei module removal will be blocked for authentication duration
and even if the mei_dev is removed, component unbind will be blocked due to this mutex dependency.

>
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
>> +							 rx_cert, paired,
>> +							 ek_pub_km, msg_sz);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
> Error handling here seems a bit strange. You close the session, but don't
> reset the port. So next op will be totally unaware that things have blown
> up. Also no warning.
>
> If we want to close the session, then I think that should be a decision
> made higher up.

This is needed as explained above. But as you have mentioned this can be moved
to the end of the authentication on error scenario. I will work on that.

>
>> +	mutex_unlock(&comp->mutex);
> With component do we still need this mutex stuff here?
>
> Exact same comments everywhere below.
>
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused)) int
>> +hdcp2_verify_hprime(struct intel_connector *connector,
>> +		    struct hdcp2_ake_send_hprime *rx_hprime)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused)) int
>> +hdcp2_store_pairing_info(struct intel_connector *connector,
>> +			 struct hdcp2_ake_send_pairing_info *pairing_info)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->store_pairing_info(comp->dev,
>> +					    data, pairing_info);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused)) int
>> +hdcp2_prepare_lc_init(struct intel_connector *connector,
>> +		      struct hdcp2_lc_init *lc_init)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->initiate_locality_check(comp->dev,
>> +						 data, lc_init);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused)) int
>> +hdcp2_verify_lprime(struct intel_connector *connector,
>> +		    struct hdcp2_lc_send_lprime *rx_lprime)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused))
>> +int hdcp2_prepare_skey(struct intel_connector *connector,
>> +		       struct hdcp2_ske_send_eks *ske_data)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused)) int
>> +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
>> +				      struct hdcp2_rep_send_receiverid_list
>> +								*rep_topology,
>> +				      struct hdcp2_rep_send_ack *rep_send_ack)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
>> +							 data, rep_topology,
>> +							 rep_send_ack);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused)) int
>> +hdcp2_verify_mprime(struct intel_connector *connector,
>> +		    struct hdcp2_rep_stream_ready *stream_ready)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused))
>> +int hdcp2_authenticate_port(struct intel_connector *connector)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
>> +	if (ret < 0)
>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> +	mutex_unlock(&comp->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused))
>> +int hdcp2_close_mei_session(struct intel_connector *connector)
>> +{
>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>> +	int ret;
>> +
>> +	if (!comp)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&comp->mutex);
>> +	if (!comp->ops || !comp->dev ||
>> +	    data->port == MEI_DDI_INVALID_PORT) {
>> +		mutex_unlock(&comp->mutex);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = comp->ops->close_hdcp_session(comp->dev, data);
> Need to reset the port here I think.

What is the reset of the port you are referring to? port is not
configured for encryption. And this is the call for marking the port as de-authenticated too.

>
>> +	mutex_unlock(&comp->mutex);
>> +	return ret;
>> +}
>> +
>> +static __attribute__((unused))
>> +int hdcp2_deauthenticate_port(struct intel_connector *connector)
>> +{
>> +	return hdcp2_close_mei_session(connector);
>> +}
>> +
>> +static int i915_hdcp_component_master_bind(struct device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>> +
>> +	return component_bind_all(dev, dev_priv->hdcp_comp);
>> +}
>> +
>> +static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
>> +{
>> +	struct drm_device *dev = &dev_priv->drm;
>> +	struct intel_connector *intel_connector;
>> +	struct drm_connector *connector;
>> +	struct drm_connector_list_iter conn_iter;
>> +
>> +	drm_connector_list_iter_begin(dev, &conn_iter);
>> +	drm_for_each_connector_iter(connector, &conn_iter) {
>> +		intel_connector = to_intel_connector(connector);
>> +		if (!(intel_connector->hdcp.shim))
>> +			continue;
>> +
>> +		intel_hdcp_disable(intel_connector);
>> +	}
>> +	drm_connector_list_iter_end(&conn_iter);
>> +}
>> +
>> +static void i915_hdcp_component_master_unbind(struct device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>> +
>> +	intel_connectors_hdcp_disable(dev_priv);
> Why this code? Once we've unregistered the driver, we should have shut
> down the hardware completely. Which should have shut down all the hdcp
> users too.

This unbind might be triggered either due to master_del or component_del.
if its triggered from mei through component_del, then before teardown of
the i/f in component_unbind(), disable the ongoing HDCP session through
hdcp_disable() for each connectors.

>
>> +	component_unbind_all(dev, dev_priv->hdcp_comp);
>> +}
>> +
>> +static const struct component_master_ops i915_hdcp_component_master_ops = {
>> +	.bind = i915_hdcp_component_master_bind,
>> +	.unbind = i915_hdcp_component_master_unbind,
>> +};
>> +
>> +static int i915_hdcp_component_match(struct device *dev, void *data)
>> +{
>> +	return !strcmp(dev->driver->name, "mei_hdcp");
>> +}
>> +
>> +static int intel_hdcp_component_init(struct intel_connector *connector)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct i915_hdcp_component_master *comp;
>> +	struct component_match *match = NULL;
>> +	int ret;
>> +
>> +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
>> +	if (!comp)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&comp->mutex);
>> +	dev_priv->hdcp_comp = comp;
>> +
>> +	component_match_add(dev_priv->drm.dev, &match,
>> +			    i915_hdcp_component_match, dev_priv);
>> +	ret = component_master_add_with_match(dev_priv->drm.dev,
>> +					      &i915_hdcp_component_master_ops,
>> +					      match);
> So I'm not sure this will work out well, hiding the master_ops here in
> hdcp. Definitely needs to be rewritten as soon as i915 needs another
> component. And might make the load/unload split complicated.
we have already discussed this.
>
>> +	if (ret < 0)
>> +		goto out_err;
>> +
>> +	DRM_INFO("I915 hdcp component master added.\n");
> You add both the master and the hdcp component here. Output is a bit
> confusing.

we have component master and a component from mei which will match to 
the master.

Here we are adding the component master.

>
>> +	return ret;
>> +
>> +out_err:
>> +	component_master_del(dev_priv->drm.dev,
>> +			     &i915_hdcp_component_master_ops);
>> +	kfree(comp);
>> +	dev_priv->hdcp_comp = NULL;
>> +	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int initialize_mei_hdcp_data(struct intel_connector *connector)
>> +{
>> +	struct intel_hdcp *hdcp = &connector->hdcp;
>> +	struct mei_hdcp_data *data = &hdcp->mei_data;
>> +	enum port port;
>> +
>> +	if (connector->encoder) {
>> +		port = connector->encoder->port;
>> +		data->port = GET_MEI_DDI_INDEX(port);
>> +	}
>> +
>> +	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
>> +	data->protocol = hdcp->shim->hdcp_protocol();
>> +
>> +	data->k = 1;
>> +	if (!data->streams)
>> +		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
>> +					GFP_KERNEL);
>> +	if (!data->streams) {
>> +		DRM_ERROR("Out of Memory\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	data->streams[0].stream_id = 0;
>> +	data->streams[0].stream_type = hdcp->content_type;
>> +
>> +	return 0;
>> +}
>> +
>>   bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
>>   {
>>   	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
>> @@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
>>   {
>>   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>   	struct intel_hdcp *hdcp = &connector->hdcp;
>> +	int ret;
>>   
>>   	WARN_ON(!is_hdcp2_supported(dev_priv));
>>   
>> -	/* TODO: MEI interface needs to be initialized here */
>> +	ret = initialize_mei_hdcp_data(connector);
>> +	if (ret) {
>> +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
>> +		return;
>> +	}
>> +
>> +	if (!dev_priv->hdcp_comp)
>> +		ret = intel_hdcp_component_init(connector);
>> +
>> +	if (ret) {
>> +		DRM_DEBUG_KMS("HDCP comp init failed\n");
>> +		kfree(hdcp->mei_data.streams);
>> +		return;
>> +	}
>> +
>>   	hdcp->hdcp2_supported = 1;
>>   }
>>   
>> @@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
>>   		mutex_lock(&intel_connector->hdcp.mutex);
>>   		intel_connector->hdcp.hdcp2_supported = 0;
>>   		intel_connector->hdcp.shim = NULL;
>> +		kfree(intel_connector->hdcp.mei_data.streams);
> We need to push this into a per-connector hdcp cleanup function. Or just
> into the generic connector cleanup.
We could move it to per connector  hdcp cleanup function.
>
>>   		mutex_unlock(&intel_connector->hdcp.mutex);
>>   	}
>>   	drm_connector_list_iter_end(&conn_iter);
>> +
>> +	if (dev_priv->hdcp_comp) {
>> +		component_master_del(dev_priv->drm.dev,
>> +				     &i915_hdcp_component_master_ops);
>> +		kfree(dev_priv->hdcp_comp);
>> +		dev_priv->hdcp_comp = NULL;
>> +	}
>>   }
>>   
>>   int intel_hdcp_enable(struct intel_connector *connector)
>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>> index fca22d463e1b..12268228f4dc 100644
>> --- a/include/drm/i915_component.h
>> +++ b/include/drm/i915_component.h
>> @@ -24,8 +24,12 @@
>>   #ifndef _I915_COMPONENT_H_
>>   #define _I915_COMPONENT_H_
>>   
>> +#include <linux/mutex.h>
>> +
>>   #include "drm_audio_component.h"
>>   
>> +#include <drm/drm_hdcp.h>
>> +
>>   /* MAX_PORT is the number of port
>>    * It must be sync with I915_MAX_PORTS defined i915_drv.h
>>    */
>> @@ -46,4 +50,71 @@ struct i915_audio_component {
>>   	int aud_sample_rate[MAX_PORTS];
>>   };
>>   
>> +struct i915_hdcp_component_ops {
> Imo that should be called mei_hdcp_component_ops and put into the
> linux/mei_hdcp.h header. Or was that Thomas' review comment?

Nope this is there for many versions. i915_hdcp_component_ops are
implemented by mei_hdcp.c and initializes the component with the &ops.

>
> Aside: Review here in public channels instead of in private would be much
> better for coordination.

Tomas,

Could you please help on this ask.?

--Ram

>
>> +	/**
>> +	 * @owner: mei_hdcp module
>> +	 */
>> +	struct module *owner;
>> +
>> +	int (*initiate_hdcp2_session)(struct device *dev,
>> +				      void *hdcp_data,
>> +				      struct hdcp2_ake_init *ake_data);
>> +	int (*verify_receiver_cert_prepare_km)(struct device *dev,
>> +					       void *hdcp_data,
>> +					       struct hdcp2_ake_send_cert
>> +								*rx_cert,
>> +					       bool *km_stored,
>> +					       struct hdcp2_ake_no_stored_km
>> +								*ek_pub_km,
>> +					       size_t *msg_sz);
>> +	int (*verify_hprime)(struct device *dev,
>> +			     void *hdcp_data,
>> +			     struct hdcp2_ake_send_hprime *rx_hprime);
>> +	int (*store_pairing_info)(struct device *dev,
>> +				  void *hdcp_data,
>> +				  struct hdcp2_ake_send_pairing_info
>> +								*pairing_info);
>> +	int (*initiate_locality_check)(struct device *dev,
>> +				       void *hdcp_data,
>> +				       struct hdcp2_lc_init *lc_init_data);
>> +	int (*verify_lprime)(struct device *dev,
>> +			     void *hdcp_data,
>> +			     struct hdcp2_lc_send_lprime *rx_lprime);
>> +	int (*get_session_key)(struct device *dev,
>> +			       void *hdcp_data,
>> +			       struct hdcp2_ske_send_eks *ske_data);
>> +	int (*repeater_check_flow_prepare_ack)(struct device *dev,
>> +					       void *hdcp_data,
>> +					       struct hdcp2_rep_send_receiverid_list
>> +								*rep_topology,
>> +					       struct hdcp2_rep_send_ack
>> +								*rep_send_ack);
>> +	int (*verify_mprime)(struct device *dev,
>> +			     void *hdcp_data,
>> +			     struct hdcp2_rep_stream_ready *stream_ready);
>> +	int (*enable_hdcp_authentication)(struct device *dev,
>> +					  void *hdcp_data);
>> +	int (*close_hdcp_session)(struct device *dev,
>> +				  void *hdcp_data);
>> +};
>> +
>> +/**
>> + * struct i915_hdcp_component_master - Used for communication between i915
>> + * and mei_hdcp for HDCP2.2 services.
>> + */
>> +struct i915_hdcp_component_master {
>> +	/**
>> +	 * @dev: a device providing hdcp
>> +	 */
>> +	struct device *dev;
>> +	/**
>> +	 * @mutex: Mutex to protect the state of dev
>> +	 */
>> +	struct mutex mutex;
>> +	/**
>> +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
>> +	 */
>> +	const struct i915_hdcp_component_ops *ops;
>> +};
>> +
>>   #endif /* _I915_COMPONENT_H_ */
>> -- 
>> 2.7.4
>>
On 12/7/2018 11:22 AM, C, Ramalingam wrote:
>
>
> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
>> On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
>>> Defining the mei-i915 interface functions and initialization of
>>> the interface.
>>>
>>> Signed-off-by: Ramalingam C<ramalingam.c@intel.com>
>>> Signed-off-by: Tomas Winkler<tomas.winkler@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h   |   2 +
>>>   drivers/gpu/drm/i915/intel_drv.h  |   7 +
>>>   drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
>>>   include/drm/i915_component.h      |  71 ++++++
>>>   4 files changed, 521 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index f763b30f98d9..b68bc980b7cd 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2015,6 +2015,8 @@ struct drm_i915_private {
>>>   
>>>   	struct i915_pmu pmu;
>>>   
>>> +	struct i915_hdcp_component_master *hdcp_comp;
>>> +
>>>   	/*
>>>   	 * 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 85a526598096..bde82f3ada85 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -29,6 +29,7 @@
>>>   #include <linux/i2c.h>
>>>   #include <linux/hdmi.h>
>>>   #include <linux/sched/clock.h>
>>> +#include <linux/mei_hdcp.h>
>>>   #include <drm/i915_drm.h>
>>>   #include "i915_drv.h"
>>>   #include <drm/drm_crtc.h>
>>> @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
>>>   	/* Detects panel's hdcp capability. This is optional for HDMI. */
>>>   	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
>>>   			    bool *hdcp_capable);
>>> +
>>> +	/* Detects the HDCP protocol(DP/HDMI) required on the port */
>>> +	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
>> Looking ahead, this seems hardwired to constant return value? Or why do we
>> need a function here?
> This is hardwired based on the connector type(DP/HDMI).
> Since we have the shim for hdcp's connector based work, I have added this function.
>
> Could have done this just with connector_type check, but in that way whole hdcp_shim
> can be done in that way. So going with the larger design here.
>>>   };
>>>   
>>>   struct intel_hdcp {
>>> @@ -399,6 +403,9 @@ struct intel_hdcp {
>>>   	 * content can flow only through a link protected by HDCP2.2.
>>>   	 */
>>>   	u8 content_type;
>>> +
>>> +	/* mei interface related information */
>>> +	struct mei_hdcp_data mei_data;
>>>   };
>>>   
>>>   struct intel_connector {
>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>>> index 99dddb540958..760780f1105c 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>> @@ -8,14 +8,20 @@
>>>   
>>>   #include <drm/drmP.h>
>>>   #include <drm/drm_hdcp.h>
>>> +#include <drm/i915_component.h>
>>>   #include <linux/i2c.h>
>>>   #include <linux/random.h>
>>> +#include <linux/component.h>
>>>   
>>>   #include "intel_drv.h"
>>>   #include "i915_reg.h"
>>>   
>>>   #define KEY_LOAD_TRIES	5
>>>   #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
>>> +#define GET_MEI_DDI_INDEX(p) ({                            \
>>> +	typeof(p) __p = (p);                               \
>>> +	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
>>> +})
>>>   
>>>   static
>>>   bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>> @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>>>   		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
>>>   }
>>>   
>>> +static __attribute__((unused)) int
>>> +hdcp2_prepare_ake_init(struct intel_connector *connector,
>>> +		       struct hdcp2_ake_init *ake_data)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
>>> +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
>>> +
>>> +	/* Clear ME FW instance for the port, just incase */
>>> +	comp->ops->close_hdcp_session(comp->dev, data);
>> Sounds like a bug somewhere if we need this? I'd put this code into the
>> ->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.
> Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth along with ME FW.
> Now if authentication failed due to some reason, then the HDCP2.2 season created with
> ME FW for that port is not closed yet.
>
> So we need to call close_hdcp_session() explicitly on authentication failures.
> Session has to be closed before restarting the auth on that port with initialite_hdcp_session.
> If we are closing the hdcp session of the port on all auth errors, we dont need this just before
> start of the hdcp session.
>> "Just in case" papering over programming bugs of our own just makes
>> debugging harder.
>>
>>> +
>>> +	ret = comp->ops->initiate_hdcp2_session(comp->dev,
>>> +						data, ake_data);
>> We should set the port only after successfully initializing this.
> hdcp2_session is created for each port at ME FW. Hence we need the port
> initialized even before calling the initiate_hdcp2_session.
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>>> +				struct hdcp2_ake_send_cert *rx_cert,
>>> +				bool *paired,
>>> +				struct hdcp2_ake_no_stored_km *ek_pub_km,
>>> +				size_t *msg_sz)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> These all look like programming mistakes that should result in a WARN_ON.
> We are using the comp->mutex for protecting the interface during the interface
> init, usage for mei communication and interface teardown.
>
> But what if mei interface teardown happens between mei communications?
> So when we try to access the mei interface after such possible tear down scenario,
> we are checking the integrity of the interface.
>
> Possible alternate solution is hold the comp->mutex across the authentication steps.
> But consequence is that mei module removal will be blocked for authentication duration
> and even if the mei_dev is removed, component unbind will be blocked due to this mutex dependency.
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
>>> +							 rx_cert, paired,
>>> +							 ek_pub_km, msg_sz);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> Error handling here seems a bit strange. You close the session, but don't
>> reset the port. So next op will be totally unaware that things have blown
>> up. Also no warning.
>>
>> If we want to close the session, then I think that should be a decision
>> made higher up.
> This is needed as explained above. But as you have mentioned this can be moved
> to the end of the authentication on error scenario. I will work on that.
>>> +	mutex_unlock(&comp->mutex);
>> With component do we still need this mutex stuff here?
>>
>> Exact same comments everywhere below.
>>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_hprime(struct intel_connector *connector,
>>> +		    struct hdcp2_ake_send_hprime *rx_hprime)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_store_pairing_info(struct intel_connector *connector,
>>> +			 struct hdcp2_ake_send_pairing_info *pairing_info)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->store_pairing_info(comp->dev,
>>> +					    data, pairing_info);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_prepare_lc_init(struct intel_connector *connector,
>>> +		      struct hdcp2_lc_init *lc_init)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->initiate_locality_check(comp->dev,
>>> +						 data, lc_init);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_lprime(struct intel_connector *connector,
>>> +		    struct hdcp2_lc_send_lprime *rx_lprime)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_prepare_skey(struct intel_connector *connector,
>>> +		       struct hdcp2_ske_send_eks *ske_data)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
>>> +				      struct hdcp2_rep_send_receiverid_list
>>> +								*rep_topology,
>>> +				      struct hdcp2_rep_send_ack *rep_send_ack)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
>>> +							 data, rep_topology,
>>> +							 rep_send_ack);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_mprime(struct intel_connector *connector,
>>> +		    struct hdcp2_rep_stream_ready *stream_ready)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_authenticate_port(struct intel_connector *connector)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_close_mei_session(struct intel_connector *connector)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev ||
>>> +	    data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->close_hdcp_session(comp->dev, data);
>> Need to reset the port here I think.
> What is the reset of the port you are referring to? port is not
> configured for encryption. And this is the call for marking the port as de-authenticated too.
>>> +	mutex_unlock(&comp->mutex);
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_deauthenticate_port(struct intel_connector *connector)
>>> +{
>>> +	return hdcp2_close_mei_session(connector);
>>> +}
>>> +
>>> +static int i915_hdcp_component_master_bind(struct device *dev)
>>> +{
>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>> +
>>> +	return component_bind_all(dev, dev_priv->hdcp_comp);
>>> +}
>>> +
>>> +static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
>>> +{
>>> +	struct drm_device *dev = &dev_priv->drm;
>>> +	struct intel_connector *intel_connector;
>>> +	struct drm_connector *connector;
>>> +	struct drm_connector_list_iter conn_iter;
>>> +
>>> +	drm_connector_list_iter_begin(dev, &conn_iter);
>>> +	drm_for_each_connector_iter(connector, &conn_iter) {
>>> +		intel_connector = to_intel_connector(connector);
>>> +		if (!(intel_connector->hdcp.shim))
>>> +			continue;
>>> +
>>> +		intel_hdcp_disable(intel_connector);
>>> +	}
>>> +	drm_connector_list_iter_end(&conn_iter);
>>> +}
>>> +
>>> +static void i915_hdcp_component_master_unbind(struct device *dev)
>>> +{
>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>> +
>>> +	intel_connectors_hdcp_disable(dev_priv);
>> Why this code? Once we've unregistered the driver, we should have shut
>> down the hardware completely. Which should have shut down all the hdcp
>> users too.
> This unbind might be triggered either due to master_del or component_del.
> if its triggered from mei through component_del, then before teardown of
> the i/f in component_unbind(), disable the ongoing HDCP session through
> hdcp_disable() for each connectors.
>>> +	component_unbind_all(dev, dev_priv->hdcp_comp);
>>> +}
>>> +
>>> +static const struct component_master_ops i915_hdcp_component_master_ops = {
>>> +	.bind = i915_hdcp_component_master_bind,
>>> +	.unbind = i915_hdcp_component_master_unbind,
>>> +};
>>> +
>>> +static int i915_hdcp_component_match(struct device *dev, void *data)
>>> +{
>>> +	return !strcmp(dev->driver->name, "mei_hdcp");
>>> +}
>>> +
>>> +static int intel_hdcp_component_init(struct intel_connector *connector)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp;
>>> +	struct component_match *match = NULL;
>>> +	int ret;
>>> +
>>> +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
>>> +	if (!comp)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&comp->mutex);
>>> +	dev_priv->hdcp_comp = comp;
>>> +
>>> +	component_match_add(dev_priv->drm.dev, &match,
>>> +			    i915_hdcp_component_match, dev_priv);
>>> +	ret = component_master_add_with_match(dev_priv->drm.dev,
>>> +					      &i915_hdcp_component_master_ops,
>>> +					      match);
>> So I'm not sure this will work out well, hiding the master_ops here in
>> hdcp. Definitely needs to be rewritten as soon as i915 needs another
>> component. And might make the load/unload split complicated.
> we have already discussed this.
>>> +	if (ret < 0)
>>> +		goto out_err;
>>> +
>>> +	DRM_INFO("I915 hdcp component master added.\n");
>> You add both the master and the hdcp component here. Output is a bit
>> confusing.
>
> we have component master and a component from mei which will match to 
> the master.
>
> Here we are adding the component master.
>
>>> +	return ret;
>>> +
>>> +out_err:
>>> +	component_master_del(dev_priv->drm.dev,
>>> +			     &i915_hdcp_component_master_ops);
>>> +	kfree(comp);
>>> +	dev_priv->hdcp_comp = NULL;
>>> +	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int initialize_mei_hdcp_data(struct intel_connector *connector)
>>> +{
>>> +	struct intel_hdcp *hdcp = &connector->hdcp;
>>> +	struct mei_hdcp_data *data = &hdcp->mei_data;
>>> +	enum port port;
>>> +
>>> +	if (connector->encoder) {
>>> +		port = connector->encoder->port;
>>> +		data->port = GET_MEI_DDI_INDEX(port);
>>> +	}
>>> +
>>> +	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
>>> +	data->protocol = hdcp->shim->hdcp_protocol();
>>> +
>>> +	data->k = 1;
>>> +	if (!data->streams)
>>> +		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
>>> +					GFP_KERNEL);
>>> +	if (!data->streams) {
>>> +		DRM_ERROR("Out of Memory\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	data->streams[0].stream_id = 0;
>>> +	data->streams[0].stream_type = hdcp->content_type;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
>>>   {
>>>   	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
>>> @@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
>>>   {
>>>   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>   	struct intel_hdcp *hdcp = &connector->hdcp;
>>> +	int ret;
>>>   
>>>   	WARN_ON(!is_hdcp2_supported(dev_priv));
>>>   
>>> -	/* TODO: MEI interface needs to be initialized here */
>>> +	ret = initialize_mei_hdcp_data(connector);
>>> +	if (ret) {
>>> +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
>>> +		return;
>>> +	}
>>> +
>>> +	if (!dev_priv->hdcp_comp)
>>> +		ret = intel_hdcp_component_init(connector);
>>> +
>>> +	if (ret) {
>>> +		DRM_DEBUG_KMS("HDCP comp init failed\n");
>>> +		kfree(hdcp->mei_data.streams);
>>> +		return;
>>> +	}
>>> +
>>>   	hdcp->hdcp2_supported = 1;
>>>   }
>>>   
>>> @@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
>>>   		mutex_lock(&intel_connector->hdcp.mutex);
>>>   		intel_connector->hdcp.hdcp2_supported = 0;
>>>   		intel_connector->hdcp.shim = NULL;
>>> +		kfree(intel_connector->hdcp.mei_data.streams);
>> We need to push this into a per-connector hdcp cleanup function. Or just
>> into the generic connector cleanup.
> We could move it to per connector  hdcp cleanup function.
>>>   		mutex_unlock(&intel_connector->hdcp.mutex);
>>>   	}
>>>   	drm_connector_list_iter_end(&conn_iter);
>>> +
>>> +	if (dev_priv->hdcp_comp) {
>>> +		component_master_del(dev_priv->drm.dev,
>>> +				     &i915_hdcp_component_master_ops);
>>> +		kfree(dev_priv->hdcp_comp);
>>> +		dev_priv->hdcp_comp = NULL;
>>> +	}
>>>   }
>>>   
>>>   int intel_hdcp_enable(struct intel_connector *connector)
>>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>>> index fca22d463e1b..12268228f4dc 100644
>>> --- a/include/drm/i915_component.h
>>> +++ b/include/drm/i915_component.h
>>> @@ -24,8 +24,12 @@
>>>   #ifndef _I915_COMPONENT_H_
>>>   #define _I915_COMPONENT_H_
>>>   
>>> +#include <linux/mutex.h>
>>> +
>>>   #include "drm_audio_component.h"
>>>   
>>> +#include <drm/drm_hdcp.h>
>>> +
>>>   /* MAX_PORT is the number of port
>>>    * It must be sync with I915_MAX_PORTS defined i915_drv.h
>>>    */
>>> @@ -46,4 +50,71 @@ struct i915_audio_component {
>>>   	int aud_sample_rate[MAX_PORTS];
>>>   };
>>>   
>>> +struct i915_hdcp_component_ops {
>> Imo that should be called mei_hdcp_component_ops and put into the
>> linux/mei_hdcp.h header. Or was that Thomas' review comment?
> Nope this is there for many versions. i915_hdcp_component_ops are
> implemented by mei_hdcp.c and initializes the component with the &ops.

Tomas and Daniel,

Is this fine? Defining the ops at i915_component.h and I915 and mei_hdcp
using it for their purpose?

If it has to be compulsorily defined by the service provider then we need
to move this into the include/linux/mei_hdcp.h. In that we can avoid the
global header from the mei_hdcp Tomas. Please help here to clarify the direction.

-Ram

>> Aside: Review here in public channels instead of in private would be much
>> better for coordination.
> Tomas,
> Could you please help on this ask.?
> --Ram
>>> +	/**
>>> +	 * @owner: mei_hdcp module
>>> +	 */
>>> +	struct module *owner;
>>> +
>>> +	int (*initiate_hdcp2_session)(struct device *dev,
>>> +				      void *hdcp_data,
>>> +				      struct hdcp2_ake_init *ake_data);
>>> +	int (*verify_receiver_cert_prepare_km)(struct device *dev,
>>> +					       void *hdcp_data,
>>> +					       struct hdcp2_ake_send_cert
>>> +								*rx_cert,
>>> +					       bool *km_stored,
>>> +					       struct hdcp2_ake_no_stored_km
>>> +								*ek_pub_km,
>>> +					       size_t *msg_sz);
>>> +	int (*verify_hprime)(struct device *dev,
>>> +			     void *hdcp_data,
>>> +			     struct hdcp2_ake_send_hprime *rx_hprime);
>>> +	int (*store_pairing_info)(struct device *dev,
>>> +				  void *hdcp_data,
>>> +				  struct hdcp2_ake_send_pairing_info
>>> +								*pairing_info);
>>> +	int (*initiate_locality_check)(struct device *dev,
>>> +				       void *hdcp_data,
>>> +				       struct hdcp2_lc_init *lc_init_data);
>>> +	int (*verify_lprime)(struct device *dev,
>>> +			     void *hdcp_data,
>>> +			     struct hdcp2_lc_send_lprime *rx_lprime);
>>> +	int (*get_session_key)(struct device *dev,
>>> +			       void *hdcp_data,
>>> +			       struct hdcp2_ske_send_eks *ske_data);
>>> +	int (*repeater_check_flow_prepare_ack)(struct device *dev,
>>> +					       void *hdcp_data,
>>> +					       struct hdcp2_rep_send_receiverid_list
>>> +								*rep_topology,
>>> +					       struct hdcp2_rep_send_ack
>>> +								*rep_send_ack);
>>> +	int (*verify_mprime)(struct device *dev,
>>> +			     void *hdcp_data,
>>> +			     struct hdcp2_rep_stream_ready *stream_ready);
>>> +	int (*enable_hdcp_authentication)(struct device *dev,
>>> +					  void *hdcp_data);
>>> +	int (*close_hdcp_session)(struct device *dev,
>>> +				  void *hdcp_data);
>>> +};
>>> +
>>> +/**
>>> + * struct i915_hdcp_component_master - Used for communication between i915
>>> + * and mei_hdcp for HDCP2.2 services.
>>> + */
>>> +struct i915_hdcp_component_master {
>>> +	/**
>>> +	 * @dev: a device providing hdcp
>>> +	 */
>>> +	struct device *dev;
>>> +	/**
>>> +	 * @mutex: Mutex to protect the state of dev
>>> +	 */
>>> +	struct mutex mutex;
>>> +	/**
>>> +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
>>> +	 */
>>> +	const struct i915_hdcp_component_ops *ops;
>>> +};
>>> +
>>>   #endif /* _I915_COMPONENT_H_ */
>>> -- 
>>> 2.7.4
>>>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/7/2018 11:22 AM, C, Ramalingam wrote:
>
>
> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
>> On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
>>> Defining the mei-i915 interface functions and initialization of
>>> the interface.
>>>
>>> Signed-off-by: Ramalingam C<ramalingam.c@intel.com>
>>> Signed-off-by: Tomas Winkler<tomas.winkler@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h   |   2 +
>>>   drivers/gpu/drm/i915/intel_drv.h  |   7 +
>>>   drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
>>>   include/drm/i915_component.h      |  71 ++++++
>>>   4 files changed, 521 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index f763b30f98d9..b68bc980b7cd 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2015,6 +2015,8 @@ struct drm_i915_private {
>>>   
>>>   	struct i915_pmu pmu;
>>>   
>>> +	struct i915_hdcp_component_master *hdcp_comp;
>>> +
>>>   	/*
>>>   	 * 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 85a526598096..bde82f3ada85 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -29,6 +29,7 @@
>>>   #include <linux/i2c.h>
>>>   #include <linux/hdmi.h>
>>>   #include <linux/sched/clock.h>
>>> +#include <linux/mei_hdcp.h>
>>>   #include <drm/i915_drm.h>
>>>   #include "i915_drv.h"
>>>   #include <drm/drm_crtc.h>
>>> @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
>>>   	/* Detects panel's hdcp capability. This is optional for HDMI. */
>>>   	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
>>>   			    bool *hdcp_capable);
>>> +
>>> +	/* Detects the HDCP protocol(DP/HDMI) required on the port */
>>> +	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
>> Looking ahead, this seems hardwired to constant return value? Or why do we
>> need a function here?
> This is hardwired based on the connector type(DP/HDMI).
> Since we have the shim for hdcp's connector based work, I have added this function.
>
> Could have done this just with connector_type check, but in that way whole hdcp_shim
> can be done in that way. So going with the larger design here.
>>>   };
>>>   
>>>   struct intel_hdcp {
>>> @@ -399,6 +403,9 @@ struct intel_hdcp {
>>>   	 * content can flow only through a link protected by HDCP2.2.
>>>   	 */
>>>   	u8 content_type;
>>> +
>>> +	/* mei interface related information */
>>> +	struct mei_hdcp_data mei_data;
>>>   };
>>>   
>>>   struct intel_connector {
>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>>> index 99dddb540958..760780f1105c 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>> @@ -8,14 +8,20 @@
>>>   
>>>   #include <drm/drmP.h>
>>>   #include <drm/drm_hdcp.h>
>>> +#include <drm/i915_component.h>
>>>   #include <linux/i2c.h>
>>>   #include <linux/random.h>
>>> +#include <linux/component.h>
>>>   
>>>   #include "intel_drv.h"
>>>   #include "i915_reg.h"
>>>   
>>>   #define KEY_LOAD_TRIES	5
>>>   #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
>>> +#define GET_MEI_DDI_INDEX(p) ({                            \
>>> +	typeof(p) __p = (p);                               \
>>> +	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
>>> +})
>>>   
>>>   static
>>>   bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>> @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>>>   		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
>>>   }
>>>   
>>> +static __attribute__((unused)) int
>>> +hdcp2_prepare_ake_init(struct intel_connector *connector,
>>> +		       struct hdcp2_ake_init *ake_data)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
>>> +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
>>> +
>>> +	/* Clear ME FW instance for the port, just incase */
>>> +	comp->ops->close_hdcp_session(comp->dev, data);
>> Sounds like a bug somewhere if we need this? I'd put this code into the
>> ->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.
> Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth along with ME FW.
> Now if authentication failed due to some reason, then the HDCP2.2 season created with
> ME FW for that port is not closed yet.
>
> So we need to call close_hdcp_session() explicitly on authentication failures.
> Session has to be closed before restarting the auth on that port with initialite_hdcp_session.
> If we are closing the hdcp session of the port on all auth errors, we dont need this just before
> start of the hdcp session.
>> "Just in case" papering over programming bugs of our own just makes
>> debugging harder.
>>
>>> +
>>> +	ret = comp->ops->initiate_hdcp2_session(comp->dev,
>>> +						data, ake_data);
>> We should set the port only after successfully initializing this.
> hdcp2_session is created for each port at ME FW. Hence we need the port
> initialized even before calling the initiate_hdcp2_session.
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>>> +				struct hdcp2_ake_send_cert *rx_cert,
>>> +				bool *paired,
>>> +				struct hdcp2_ake_no_stored_km *ek_pub_km,
>>> +				size_t *msg_sz)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>> These all look like programming mistakes that should result in a WARN_ON.
> We are using the comp->mutex for protecting the interface during the interface
> init, usage for mei communication and interface teardown.
>
> But what if mei interface teardown happens between mei communications?
> So when we try to access the mei interface after such possible tear down scenario,
> we are checking the integrity of the interface.
>
> Possible alternate solution is hold the comp->mutex across the authentication steps.
> But consequence is that mei module removal will be blocked for authentication duration
> and even if the mei_dev is removed, component unbind will be blocked due to this mutex dependency.
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
>>> +							 rx_cert, paired,
>>> +							 ek_pub_km, msg_sz);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>> Error handling here seems a bit strange. You close the session, but don't
>> reset the port. So next op will be totally unaware that things have blown
>> up. Also no warning.
>>
>> If we want to close the session, then I think that should be a decision
>> made higher up.
> This is needed as explained above. But as you have mentioned this can be moved
> to the end of the authentication on error scenario. I will work on that.
>>> +	mutex_unlock(&comp->mutex);
>> With component do we still need this mutex stuff here?
>>
>> Exact same comments everywhere below.
>>
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_hprime(struct intel_connector *connector,
>>> +		    struct hdcp2_ake_send_hprime *rx_hprime)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_store_pairing_info(struct intel_connector *connector,
>>> +			 struct hdcp2_ake_send_pairing_info *pairing_info)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->store_pairing_info(comp->dev,
>>> +					    data, pairing_info);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_prepare_lc_init(struct intel_connector *connector,
>>> +		      struct hdcp2_lc_init *lc_init)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->initiate_locality_check(comp->dev,
>>> +						 data, lc_init);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_lprime(struct intel_connector *connector,
>>> +		    struct hdcp2_lc_send_lprime *rx_lprime)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_prepare_skey(struct intel_connector *connector,
>>> +		       struct hdcp2_ske_send_eks *ske_data)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
>>> +				      struct hdcp2_rep_send_receiverid_list
>>> +								*rep_topology,
>>> +				      struct hdcp2_rep_send_ack *rep_send_ack)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
>>> +							 data, rep_topology,
>>> +							 rep_send_ack);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused)) int
>>> +hdcp2_verify_mprime(struct intel_connector *connector,
>>> +		    struct hdcp2_rep_stream_ready *stream_ready)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_authenticate_port(struct intel_connector *connector)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
>>> +	if (ret < 0)
>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> +	mutex_unlock(&comp->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_close_mei_session(struct intel_connector *connector)
>>> +{
>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>> +	int ret;
>>> +
>>> +	if (!comp)
>>> +		return -EINVAL;
>>> +
>>> +	mutex_lock(&comp->mutex);
>>> +	if (!comp->ops || !comp->dev ||
>>> +	    data->port == MEI_DDI_INVALID_PORT) {
>>> +		mutex_unlock(&comp->mutex);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = comp->ops->close_hdcp_session(comp->dev, data);
>> Need to reset the port here I think.
> What is the reset of the port you are referring to? port is not
> configured for encryption. And this is the call for marking the port as de-authenticated too.
>>> +	mutex_unlock(&comp->mutex);
>>> +	return ret;
>>> +}
>>> +
>>> +static __attribute__((unused))
>>> +int hdcp2_deauthenticate_port(struct intel_connector *connector)
>>> +{
>>> +	return hdcp2_close_mei_session(connector);
>>> +}
>>> +
>>> +static int i915_hdcp_component_master_bind(struct device *dev)
>>> +{
>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>> +
>>> +	return component_bind_all(dev, dev_priv->hdcp_comp);
>>> +}
>>> +
>>> +static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
>>> +{
>>> +	struct drm_device *dev = &dev_priv->drm;
>>> +	struct intel_connector *intel_connector;
>>> +	struct drm_connector *connector;
>>> +	struct drm_connector_list_iter conn_iter;
>>> +
>>> +	drm_connector_list_iter_begin(dev, &conn_iter);
>>> +	drm_for_each_connector_iter(connector, &conn_iter) {
>>> +		intel_connector = to_intel_connector(connector);
>>> +		if (!(intel_connector->hdcp.shim))
>>> +			continue;
>>> +
>>> +		intel_hdcp_disable(intel_connector);
>>> +	}
>>> +	drm_connector_list_iter_end(&conn_iter);
>>> +}
>>> +
>>> +static void i915_hdcp_component_master_unbind(struct device *dev)
>>> +{
>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>> +
>>> +	intel_connectors_hdcp_disable(dev_priv);
>> Why this code? Once we've unregistered the driver, we should have shut
>> down the hardware completely. Which should have shut down all the hdcp
>> users too.
> This unbind might be triggered either due to master_del or component_del.
> if its triggered from mei through component_del, then before teardown of
> the i/f in component_unbind(), disable the ongoing HDCP session through
> hdcp_disable() for each connectors.
>>> +	component_unbind_all(dev, dev_priv->hdcp_comp);
>>> +}
>>> +
>>> +static const struct component_master_ops i915_hdcp_component_master_ops = {
>>> +	.bind = i915_hdcp_component_master_bind,
>>> +	.unbind = i915_hdcp_component_master_unbind,
>>> +};
>>> +
>>> +static int i915_hdcp_component_match(struct device *dev, void *data)
>>> +{
>>> +	return !strcmp(dev->driver->name, "mei_hdcp");
>>> +}
>>> +
>>> +static int intel_hdcp_component_init(struct intel_connector *connector)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>> +	struct i915_hdcp_component_master *comp;
>>> +	struct component_match *match = NULL;
>>> +	int ret;
>>> +
>>> +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
>>> +	if (!comp)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&comp->mutex);
>>> +	dev_priv->hdcp_comp = comp;
>>> +
>>> +	component_match_add(dev_priv->drm.dev, &match,
>>> +			    i915_hdcp_component_match, dev_priv);
>>> +	ret = component_master_add_with_match(dev_priv->drm.dev,
>>> +					      &i915_hdcp_component_master_ops,
>>> +					      match);
>> So I'm not sure this will work out well, hiding the master_ops here in
>> hdcp. Definitely needs to be rewritten as soon as i915 needs another
>> component. And might make the load/unload split complicated.
> we have already discussed this.
>>> +	if (ret < 0)
>>> +		goto out_err;
>>> +
>>> +	DRM_INFO("I915 hdcp component master added.\n");
>> You add both the master and the hdcp component here. Output is a bit
>> confusing.
>
> we have component master and a component from mei which will match to 
> the master.
>
> Here we are adding the component master.
>
>>> +	return ret;
>>> +
>>> +out_err:
>>> +	component_master_del(dev_priv->drm.dev,
>>> +			     &i915_hdcp_component_master_ops);
>>> +	kfree(comp);
>>> +	dev_priv->hdcp_comp = NULL;
>>> +	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int initialize_mei_hdcp_data(struct intel_connector *connector)
>>> +{
>>> +	struct intel_hdcp *hdcp = &connector->hdcp;
>>> +	struct mei_hdcp_data *data = &hdcp->mei_data;
>>> +	enum port port;
>>> +
>>> +	if (connector->encoder) {
>>> +		port = connector->encoder->port;
>>> +		data->port = GET_MEI_DDI_INDEX(port);
>>> +	}
>>> +
>>> +	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
>>> +	data->protocol = hdcp->shim->hdcp_protocol();
>>> +
>>> +	data->k = 1;
>>> +	if (!data->streams)
>>> +		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
>>> +					GFP_KERNEL);
>>> +	if (!data->streams) {
>>> +		DRM_ERROR("Out of Memory\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	data->streams[0].stream_id = 0;
>>> +	data->streams[0].stream_type = hdcp->content_type;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
>>>   {
>>>   	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
>>> @@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
>>>   {
>>>   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>   	struct intel_hdcp *hdcp = &connector->hdcp;
>>> +	int ret;
>>>   
>>>   	WARN_ON(!is_hdcp2_supported(dev_priv));
>>>   
>>> -	/* TODO: MEI interface needs to be initialized here */
>>> +	ret = initialize_mei_hdcp_data(connector);
>>> +	if (ret) {
>>> +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
>>> +		return;
>>> +	}
>>> +
>>> +	if (!dev_priv->hdcp_comp)
>>> +		ret = intel_hdcp_component_init(connector);
>>> +
>>> +	if (ret) {
>>> +		DRM_DEBUG_KMS("HDCP comp init failed\n");
>>> +		kfree(hdcp->mei_data.streams);
>>> +		return;
>>> +	}
>>> +
>>>   	hdcp->hdcp2_supported = 1;
>>>   }
>>>   
>>> @@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
>>>   		mutex_lock(&intel_connector->hdcp.mutex);
>>>   		intel_connector->hdcp.hdcp2_supported = 0;
>>>   		intel_connector->hdcp.shim = NULL;
>>> +		kfree(intel_connector->hdcp.mei_data.streams);
>> We need to push this into a per-connector hdcp cleanup function. Or just
>> into the generic connector cleanup.
> We could move it to per connector  hdcp cleanup function.
>>>   		mutex_unlock(&intel_connector->hdcp.mutex);
>>>   	}
>>>   	drm_connector_list_iter_end(&conn_iter);
>>> +
>>> +	if (dev_priv->hdcp_comp) {
>>> +		component_master_del(dev_priv->drm.dev,
>>> +				     &i915_hdcp_component_master_ops);
>>> +		kfree(dev_priv->hdcp_comp);
>>> +		dev_priv->hdcp_comp = NULL;
>>> +	}
>>>   }
>>>   
>>>   int intel_hdcp_enable(struct intel_connector *connector)
>>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>>> index fca22d463e1b..12268228f4dc 100644
>>> --- a/include/drm/i915_component.h
>>> +++ b/include/drm/i915_component.h
>>> @@ -24,8 +24,12 @@
>>>   #ifndef _I915_COMPONENT_H_
>>>   #define _I915_COMPONENT_H_
>>>   
>>> +#include <linux/mutex.h>
>>> +
>>>   #include "drm_audio_component.h"
>>>   
>>> +#include <drm/drm_hdcp.h>
>>> +
>>>   /* MAX_PORT is the number of port
>>>    * It must be sync with I915_MAX_PORTS defined i915_drv.h
>>>    */
>>> @@ -46,4 +50,71 @@ struct i915_audio_component {
>>>   	int aud_sample_rate[MAX_PORTS];
>>>   };
>>>   
>>> +struct i915_hdcp_component_ops {
>> Imo that should be called mei_hdcp_component_ops and put into the
>> linux/mei_hdcp.h header. Or was that Thomas' review comment?
> Nope this is there for many versions. i915_hdcp_component_ops are
> implemented by mei_hdcp.c and initializes the component with the &ops.

Tomas and Daniel,

Is this fine? Defining the ops at i915_component.h and I915 and mei_hdcp
using it for their purpose?

If it has to be compulsorily defined by the service provider then we need
to move this into the include/linux/mei_hdcp.h. In that we can avoid the
global header from the mei_hdcp Tomas. Please help here to clarify the direction.

-Ram

>> Aside: Review here in public channels instead of in private would be much
>> better for coordination.
> Tomas,
> Could you please help on this ask.?
> --Ram
>>> +	/**
>>> +	 * @owner: mei_hdcp module
>>> +	 */
>>> +	struct module *owner;
>>> +
>>> +	int (*initiate_hdcp2_session)(struct device *dev,
>>> +				      void *hdcp_data,
>>> +				      struct hdcp2_ake_init *ake_data);
>>> +	int (*verify_receiver_cert_prepare_km)(struct device *dev,
>>> +					       void *hdcp_data,
>>> +					       struct hdcp2_ake_send_cert
>>> +								*rx_cert,
>>> +					       bool *km_stored,
>>> +					       struct hdcp2_ake_no_stored_km
>>> +								*ek_pub_km,
>>> +					       size_t *msg_sz);
>>> +	int (*verify_hprime)(struct device *dev,
>>> +			     void *hdcp_data,
>>> +			     struct hdcp2_ake_send_hprime *rx_hprime);
>>> +	int (*store_pairing_info)(struct device *dev,
>>> +				  void *hdcp_data,
>>> +				  struct hdcp2_ake_send_pairing_info
>>> +								*pairing_info);
>>> +	int (*initiate_locality_check)(struct device *dev,
>>> +				       void *hdcp_data,
>>> +				       struct hdcp2_lc_init *lc_init_data);
>>> +	int (*verify_lprime)(struct device *dev,
>>> +			     void *hdcp_data,
>>> +			     struct hdcp2_lc_send_lprime *rx_lprime);
>>> +	int (*get_session_key)(struct device *dev,
>>> +			       void *hdcp_data,
>>> +			       struct hdcp2_ske_send_eks *ske_data);
>>> +	int (*repeater_check_flow_prepare_ack)(struct device *dev,
>>> +					       void *hdcp_data,
>>> +					       struct hdcp2_rep_send_receiverid_list
>>> +								*rep_topology,
>>> +					       struct hdcp2_rep_send_ack
>>> +								*rep_send_ack);
>>> +	int (*verify_mprime)(struct device *dev,
>>> +			     void *hdcp_data,
>>> +			     struct hdcp2_rep_stream_ready *stream_ready);
>>> +	int (*enable_hdcp_authentication)(struct device *dev,
>>> +					  void *hdcp_data);
>>> +	int (*close_hdcp_session)(struct device *dev,
>>> +				  void *hdcp_data);
>>> +};
>>> +
>>> +/**
>>> + * struct i915_hdcp_component_master - Used for communication between i915
>>> + * and mei_hdcp for HDCP2.2 services.
>>> + */
>>> +struct i915_hdcp_component_master {
>>> +	/**
>>> +	 * @dev: a device providing hdcp
>>> +	 */
>>> +	struct device *dev;
>>> +	/**
>>> +	 * @mutex: Mutex to protect the state of dev
>>> +	 */
>>> +	struct mutex mutex;
>>> +	/**
>>> +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
>>> +	 */
>>> +	const struct i915_hdcp_component_ops *ops;
>>> +};
>>> +
>>>   #endif /* _I915_COMPONENT_H_ */
>>> -- 
>>> 2.7.4
>>>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
> 
> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > Defining the mei-i915 interface functions and initialization of
> > > the interface.
> > > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.h   |   2 +
> > >   drivers/gpu/drm/i915/intel_drv.h  |   7 +
> > >   drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
> > >   include/drm/i915_component.h      |  71 ++++++
> > >   4 files changed, 521 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index f763b30f98d9..b68bc980b7cd 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2015,6 +2015,8 @@ struct drm_i915_private {
> > >   	struct i915_pmu pmu;
> > > +	struct i915_hdcp_component_master *hdcp_comp;
> > > +
> > >   	/*
> > >   	 * 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 85a526598096..bde82f3ada85 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -29,6 +29,7 @@
> > >   #include <linux/i2c.h>
> > >   #include <linux/hdmi.h>
> > >   #include <linux/sched/clock.h>
> > > +#include <linux/mei_hdcp.h>
> > >   #include <drm/i915_drm.h>
> > >   #include "i915_drv.h"
> > >   #include <drm/drm_crtc.h>
> > > @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
> > >   	/* Detects panel's hdcp capability. This is optional for HDMI. */
> > >   	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> > >   			    bool *hdcp_capable);
> > > +
> > > +	/* Detects the HDCP protocol(DP/HDMI) required on the port */
> > > +	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
> > Looking ahead, this seems hardwired to constant return value? Or why do we
> > need a function here?
> 
> This is hardwired based on the connector type(DP/HDMI).
> Since we have the shim for hdcp's connector based work, I have added this function.
> 
> Could have done this just with connector_type check, but in that way whole hdcp_shim
> can be done in that way. So going with the larger design here.

If it's hardwired then just make it a hardwired struct member. As long as
it's all const, we can mix data an function pointers. If you have runtime
variable data, then it's better to split it out from the ops structure, so
that we can keep ops read-only and protected against possible exploits
(any function pointers are a high-value target in the kernel).

> 
> > 
> > >   };
> > >   struct intel_hdcp {
> > > @@ -399,6 +403,9 @@ struct intel_hdcp {
> > >   	 * content can flow only through a link protected by HDCP2.2.
> > >   	 */
> > >   	u8 content_type;
> > > +
> > > +	/* mei interface related information */
> > > +	struct mei_hdcp_data mei_data;
> > >   };
> > >   struct intel_connector {
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index 99dddb540958..760780f1105c 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -8,14 +8,20 @@
> > >   #include <drm/drmP.h>
> > >   #include <drm/drm_hdcp.h>
> > > +#include <drm/i915_component.h>
> > >   #include <linux/i2c.h>
> > >   #include <linux/random.h>
> > > +#include <linux/component.h>
> > >   #include "intel_drv.h"
> > >   #include "i915_reg.h"
> > >   #define KEY_LOAD_TRIES	5
> > >   #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
> > > +#define GET_MEI_DDI_INDEX(p) ({                            \
> > > +	typeof(p) __p = (p);                               \
> > > +	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
> > > +})
> > >   static
> > >   bool intel_hdcp_is_ksv_valid(u8 *ksv)
> > > @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
> > >   		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
> > >   }
> > > +static __attribute__((unused)) int
> > > +hdcp2_prepare_ake_init(struct intel_connector *connector,
> > > +		       struct hdcp2_ake_init *ake_data)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> > > +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
> > > +
> > > +	/* Clear ME FW instance for the port, just incase */
> > > +	comp->ops->close_hdcp_session(comp->dev, data);
> > Sounds like a bug somewhere if we need this? I'd put this code into the
> > ->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.
> 
> Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth along with ME FW.
> Now if authentication failed due to some reason, then the HDCP2.2 season created with
> ME FW for that port is not closed yet.
> 
> So we need to call close_hdcp_session() explicitly on authentication failures.
> Session has to be closed before restarting the auth on that port with initialite_hdcp_session.
> If we are closing the hdcp session of the port on all auth errors, we dont need this just before
> start of the hdcp session.

Yeah, makes sense. But right now the close_session stuff is sprinkled all
over, so it's hard to see what's going on and whether there's bugs
somewhere. I much prefer code that looks like it knows what it's doing. I
think one top-level close_session call in the main hdcp2 functions (called
when everything is done, or on failure) would be much cleaner.

Plus then some WARN_ON to make sure we never forget to close a session.
Sprinkling cleanup code all over the place looks like it's easier, but
long term it's much harder code to maintain and refactor because you never
sure which cleanup code is actually doing the cleanup, and which cleanup
code is just there for nothing.
> 
> > 
> > "Just in case" papering over programming bugs of our own just makes
> > debugging harder.
> > 
> > > +
> > > +	ret = comp->ops->initiate_hdcp2_session(comp->dev,
> > > +						data, ake_data);
> > We should set the port only after successfully initializing this.
> 
> hdcp2_session is created for each port at ME FW. Hence we need the port
> initialized even before calling the initiate_hdcp2_session.

> > > +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> > > +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);

This line right above in your patch should only run on success. I think.

Also, looking at this again: Why could connector->encoder ever be NULL?
That smells like another possible bug in our setup code. Better to wrap
this in a WARN_ON and bail out.


> 
> > 
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
> > > +				struct hdcp2_ake_send_cert *rx_cert,
> > > +				bool *paired,
> > > +				struct hdcp2_ake_no_stored_km *ek_pub_km,
> > > +				size_t *msg_sz)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > These all look like programming mistakes that should result in a WARN_ON.
> 
> We are using the comp->mutex for protecting the interface during the interface
> init, usage for mei communication and interface teardown.
> 
> But what if mei interface teardown happens between mei communications?
> So when we try to access the mei interface after such possible tear down scenario,
> we are checking the integrity of the interface.
> 
> Possible alternate solution is hold the comp->mutex across the authentication steps.
> But consequence is that mei module removal will be blocked for authentication duration
> and even if the mei_dev is removed, component unbind will be blocked due to this mutex dependency.

I think with the v7 component code this should all be impossible and we
can remove it. With the v8 component code it's definitely necessary (which
is why I don't like the v8 component code). This is all because I didn't
fully realize the changes with v8 when reading the patches again ...

> 
> > 
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
> > > +							 rx_cert, paired,
> > > +							 ek_pub_km, msg_sz);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > Error handling here seems a bit strange. You close the session, but don't
> > reset the port. So next op will be totally unaware that things have blown
> > up. Also no warning.
> > 
> > If we want to close the session, then I think that should be a decision
> > made higher up.
> 
> This is needed as explained above. But as you have mentioned this can be moved
> to the end of the authentication on error scenario. I will work on that.

Yeah, I think that'll lead to cleaner code.

> 
> > 
> > > +	mutex_unlock(&comp->mutex);
> > With component do we still need this mutex stuff here?
> > 
> > Exact same comments everywhere below.
> > 
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_hprime(struct intel_connector *connector,
> > > +		    struct hdcp2_ake_send_hprime *rx_hprime)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_store_pairing_info(struct intel_connector *connector,
> > > +			 struct hdcp2_ake_send_pairing_info *pairing_info)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->store_pairing_info(comp->dev,
> > > +					    data, pairing_info);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_prepare_lc_init(struct intel_connector *connector,
> > > +		      struct hdcp2_lc_init *lc_init)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->initiate_locality_check(comp->dev,
> > > +						 data, lc_init);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_lprime(struct intel_connector *connector,
> > > +		    struct hdcp2_lc_send_lprime *rx_lprime)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_prepare_skey(struct intel_connector *connector,
> > > +		       struct hdcp2_ske_send_eks *ske_data)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
> > > +				      struct hdcp2_rep_send_receiverid_list
> > > +								*rep_topology,
> > > +				      struct hdcp2_rep_send_ack *rep_send_ack)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
> > > +							 data, rep_topology,
> > > +							 rep_send_ack);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused)) int
> > > +hdcp2_verify_mprime(struct intel_connector *connector,
> > > +		    struct hdcp2_rep_stream_ready *stream_ready)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_authenticate_port(struct intel_connector *connector)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
> > > +	if (ret < 0)
> > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > +	mutex_unlock(&comp->mutex);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_close_mei_session(struct intel_connector *connector)
> > > +{
> > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > +	int ret;
> > > +
> > > +	if (!comp)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&comp->mutex);
> > > +	if (!comp->ops || !comp->dev ||
> > > +	    data->port == MEI_DDI_INVALID_PORT) {
> > > +		mutex_unlock(&comp->mutex);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = comp->ops->close_hdcp_session(comp->dev, data);
> > Need to reset the port here I think.
> 
> What is the reset of the port you are referring to? port is not
> configured for encryption. And this is the call for marking the port as de-authenticated too.
> 
> > 
> > > +	mutex_unlock(&comp->mutex);
> > > +	return ret;
> > > +}
> > > +
> > > +static __attribute__((unused))
> > > +int hdcp2_deauthenticate_port(struct intel_connector *connector)
> > > +{
> > > +	return hdcp2_close_mei_session(connector);
> > > +}
> > > +
> > > +static int i915_hdcp_component_master_bind(struct device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > +
> > > +	return component_bind_all(dev, dev_priv->hdcp_comp);
> > > +}
> > > +
> > > +static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct drm_device *dev = &dev_priv->drm;
> > > +	struct intel_connector *intel_connector;
> > > +	struct drm_connector *connector;
> > > +	struct drm_connector_list_iter conn_iter;
> > > +
> > > +	drm_connector_list_iter_begin(dev, &conn_iter);
> > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > +		intel_connector = to_intel_connector(connector);
> > > +		if (!(intel_connector->hdcp.shim))
> > > +			continue;
> > > +
> > > +		intel_hdcp_disable(intel_connector);
> > > +	}
> > > +	drm_connector_list_iter_end(&conn_iter);
> > > +}
> > > +
> > > +static void i915_hdcp_component_master_unbind(struct device *dev)
> > > +{
> > > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > +
> > > +	intel_connectors_hdcp_disable(dev_priv);
> > Why this code? Once we've unregistered the driver, we should have shut
> > down the hardware completely. Which should have shut down all the hdcp
> > users too.
> 
> This unbind might be triggered either due to master_del or component_del.
> if its triggered from mei through component_del, then before teardown of
> the i/f in component_unbind(), disable the ongoing HDCP session through
> hdcp_disable() for each connectors.

I looked at your v7 component code again. I think if we put the
drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
of that. Essentially what you're doing here is open-coding part of that
function. Better to just move the existing call to the right place.

And shutting down the entire hw is kinda what master_unbind should be
doing I think. We might also need to move the general hw quiescent into
master_unbind, that should work too.

> 
> > 
> > > +	component_unbind_all(dev, dev_priv->hdcp_comp);
> > > +}
> > > +
> > > +static const struct component_master_ops i915_hdcp_component_master_ops = {
> > > +	.bind = i915_hdcp_component_master_bind,
> > > +	.unbind = i915_hdcp_component_master_unbind,
> > > +};
> > > +
> > > +static int i915_hdcp_component_match(struct device *dev, void *data)
> > > +{
> > > +	return !strcmp(dev->driver->name, "mei_hdcp");
> > > +}
> > > +
> > > +static int intel_hdcp_component_init(struct intel_connector *connector)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct i915_hdcp_component_master *comp;
> > > +	struct component_match *match = NULL;
> > > +	int ret;
> > > +
> > > +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
> > > +	if (!comp)
> > > +		return -ENOMEM;
> > > +
> > > +	mutex_init(&comp->mutex);
> > > +	dev_priv->hdcp_comp = comp;
> > > +
> > > +	component_match_add(dev_priv->drm.dev, &match,
> > > +			    i915_hdcp_component_match, dev_priv);
> > > +	ret = component_master_add_with_match(dev_priv->drm.dev,
> > > +					      &i915_hdcp_component_master_ops,
> > > +					      match);
> > So I'm not sure this will work out well, hiding the master_ops here in
> > hdcp. Definitely needs to be rewritten as soon as i915 needs another
> > component. And might make the load/unload split complicated.
> we have already discussed this.
> > 
> > > +	if (ret < 0)
> > > +		goto out_err;
> > > +
> > > +	DRM_INFO("I915 hdcp component master added.\n");
> > You add both the master and the hdcp component here. Output is a bit
> > confusing.
> 
> we have component master and a component from mei which will match to the
> master.
> 
> Here we are adding the component master.
> 
> > 
> > > +	return ret;
> > > +
> > > +out_err:
> > > +	component_master_del(dev_priv->drm.dev,
> > > +			     &i915_hdcp_component_master_ops);
> > > +	kfree(comp);
> > > +	dev_priv->hdcp_comp = NULL;
> > > +	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int initialize_mei_hdcp_data(struct intel_connector *connector)
> > > +{
> > > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > > +	struct mei_hdcp_data *data = &hdcp->mei_data;
> > > +	enum port port;
> > > +
> > > +	if (connector->encoder) {
> > > +		port = connector->encoder->port;
> > > +		data->port = GET_MEI_DDI_INDEX(port);
> > > +	}
> > > +
> > > +	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
> > > +	data->protocol = hdcp->shim->hdcp_protocol();
> > > +
> > > +	data->k = 1;
> > > +	if (!data->streams)
> > > +		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
> > > +					GFP_KERNEL);
> > > +	if (!data->streams) {
> > > +		DRM_ERROR("Out of Memory\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	data->streams[0].stream_id = 0;
> > > +	data->streams[0].stream_type = hdcp->content_type;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
> > >   {
> > >   	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
> > > @@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
> > >   {
> > >   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > >   	struct intel_hdcp *hdcp = &connector->hdcp;
> > > +	int ret;
> > >   	WARN_ON(!is_hdcp2_supported(dev_priv));
> > > -	/* TODO: MEI interface needs to be initialized here */
> > > +	ret = initialize_mei_hdcp_data(connector);
> > > +	if (ret) {
> > > +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
> > > +		return;
> > > +	}
> > > +
> > > +	if (!dev_priv->hdcp_comp)
> > > +		ret = intel_hdcp_component_init(connector);
> > > +
> > > +	if (ret) {
> > > +		DRM_DEBUG_KMS("HDCP comp init failed\n");
> > > +		kfree(hdcp->mei_data.streams);
> > > +		return;
> > > +	}
> > > +
> > >   	hdcp->hdcp2_supported = 1;
> > >   }
> > > @@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
> > >   		mutex_lock(&intel_connector->hdcp.mutex);
> > >   		intel_connector->hdcp.hdcp2_supported = 0;
> > >   		intel_connector->hdcp.shim = NULL;
> > > +		kfree(intel_connector->hdcp.mei_data.streams);
> > We need to push this into a per-connector hdcp cleanup function. Or just
> > into the generic connector cleanup.
> We could move it to per connector  hdcp cleanup function.

Yeah I think that would be cleaner.

> > 
> > >   		mutex_unlock(&intel_connector->hdcp.mutex);
> > >   	}
> > >   	drm_connector_list_iter_end(&conn_iter);
> > > +
> > > +	if (dev_priv->hdcp_comp) {
> > > +		component_master_del(dev_priv->drm.dev,
> > > +				     &i915_hdcp_component_master_ops);
> > > +		kfree(dev_priv->hdcp_comp);
> > > +		dev_priv->hdcp_comp = NULL;
> > > +	}
> > >   }
> > >   int intel_hdcp_enable(struct intel_connector *connector)
> > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > index fca22d463e1b..12268228f4dc 100644
> > > --- a/include/drm/i915_component.h
> > > +++ b/include/drm/i915_component.h
> > > @@ -24,8 +24,12 @@
> > >   #ifndef _I915_COMPONENT_H_
> > >   #define _I915_COMPONENT_H_
> > > +#include <linux/mutex.h>
> > > +
> > >   #include "drm_audio_component.h"
> > > +#include <drm/drm_hdcp.h>
> > > +
> > >   /* MAX_PORT is the number of port
> > >    * It must be sync with I915_MAX_PORTS defined i915_drv.h
> > >    */
> > > @@ -46,4 +50,71 @@ struct i915_audio_component {
> > >   	int aud_sample_rate[MAX_PORTS];
> > >   };
> > > +struct i915_hdcp_component_ops {
> > Imo that should be called mei_hdcp_component_ops and put into the
> > linux/mei_hdcp.h header. Or was that Thomas' review comment?
> 
> Nope this is there for many versions. i915_hdcp_component_ops are
> implemented by mei_hdcp.c and initializes the component with the &ops.

I summarized all the discussion around the i915/mei_hdcp interface in the
thread in an earlier patch in this series.

Cheers, Daniel

> 
> > 
> > Aside: Review here in public channels instead of in private would be much
> > better for coordination.
> 
> Tomas,
> 
> Could you please help on this ask.?
> 
> --Ram
> 
> > 
> > > +	/**
> > > +	 * @owner: mei_hdcp module
> > > +	 */
> > > +	struct module *owner;
> > > +
> > > +	int (*initiate_hdcp2_session)(struct device *dev,
> > > +				      void *hdcp_data,
> > > +				      struct hdcp2_ake_init *ake_data);
> > > +	int (*verify_receiver_cert_prepare_km)(struct device *dev,
> > > +					       void *hdcp_data,
> > > +					       struct hdcp2_ake_send_cert
> > > +								*rx_cert,
> > > +					       bool *km_stored,
> > > +					       struct hdcp2_ake_no_stored_km
> > > +								*ek_pub_km,
> > > +					       size_t *msg_sz);
> > > +	int (*verify_hprime)(struct device *dev,
> > > +			     void *hdcp_data,
> > > +			     struct hdcp2_ake_send_hprime *rx_hprime);
> > > +	int (*store_pairing_info)(struct device *dev,
> > > +				  void *hdcp_data,
> > > +				  struct hdcp2_ake_send_pairing_info
> > > +								*pairing_info);
> > > +	int (*initiate_locality_check)(struct device *dev,
> > > +				       void *hdcp_data,
> > > +				       struct hdcp2_lc_init *lc_init_data);
> > > +	int (*verify_lprime)(struct device *dev,
> > > +			     void *hdcp_data,
> > > +			     struct hdcp2_lc_send_lprime *rx_lprime);
> > > +	int (*get_session_key)(struct device *dev,
> > > +			       void *hdcp_data,
> > > +			       struct hdcp2_ske_send_eks *ske_data);
> > > +	int (*repeater_check_flow_prepare_ack)(struct device *dev,
> > > +					       void *hdcp_data,
> > > +					       struct hdcp2_rep_send_receiverid_list
> > > +								*rep_topology,
> > > +					       struct hdcp2_rep_send_ack
> > > +								*rep_send_ack);
> > > +	int (*verify_mprime)(struct device *dev,
> > > +			     void *hdcp_data,
> > > +			     struct hdcp2_rep_stream_ready *stream_ready);
> > > +	int (*enable_hdcp_authentication)(struct device *dev,
> > > +					  void *hdcp_data);
> > > +	int (*close_hdcp_session)(struct device *dev,
> > > +				  void *hdcp_data);
> > > +};
> > > +
> > > +/**
> > > + * struct i915_hdcp_component_master - Used for communication between i915
> > > + * and mei_hdcp for HDCP2.2 services.
> > > + */
> > > +struct i915_hdcp_component_master {
> > > +	/**
> > > +	 * @dev: a device providing hdcp
> > > +	 */
> > > +	struct device *dev;
> > > +	/**
> > > +	 * @mutex: Mutex to protect the state of dev
> > > +	 */
> > > +	struct mutex mutex;
> > > +	/**
> > > +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
> > > +	 */
> > > +	const struct i915_hdcp_component_ops *ops;
> > > +};
> > > +
> > >   #endif /* _I915_COMPONENT_H_ */
> > > -- 
> > > 2.7.4
> > >
On Fri, Dec 07, 2018 at 04:18:25PM +0530, C, Ramalingam wrote:
> 
> On 12/7/2018 11:22 AM, C, Ramalingam wrote:
> > 
> > 
> > On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > > Defining the mei-i915 interface functions and initialization of
> > > > the interface.
> > > > 
> > > > Signed-off-by: Ramalingam C<ramalingam.c@intel.com>
> > > > Signed-off-by: Tomas Winkler<tomas.winkler@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/i915_drv.h   |   2 +
> > > >   drivers/gpu/drm/i915/intel_drv.h  |   7 +
> > > >   drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
> > > >   include/drm/i915_component.h      |  71 ++++++
> > > >   4 files changed, 521 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index f763b30f98d9..b68bc980b7cd 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2015,6 +2015,8 @@ struct drm_i915_private {
> > > >   	struct i915_pmu pmu;
> > > > +	struct i915_hdcp_component_master *hdcp_comp;
> > > > +
> > > >   	/*
> > > >   	 * 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 85a526598096..bde82f3ada85 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -29,6 +29,7 @@
> > > >   #include <linux/i2c.h>
> > > >   #include <linux/hdmi.h>
> > > >   #include <linux/sched/clock.h>
> > > > +#include <linux/mei_hdcp.h>
> > > >   #include <drm/i915_drm.h>
> > > >   #include "i915_drv.h"
> > > >   #include <drm/drm_crtc.h>
> > > > @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
> > > >   	/* Detects panel's hdcp capability. This is optional for HDMI. */
> > > >   	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> > > >   			    bool *hdcp_capable);
> > > > +
> > > > +	/* Detects the HDCP protocol(DP/HDMI) required on the port */
> > > > +	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
> > > Looking ahead, this seems hardwired to constant return value? Or why do we
> > > need a function here?
> > This is hardwired based on the connector type(DP/HDMI).
> > Since we have the shim for hdcp's connector based work, I have added this function.
> > 
> > Could have done this just with connector_type check, but in that way whole hdcp_shim
> > can be done in that way. So going with the larger design here.
> > > >   };
> > > >   struct intel_hdcp {
> > > > @@ -399,6 +403,9 @@ struct intel_hdcp {
> > > >   	 * content can flow only through a link protected by HDCP2.2.
> > > >   	 */
> > > >   	u8 content_type;
> > > > +
> > > > +	/* mei interface related information */
> > > > +	struct mei_hdcp_data mei_data;
> > > >   };
> > > >   struct intel_connector {
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > index 99dddb540958..760780f1105c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > @@ -8,14 +8,20 @@
> > > >   #include <drm/drmP.h>
> > > >   #include <drm/drm_hdcp.h>
> > > > +#include <drm/i915_component.h>
> > > >   #include <linux/i2c.h>
> > > >   #include <linux/random.h>
> > > > +#include <linux/component.h>
> > > >   #include "intel_drv.h"
> > > >   #include "i915_reg.h"
> > > >   #define KEY_LOAD_TRIES	5
> > > >   #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
> > > > +#define GET_MEI_DDI_INDEX(p) ({                            \
> > > > +	typeof(p) __p = (p);                               \
> > > > +	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
> > > > +})
> > > >   static
> > > >   bool intel_hdcp_is_ksv_valid(u8 *ksv)
> > > > @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
> > > >   		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
> > > >   }
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_prepare_ake_init(struct intel_connector *connector,
> > > > +		       struct hdcp2_ake_init *ake_data)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
> > > > +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
> > > > +
> > > > +	/* Clear ME FW instance for the port, just incase */
> > > > +	comp->ops->close_hdcp_session(comp->dev, data);
> > > Sounds like a bug somewhere if we need this? I'd put this code into the
> > > ->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.
> > Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth along with ME FW.
> > Now if authentication failed due to some reason, then the HDCP2.2 season created with
> > ME FW for that port is not closed yet.
> > 
> > So we need to call close_hdcp_session() explicitly on authentication failures.
> > Session has to be closed before restarting the auth on that port with initialite_hdcp_session.
> > If we are closing the hdcp session of the port on all auth errors, we dont need this just before
> > start of the hdcp session.
> > > "Just in case" papering over programming bugs of our own just makes
> > > debugging harder.
> > > 
> > > > +
> > > > +	ret = comp->ops->initiate_hdcp2_session(comp->dev,
> > > > +						data, ake_data);
> > > We should set the port only after successfully initializing this.
> > hdcp2_session is created for each port at ME FW. Hence we need the port
> > initialized even before calling the initiate_hdcp2_session.
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
> > > > +				struct hdcp2_ake_send_cert *rx_cert,
> > > > +				bool *paired,
> > > > +				struct hdcp2_ake_no_stored_km *ek_pub_km,
> > > > +				size_t *msg_sz)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > These all look like programming mistakes that should result in a WARN_ON.
> > We are using the comp->mutex for protecting the interface during the interface
> > init, usage for mei communication and interface teardown.
> > 
> > But what if mei interface teardown happens between mei communications?
> > So when we try to access the mei interface after such possible tear down scenario,
> > we are checking the integrity of the interface.
> > 
> > Possible alternate solution is hold the comp->mutex across the authentication steps.
> > But consequence is that mei module removal will be blocked for authentication duration
> > and even if the mei_dev is removed, component unbind will be blocked due to this mutex dependency.
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
> > > > +							 rx_cert, paired,
> > > > +							 ek_pub_km, msg_sz);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > Error handling here seems a bit strange. You close the session, but don't
> > > reset the port. So next op will be totally unaware that things have blown
> > > up. Also no warning.
> > > 
> > > If we want to close the session, then I think that should be a decision
> > > made higher up.
> > This is needed as explained above. But as you have mentioned this can be moved
> > to the end of the authentication on error scenario. I will work on that.
> > > > +	mutex_unlock(&comp->mutex);
> > > With component do we still need this mutex stuff here?
> > > 
> > > Exact same comments everywhere below.
> > > 
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_verify_hprime(struct intel_connector *connector,
> > > > +		    struct hdcp2_ake_send_hprime *rx_hprime)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_store_pairing_info(struct intel_connector *connector,
> > > > +			 struct hdcp2_ake_send_pairing_info *pairing_info)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->store_pairing_info(comp->dev,
> > > > +					    data, pairing_info);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_prepare_lc_init(struct intel_connector *connector,
> > > > +		      struct hdcp2_lc_init *lc_init)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->initiate_locality_check(comp->dev,
> > > > +						 data, lc_init);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_verify_lprime(struct intel_connector *connector,
> > > > +		    struct hdcp2_lc_send_lprime *rx_lprime)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused))
> > > > +int hdcp2_prepare_skey(struct intel_connector *connector,
> > > > +		       struct hdcp2_ske_send_eks *ske_data)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
> > > > +				      struct hdcp2_rep_send_receiverid_list
> > > > +								*rep_topology,
> > > > +				      struct hdcp2_rep_send_ack *rep_send_ack)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
> > > > +							 data, rep_topology,
> > > > +							 rep_send_ack);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused)) int
> > > > +hdcp2_verify_mprime(struct intel_connector *connector,
> > > > +		    struct hdcp2_rep_stream_ready *stream_ready)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused))
> > > > +int hdcp2_authenticate_port(struct intel_connector *connector)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
> > > > +	if (ret < 0)
> > > > +		comp->ops->close_hdcp_session(comp->dev, data);
> > > > +	mutex_unlock(&comp->mutex);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused))
> > > > +int hdcp2_close_mei_session(struct intel_connector *connector)
> > > > +{
> > > > +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
> > > > +	int ret;
> > > > +
> > > > +	if (!comp)
> > > > +		return -EINVAL;
> > > > +
> > > > +	mutex_lock(&comp->mutex);
> > > > +	if (!comp->ops || !comp->dev ||
> > > > +	    data->port == MEI_DDI_INVALID_PORT) {
> > > > +		mutex_unlock(&comp->mutex);
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = comp->ops->close_hdcp_session(comp->dev, data);
> > > Need to reset the port here I think.
> > What is the reset of the port you are referring to? port is not
> > configured for encryption. And this is the call for marking the port as de-authenticated too.
> > > > +	mutex_unlock(&comp->mutex);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static __attribute__((unused))
> > > > +int hdcp2_deauthenticate_port(struct intel_connector *connector)
> > > > +{
> > > > +	return hdcp2_close_mei_session(connector);
> > > > +}
> > > > +
> > > > +static int i915_hdcp_component_master_bind(struct device *dev)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > > +
> > > > +	return component_bind_all(dev, dev_priv->hdcp_comp);
> > > > +}
> > > > +
> > > > +static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
> > > > +{
> > > > +	struct drm_device *dev = &dev_priv->drm;
> > > > +	struct intel_connector *intel_connector;
> > > > +	struct drm_connector *connector;
> > > > +	struct drm_connector_list_iter conn_iter;
> > > > +
> > > > +	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > +		intel_connector = to_intel_connector(connector);
> > > > +		if (!(intel_connector->hdcp.shim))
> > > > +			continue;
> > > > +
> > > > +		intel_hdcp_disable(intel_connector);
> > > > +	}
> > > > +	drm_connector_list_iter_end(&conn_iter);
> > > > +}
> > > > +
> > > > +static void i915_hdcp_component_master_unbind(struct device *dev)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > > +
> > > > +	intel_connectors_hdcp_disable(dev_priv);
> > > Why this code? Once we've unregistered the driver, we should have shut
> > > down the hardware completely. Which should have shut down all the hdcp
> > > users too.
> > This unbind might be triggered either due to master_del or component_del.
> > if its triggered from mei through component_del, then before teardown of
> > the i/f in component_unbind(), disable the ongoing HDCP session through
> > hdcp_disable() for each connectors.
> > > > +	component_unbind_all(dev, dev_priv->hdcp_comp);
> > > > +}
> > > > +
> > > > +static const struct component_master_ops i915_hdcp_component_master_ops = {
> > > > +	.bind = i915_hdcp_component_master_bind,
> > > > +	.unbind = i915_hdcp_component_master_unbind,
> > > > +};
> > > > +
> > > > +static int i915_hdcp_component_match(struct device *dev, void *data)
> > > > +{
> > > > +	return !strcmp(dev->driver->name, "mei_hdcp");
> > > > +}
> > > > +
> > > > +static int intel_hdcp_component_init(struct intel_connector *connector)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > +	struct i915_hdcp_component_master *comp;
> > > > +	struct component_match *match = NULL;
> > > > +	int ret;
> > > > +
> > > > +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
> > > > +	if (!comp)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	mutex_init(&comp->mutex);
> > > > +	dev_priv->hdcp_comp = comp;
> > > > +
> > > > +	component_match_add(dev_priv->drm.dev, &match,
> > > > +			    i915_hdcp_component_match, dev_priv);
> > > > +	ret = component_master_add_with_match(dev_priv->drm.dev,
> > > > +					      &i915_hdcp_component_master_ops,
> > > > +					      match);
> > > So I'm not sure this will work out well, hiding the master_ops here in
> > > hdcp. Definitely needs to be rewritten as soon as i915 needs another
> > > component. And might make the load/unload split complicated.
> > we have already discussed this.
> > > > +	if (ret < 0)
> > > > +		goto out_err;
> > > > +
> > > > +	DRM_INFO("I915 hdcp component master added.\n");
> > > You add both the master and the hdcp component here. Output is a bit
> > > confusing.
> > 
> > we have component master and a component from mei which will match to
> > the master.
> > 
> > Here we are adding the component master.
> > 
> > > > +	return ret;
> > > > +
> > > > +out_err:
> > > > +	component_master_del(dev_priv->drm.dev,
> > > > +			     &i915_hdcp_component_master_ops);
> > > > +	kfree(comp);
> > > > +	dev_priv->hdcp_comp = NULL;
> > > > +	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int initialize_mei_hdcp_data(struct intel_connector *connector)
> > > > +{
> > > > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > > > +	struct mei_hdcp_data *data = &hdcp->mei_data;
> > > > +	enum port port;
> > > > +
> > > > +	if (connector->encoder) {
> > > > +		port = connector->encoder->port;
> > > > +		data->port = GET_MEI_DDI_INDEX(port);
> > > > +	}
> > > > +
> > > > +	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
> > > > +	data->protocol = hdcp->shim->hdcp_protocol();
> > > > +
> > > > +	data->k = 1;
> > > > +	if (!data->streams)
> > > > +		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
> > > > +					GFP_KERNEL);
> > > > +	if (!data->streams) {
> > > > +		DRM_ERROR("Out of Memory\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	data->streams[0].stream_id = 0;
> > > > +	data->streams[0].stream_type = hdcp->content_type;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >   bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
> > > >   {
> > > >   	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
> > > > @@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
> > > >   {
> > > >   	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > >   	struct intel_hdcp *hdcp = &connector->hdcp;
> > > > +	int ret;
> > > >   	WARN_ON(!is_hdcp2_supported(dev_priv));
> > > > -	/* TODO: MEI interface needs to be initialized here */
> > > > +	ret = initialize_mei_hdcp_data(connector);
> > > > +	if (ret) {
> > > > +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	if (!dev_priv->hdcp_comp)
> > > > +		ret = intel_hdcp_component_init(connector);
> > > > +
> > > > +	if (ret) {
> > > > +		DRM_DEBUG_KMS("HDCP comp init failed\n");
> > > > +		kfree(hdcp->mei_data.streams);
> > > > +		return;
> > > > +	}
> > > > +
> > > >   	hdcp->hdcp2_supported = 1;
> > > >   }
> > > > @@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
> > > >   		mutex_lock(&intel_connector->hdcp.mutex);
> > > >   		intel_connector->hdcp.hdcp2_supported = 0;
> > > >   		intel_connector->hdcp.shim = NULL;
> > > > +		kfree(intel_connector->hdcp.mei_data.streams);
> > > We need to push this into a per-connector hdcp cleanup function. Or just
> > > into the generic connector cleanup.
> > We could move it to per connector  hdcp cleanup function.
> > > >   		mutex_unlock(&intel_connector->hdcp.mutex);
> > > >   	}
> > > >   	drm_connector_list_iter_end(&conn_iter);
> > > > +
> > > > +	if (dev_priv->hdcp_comp) {
> > > > +		component_master_del(dev_priv->drm.dev,
> > > > +				     &i915_hdcp_component_master_ops);
> > > > +		kfree(dev_priv->hdcp_comp);
> > > > +		dev_priv->hdcp_comp = NULL;
> > > > +	}
> > > >   }
> > > >   int intel_hdcp_enable(struct intel_connector *connector)
> > > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > > index fca22d463e1b..12268228f4dc 100644
> > > > --- a/include/drm/i915_component.h
> > > > +++ b/include/drm/i915_component.h
> > > > @@ -24,8 +24,12 @@
> > > >   #ifndef _I915_COMPONENT_H_
> > > >   #define _I915_COMPONENT_H_
> > > > +#include <linux/mutex.h>
> > > > +
> > > >   #include "drm_audio_component.h"
> > > > +#include <drm/drm_hdcp.h>
> > > > +
> > > >   /* MAX_PORT is the number of port
> > > >    * It must be sync with I915_MAX_PORTS defined i915_drv.h
> > > >    */
> > > > @@ -46,4 +50,71 @@ struct i915_audio_component {
> > > >   	int aud_sample_rate[MAX_PORTS];
> > > >   };
> > > > +struct i915_hdcp_component_ops {
> > > Imo that should be called mei_hdcp_component_ops and put into the
> > > linux/mei_hdcp.h header. Or was that Thomas' review comment?
> > Nope this is there for many versions. i915_hdcp_component_ops are
> > implemented by mei_hdcp.c and initializes the component with the &ops.
> 
> Tomas and Daniel,
> 
> Is this fine? Defining the ops at i915_component.h and I915 and mei_hdcp
> using it for their purpose?
> 
> If it has to be compulsorily defined by the service provider then we need
> to move this into the include/linux/mei_hdcp.h. In that we can avoid the
> global header from the mei_hdcp Tomas. Please help here to clarify the direction.

Let's move this discussion to the other reply chain, to avoid splitting
discussion up too much.
-Daniel

> 
> -Ram
> 
> > > Aside: Review here in public channels instead of in private would be much
> > > better for coordination.
> > Tomas,
> > Could you please help on this ask.?
> > --Ram
> > > > +	/**
> > > > +	 * @owner: mei_hdcp module
> > > > +	 */
> > > > +	struct module *owner;
> > > > +
> > > > +	int (*initiate_hdcp2_session)(struct device *dev,
> > > > +				      void *hdcp_data,
> > > > +				      struct hdcp2_ake_init *ake_data);
> > > > +	int (*verify_receiver_cert_prepare_km)(struct device *dev,
> > > > +					       void *hdcp_data,
> > > > +					       struct hdcp2_ake_send_cert
> > > > +								*rx_cert,
> > > > +					       bool *km_stored,
> > > > +					       struct hdcp2_ake_no_stored_km
> > > > +								*ek_pub_km,
> > > > +					       size_t *msg_sz);
> > > > +	int (*verify_hprime)(struct device *dev,
> > > > +			     void *hdcp_data,
> > > > +			     struct hdcp2_ake_send_hprime *rx_hprime);
> > > > +	int (*store_pairing_info)(struct device *dev,
> > > > +				  void *hdcp_data,
> > > > +				  struct hdcp2_ake_send_pairing_info
> > > > +								*pairing_info);
> > > > +	int (*initiate_locality_check)(struct device *dev,
> > > > +				       void *hdcp_data,
> > > > +				       struct hdcp2_lc_init *lc_init_data);
> > > > +	int (*verify_lprime)(struct device *dev,
> > > > +			     void *hdcp_data,
> > > > +			     struct hdcp2_lc_send_lprime *rx_lprime);
> > > > +	int (*get_session_key)(struct device *dev,
> > > > +			       void *hdcp_data,
> > > > +			       struct hdcp2_ske_send_eks *ske_data);
> > > > +	int (*repeater_check_flow_prepare_ack)(struct device *dev,
> > > > +					       void *hdcp_data,
> > > > +					       struct hdcp2_rep_send_receiverid_list
> > > > +								*rep_topology,
> > > > +					       struct hdcp2_rep_send_ack
> > > > +								*rep_send_ack);
> > > > +	int (*verify_mprime)(struct device *dev,
> > > > +			     void *hdcp_data,
> > > > +			     struct hdcp2_rep_stream_ready *stream_ready);
> > > > +	int (*enable_hdcp_authentication)(struct device *dev,
> > > > +					  void *hdcp_data);
> > > > +	int (*close_hdcp_session)(struct device *dev,
> > > > +				  void *hdcp_data);
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct i915_hdcp_component_master - Used for communication between i915
> > > > + * and mei_hdcp for HDCP2.2 services.
> > > > + */
> > > > +struct i915_hdcp_component_master {
> > > > +	/**
> > > > +	 * @dev: a device providing hdcp
> > > > +	 */
> > > > +	struct device *dev;
> > > > +	/**
> > > > +	 * @mutex: Mutex to protect the state of dev
> > > > +	 */
> > > > +	struct mutex mutex;
> > > > +	/**
> > > > +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
> > > > +	 */
> > > > +	const struct i915_hdcp_component_ops *ops;
> > > > +};
> > > > +
> > > >   #endif /* _I915_COMPONENT_H_ */
> > > > -- 
> > > > 2.7.4
> > > > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/7/2018 7:59 PM, Daniel Vetter wrote:
> On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
>> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
>>> On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
>>>> Defining the mei-i915 interface functions and initialization of
>>>> the interface.
>>>>
>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h   |   2 +
>>>>    drivers/gpu/drm/i915/intel_drv.h  |   7 +
>>>>    drivers/gpu/drm/i915/intel_hdcp.c | 442 +++++++++++++++++++++++++++++++++++++-
>>>>    include/drm/i915_component.h      |  71 ++++++
>>>>    4 files changed, 521 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index f763b30f98d9..b68bc980b7cd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -2015,6 +2015,8 @@ struct drm_i915_private {
>>>>    	struct i915_pmu pmu;
>>>> +	struct i915_hdcp_component_master *hdcp_comp;
>>>> +
>>>>    	/*
>>>>    	 * 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 85a526598096..bde82f3ada85 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -29,6 +29,7 @@
>>>>    #include <linux/i2c.h>
>>>>    #include <linux/hdmi.h>
>>>>    #include <linux/sched/clock.h>
>>>> +#include <linux/mei_hdcp.h>
>>>>    #include <drm/i915_drm.h>
>>>>    #include "i915_drv.h"
>>>>    #include <drm/drm_crtc.h>
>>>> @@ -379,6 +380,9 @@ struct intel_hdcp_shim {
>>>>    	/* Detects panel's hdcp capability. This is optional for HDMI. */
>>>>    	int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
>>>>    			    bool *hdcp_capable);
>>>> +
>>>> +	/* Detects the HDCP protocol(DP/HDMI) required on the port */
>>>> +	enum mei_hdcp_wired_protocol (*hdcp_protocol)(void);
>>> Looking ahead, this seems hardwired to constant return value? Or why do we
>>> need a function here?
>> This is hardwired based on the connector type(DP/HDMI).
>> Since we have the shim for hdcp's connector based work, I have added this function.
>>
>> Could have done this just with connector_type check, but in that way whole hdcp_shim
>> can be done in that way. So going with the larger design here.
> If it's hardwired then just make it a hardwired struct member. As long as
> it's all const, we can mix data an function pointers. If you have runtime
> variable data, then it's better to split it out from the ops structure, so
> that we can keep ops read-only and protected against possible exploits
> (any function pointers are a high-value target in the kernel).
Sure. Done.
>>>>    };
>>>>    struct intel_hdcp {
>>>> @@ -399,6 +403,9 @@ struct intel_hdcp {
>>>>    	 * content can flow only through a link protected by HDCP2.2.
>>>>    	 */
>>>>    	u8 content_type;
>>>> +
>>>> +	/* mei interface related information */
>>>> +	struct mei_hdcp_data mei_data;
>>>>    };
>>>>    struct intel_connector {
>>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>>>> index 99dddb540958..760780f1105c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>>>> @@ -8,14 +8,20 @@
>>>>    #include <drm/drmP.h>
>>>>    #include <drm/drm_hdcp.h>
>>>> +#include <drm/i915_component.h>
>>>>    #include <linux/i2c.h>
>>>>    #include <linux/random.h>
>>>> +#include <linux/component.h>
>>>>    #include "intel_drv.h"
>>>>    #include "i915_reg.h"
>>>>    #define KEY_LOAD_TRIES	5
>>>>    #define TIME_FOR_ENCRYPT_STATUS_CHANGE	50
>>>> +#define GET_MEI_DDI_INDEX(p) ({                            \
>>>> +	typeof(p) __p = (p);                               \
>>>> +	__p == PORT_A ? MEI_DDI_A : (enum mei_hdcp_ddi)__p;\
>>>> +})
>>>>    static
>>>>    bool intel_hdcp_is_ksv_valid(u8 *ksv)
>>>> @@ -833,6 +839,417 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
>>>>    		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
>>>>    }
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_prepare_ake_init(struct intel_connector *connector,
>>>> +		       struct hdcp2_ake_init *ake_data)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
>>>> +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
>>>> +
>>>> +	/* Clear ME FW instance for the port, just incase */
>>>> +	comp->ops->close_hdcp_session(comp->dev, data);
>>> Sounds like a bug somewhere if we need this? I'd put this code into the
>>> ->initiate_hdcp2_session, with a big WARN_ON if it's actually needed.
>> Not really. Lets say you have progressed beyond the first step of HDCP2.2 auth along with ME FW.
>> Now if authentication failed due to some reason, then the HDCP2.2 season created with
>> ME FW for that port is not closed yet.
>>
>> So we need to call close_hdcp_session() explicitly on authentication failures.
>> Session has to be closed before restarting the auth on that port with initialite_hdcp_session.
>> If we are closing the hdcp session of the port on all auth errors, we dont need this just before
>> start of the hdcp session.
> Yeah, makes sense. But right now the close_session stuff is sprinkled all
> over, so it's hard to see what's going on and whether there's bugs
> somewhere. I much prefer code that looks like it knows what it's doing. I
> think one top-level close_session call in the main hdcp2 functions (called
> when everything is done, or on failure) would be much cleaner.
Ok. Sure.
>
> Plus then some WARN_ON to make sure we never forget to close a session.
> Sprinkling cleanup code all over the place looks like it's easier, but
> long term it's much harder code to maintain and refactor because you never
> sure which cleanup code is actually doing the cleanup, and which cleanup
> code is just there for nothing.
>>> "Just in case" papering over programming bugs of our own just makes
>>> debugging harder.
>>>
>>>> +
>>>> +	ret = comp->ops->initiate_hdcp2_session(comp->dev,
>>>> +						data, ake_data);
>>> We should set the port only after successfully initializing this.
>> hdcp2_session is created for each port at ME FW. Hence we need the port
>> initialized even before calling the initiate_hdcp2_session.
>>>> +	if (data->port == MEI_DDI_INVALID_PORT && connector->encoder)
>>>> +		data->port = GET_MEI_DDI_INDEX(connector->encoder->port);
> This line right above in your patch should only run on success. I think.
>
> Also, looking at this again: Why could connector->encoder ever be NULL?
> That smells like another possible bug in our setup code. Better to wrap
> this in a WARN_ON and bail out.
>
>
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,
>>>> +				struct hdcp2_ake_send_cert *rx_cert,
>>>> +				bool *paired,
>>>> +				struct hdcp2_ake_no_stored_km *ek_pub_km,
>>>> +				size_t *msg_sz)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>> These all look like programming mistakes that should result in a WARN_ON.
>> We are using the comp->mutex for protecting the interface during the interface
>> init, usage for mei communication and interface teardown.
>>
>> But what if mei interface teardown happens between mei communications?
>> So when we try to access the mei interface after such possible tear down scenario,
>> we are checking the integrity of the interface.
>>
>> Possible alternate solution is hold the comp->mutex across the authentication steps.
>> But consequence is that mei module removal will be blocked for authentication duration
>> and even if the mei_dev is removed, component unbind will be blocked due to this mutex dependency.
> I think with the v7 component code this should all be impossible and we
> can remove it. With the v8 component code it's definitely necessary (which
> is why I don't like the v8 component code). This is all because I didn't
> fully realize the changes with v8 when reading the patches again ...
Got addressed as soon as we move to version implemented in v7.
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->verify_receiver_cert_prepare_km(comp->dev, data,
>>>> +							 rx_cert, paired,
>>>> +							 ek_pub_km, msg_sz);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>> Error handling here seems a bit strange. You close the session, but don't
>>> reset the port. So next op will be totally unaware that things have blown
>>> up. Also no warning.
>>>
>>> If we want to close the session, then I think that should be a decision
>>> made higher up.
>> This is needed as explained above. But as you have mentioned this can be moved
>> to the end of the authentication on error scenario. I will work on that.
> Yeah, I think that'll lead to cleaner code.
>
>>>> +	mutex_unlock(&comp->mutex);
>>> With component do we still need this mutex stuff here?
>>>
>>> Exact same comments everywhere below.
>>>
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_verify_hprime(struct intel_connector *connector,
>>>> +		    struct hdcp2_ake_send_hprime *rx_hprime)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->verify_hprime(comp->dev, data, rx_hprime);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_store_pairing_info(struct intel_connector *connector,
>>>> +			 struct hdcp2_ake_send_pairing_info *pairing_info)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->store_pairing_info(comp->dev,
>>>> +					    data, pairing_info);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_prepare_lc_init(struct intel_connector *connector,
>>>> +		      struct hdcp2_lc_init *lc_init)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->initiate_locality_check(comp->dev,
>>>> +						 data, lc_init);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_verify_lprime(struct intel_connector *connector,
>>>> +		    struct hdcp2_lc_send_lprime *rx_lprime)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->verify_lprime(comp->dev, data, rx_lprime);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused))
>>>> +int hdcp2_prepare_skey(struct intel_connector *connector,
>>>> +		       struct hdcp2_ske_send_eks *ske_data)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->get_session_key(comp->dev, data, ske_data);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,
>>>> +				      struct hdcp2_rep_send_receiverid_list
>>>> +								*rep_topology,
>>>> +				      struct hdcp2_rep_send_ack *rep_send_ack)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->repeater_check_flow_prepare_ack(comp->dev,
>>>> +							 data, rep_topology,
>>>> +							 rep_send_ack);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused)) int
>>>> +hdcp2_verify_mprime(struct intel_connector *connector,
>>>> +		    struct hdcp2_rep_stream_ready *stream_ready)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->verify_mprime(comp->dev, data, stream_ready);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused))
>>>> +int hdcp2_authenticate_port(struct intel_connector *connector)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev || data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->enable_hdcp_authentication(comp->dev, data);
>>>> +	if (ret < 0)
>>>> +		comp->ops->close_hdcp_session(comp->dev, data);
>>>> +	mutex_unlock(&comp->mutex);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused))
>>>> +int hdcp2_close_mei_session(struct intel_connector *connector)
>>>> +{
>>>> +	struct mei_hdcp_data *data = &connector->hdcp.mei_data;
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp = dev_priv->hdcp_comp;
>>>> +	int ret;
>>>> +
>>>> +	if (!comp)
>>>> +		return -EINVAL;
>>>> +
>>>> +	mutex_lock(&comp->mutex);
>>>> +	if (!comp->ops || !comp->dev ||
>>>> +	    data->port == MEI_DDI_INVALID_PORT) {
>>>> +		mutex_unlock(&comp->mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	ret = comp->ops->close_hdcp_session(comp->dev, data);
>>> Need to reset the port here I think.
>> What is the reset of the port you are referring to? port is not
>> configured for encryption. And this is the call for marking the port as de-authenticated too.
>>
>>>> +	mutex_unlock(&comp->mutex);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static __attribute__((unused))
>>>> +int hdcp2_deauthenticate_port(struct intel_connector *connector)
>>>> +{
>>>> +	return hdcp2_close_mei_session(connector);
>>>> +}
>>>> +
>>>> +static int i915_hdcp_component_master_bind(struct device *dev)
>>>> +{
>>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>>> +
>>>> +	return component_bind_all(dev, dev_priv->hdcp_comp);
>>>> +}
>>>> +
>>>> +static void intel_connectors_hdcp_disable(struct drm_i915_private *dev_priv)
>>>> +{
>>>> +	struct drm_device *dev = &dev_priv->drm;
>>>> +	struct intel_connector *intel_connector;
>>>> +	struct drm_connector *connector;
>>>> +	struct drm_connector_list_iter conn_iter;
>>>> +
>>>> +	drm_connector_list_iter_begin(dev, &conn_iter);
>>>> +	drm_for_each_connector_iter(connector, &conn_iter) {
>>>> +		intel_connector = to_intel_connector(connector);
>>>> +		if (!(intel_connector->hdcp.shim))
>>>> +			continue;
>>>> +
>>>> +		intel_hdcp_disable(intel_connector);
>>>> +	}
>>>> +	drm_connector_list_iter_end(&conn_iter);
>>>> +}
>>>> +
>>>> +static void i915_hdcp_component_master_unbind(struct device *dev)
>>>> +{
>>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>>> +
>>>> +	intel_connectors_hdcp_disable(dev_priv);
>>> Why this code? Once we've unregistered the driver, we should have shut
>>> down the hardware completely. Which should have shut down all the hdcp
>>> users too.
>> This unbind might be triggered either due to master_del or component_del.
>> if its triggered from mei through component_del, then before teardown of
>> the i/f in component_unbind(), disable the ongoing HDCP session through
>> hdcp_disable() for each connectors.
> I looked at your v7 component code again. I think if we put the
> drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
> of that. Essentially what you're doing here is open-coding part of that
> function. Better to just move the existing call to the right place.
>
> And shutting down the entire hw is kinda what master_unbind should be
> doing I think. We might also need to move the general hw quiescent into
> master_unbind, that should work too.

Need some more information on this. As per v7 on master_unbind will invoke
i915_unload_head that is i915_driver_unregister(dev_priv);
Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv);

We are not initializing/shutting the HW in master_bind/unbind .
But this comment is contradicting with current approach. Could you please elaborate.?

-Ram

>>>> +	component_unbind_all(dev, dev_priv->hdcp_comp);
>>>> +}
>>>> +
>>>> +static const struct component_master_ops i915_hdcp_component_master_ops = {
>>>> +	.bind = i915_hdcp_component_master_bind,
>>>> +	.unbind = i915_hdcp_component_master_unbind,
>>>> +};
>>>> +
>>>> +static int i915_hdcp_component_match(struct device *dev, void *data)
>>>> +{
>>>> +	return !strcmp(dev->driver->name, "mei_hdcp");
>>>> +}
>>>> +
>>>> +static int intel_hdcp_component_init(struct intel_connector *connector)
>>>> +{
>>>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>> +	struct i915_hdcp_component_master *comp;
>>>> +	struct component_match *match = NULL;
>>>> +	int ret;
>>>> +
>>>> +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
>>>> +	if (!comp)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	mutex_init(&comp->mutex);
>>>> +	dev_priv->hdcp_comp = comp;
>>>> +
>>>> +	component_match_add(dev_priv->drm.dev, &match,
>>>> +			    i915_hdcp_component_match, dev_priv);
>>>> +	ret = component_master_add_with_match(dev_priv->drm.dev,
>>>> +					      &i915_hdcp_component_master_ops,
>>>> +					      match);
>>> So I'm not sure this will work out well, hiding the master_ops here in
>>> hdcp. Definitely needs to be rewritten as soon as i915 needs another
>>> component. And might make the load/unload split complicated.
>> we have already discussed this.
>>>> +	if (ret < 0)
>>>> +		goto out_err;
>>>> +
>>>> +	DRM_INFO("I915 hdcp component master added.\n");
>>> You add both the master and the hdcp component here. Output is a bit
>>> confusing.
>> we have component master and a component from mei which will match to the
>> master.
>>
>> Here we are adding the component master.
>>
>>>> +	return ret;
>>>> +
>>>> +out_err:
>>>> +	component_master_del(dev_priv->drm.dev,
>>>> +			     &i915_hdcp_component_master_ops);
>>>> +	kfree(comp);
>>>> +	dev_priv->hdcp_comp = NULL;
>>>> +	DRM_ERROR("Failed to add i915 hdcp component master (%d)\n", ret);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int initialize_mei_hdcp_data(struct intel_connector *connector)
>>>> +{
>>>> +	struct intel_hdcp *hdcp = &connector->hdcp;
>>>> +	struct mei_hdcp_data *data = &hdcp->mei_data;
>>>> +	enum port port;
>>>> +
>>>> +	if (connector->encoder) {
>>>> +		port = connector->encoder->port;
>>>> +		data->port = GET_MEI_DDI_INDEX(port);
>>>> +	}
>>>> +
>>>> +	data->port_type = MEI_HDCP_PORT_TYPE_INTEGRATED;
>>>> +	data->protocol = hdcp->shim->hdcp_protocol();
>>>> +
>>>> +	data->k = 1;
>>>> +	if (!data->streams)
>>>> +		data->streams = kcalloc(data->k, sizeof(data->streams[0]),
>>>> +					GFP_KERNEL);
>>>> +	if (!data->streams) {
>>>> +		DRM_ERROR("Out of Memory\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	data->streams[0].stream_id = 0;
>>>> +	data->streams[0].stream_type = hdcp->content_type;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
>>>>    {
>>>>    	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
>>>> @@ -843,10 +1260,25 @@ static void intel_hdcp2_init(struct intel_connector *connector)
>>>>    {
>>>>    	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>>>>    	struct intel_hdcp *hdcp = &connector->hdcp;
>>>> +	int ret;
>>>>    	WARN_ON(!is_hdcp2_supported(dev_priv));
>>>> -	/* TODO: MEI interface needs to be initialized here */
>>>> +	ret = initialize_mei_hdcp_data(connector);
>>>> +	if (ret) {
>>>> +		DRM_DEBUG_KMS("Mei hdcp data init failed\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (!dev_priv->hdcp_comp)
>>>> +		ret = intel_hdcp_component_init(connector);
>>>> +
>>>> +	if (ret) {
>>>> +		DRM_DEBUG_KMS("HDCP comp init failed\n");
>>>> +		kfree(hdcp->mei_data.streams);
>>>> +		return;
>>>> +	}
>>>> +
>>>>    	hdcp->hdcp2_supported = 1;
>>>>    }
>>>> @@ -894,9 +1326,17 @@ void intel_hdcp_exit(struct drm_i915_private *dev_priv)
>>>>    		mutex_lock(&intel_connector->hdcp.mutex);
>>>>    		intel_connector->hdcp.hdcp2_supported = 0;
>>>>    		intel_connector->hdcp.shim = NULL;
>>>> +		kfree(intel_connector->hdcp.mei_data.streams);
>>> We need to push this into a per-connector hdcp cleanup function. Or just
>>> into the generic connector cleanup.
>> We could move it to per connector  hdcp cleanup function.
> Yeah I think that would be cleaner.
>
>>>>    		mutex_unlock(&intel_connector->hdcp.mutex);
>>>>    	}
>>>>    	drm_connector_list_iter_end(&conn_iter);
>>>> +
>>>> +	if (dev_priv->hdcp_comp) {
>>>> +		component_master_del(dev_priv->drm.dev,
>>>> +				     &i915_hdcp_component_master_ops);
>>>> +		kfree(dev_priv->hdcp_comp);
>>>> +		dev_priv->hdcp_comp = NULL;
>>>> +	}
>>>>    }
>>>>    int intel_hdcp_enable(struct intel_connector *connector)
>>>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>>>> index fca22d463e1b..12268228f4dc 100644
>>>> --- a/include/drm/i915_component.h
>>>> +++ b/include/drm/i915_component.h
>>>> @@ -24,8 +24,12 @@
>>>>    #ifndef _I915_COMPONENT_H_
>>>>    #define _I915_COMPONENT_H_
>>>> +#include <linux/mutex.h>
>>>> +
>>>>    #include "drm_audio_component.h"
>>>> +#include <drm/drm_hdcp.h>
>>>> +
>>>>    /* MAX_PORT is the number of port
>>>>     * It must be sync with I915_MAX_PORTS defined i915_drv.h
>>>>     */
>>>> @@ -46,4 +50,71 @@ struct i915_audio_component {
>>>>    	int aud_sample_rate[MAX_PORTS];
>>>>    };
>>>> +struct i915_hdcp_component_ops {
>>> Imo that should be called mei_hdcp_component_ops and put into the
>>> linux/mei_hdcp.h header. Or was that Thomas' review comment?
>> Nope this is there for many versions. i915_hdcp_component_ops are
>> implemented by mei_hdcp.c and initializes the component with the &ops.
> I summarized all the discussion around the i915/mei_hdcp interface in the
> thread in an earlier patch in this series.
>
> Cheers, Daniel
>
>>> Aside: Review here in public channels instead of in private would be much
>>> better for coordination.
>> Tomas,
>>
>> Could you please help on this ask.?
>>
>> --Ram
>>
>>>> +	/**
>>>> +	 * @owner: mei_hdcp module
>>>> +	 */
>>>> +	struct module *owner;
>>>> +
>>>> +	int (*initiate_hdcp2_session)(struct device *dev,
>>>> +				      void *hdcp_data,
>>>> +				      struct hdcp2_ake_init *ake_data);
>>>> +	int (*verify_receiver_cert_prepare_km)(struct device *dev,
>>>> +					       void *hdcp_data,
>>>> +					       struct hdcp2_ake_send_cert
>>>> +								*rx_cert,
>>>> +					       bool *km_stored,
>>>> +					       struct hdcp2_ake_no_stored_km
>>>> +								*ek_pub_km,
>>>> +					       size_t *msg_sz);
>>>> +	int (*verify_hprime)(struct device *dev,
>>>> +			     void *hdcp_data,
>>>> +			     struct hdcp2_ake_send_hprime *rx_hprime);
>>>> +	int (*store_pairing_info)(struct device *dev,
>>>> +				  void *hdcp_data,
>>>> +				  struct hdcp2_ake_send_pairing_info
>>>> +								*pairing_info);
>>>> +	int (*initiate_locality_check)(struct device *dev,
>>>> +				       void *hdcp_data,
>>>> +				       struct hdcp2_lc_init *lc_init_data);
>>>> +	int (*verify_lprime)(struct device *dev,
>>>> +			     void *hdcp_data,
>>>> +			     struct hdcp2_lc_send_lprime *rx_lprime);
>>>> +	int (*get_session_key)(struct device *dev,
>>>> +			       void *hdcp_data,
>>>> +			       struct hdcp2_ske_send_eks *ske_data);
>>>> +	int (*repeater_check_flow_prepare_ack)(struct device *dev,
>>>> +					       void *hdcp_data,
>>>> +					       struct hdcp2_rep_send_receiverid_list
>>>> +								*rep_topology,
>>>> +					       struct hdcp2_rep_send_ack
>>>> +								*rep_send_ack);
>>>> +	int (*verify_mprime)(struct device *dev,
>>>> +			     void *hdcp_data,
>>>> +			     struct hdcp2_rep_stream_ready *stream_ready);
>>>> +	int (*enable_hdcp_authentication)(struct device *dev,
>>>> +					  void *hdcp_data);
>>>> +	int (*close_hdcp_session)(struct device *dev,
>>>> +				  void *hdcp_data);
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct i915_hdcp_component_master - Used for communication between i915
>>>> + * and mei_hdcp for HDCP2.2 services.
>>>> + */
>>>> +struct i915_hdcp_component_master {
>>>> +	/**
>>>> +	 * @dev: a device providing hdcp
>>>> +	 */
>>>> +	struct device *dev;
>>>> +	/**
>>>> +	 * @mutex: Mutex to protect the state of dev
>>>> +	 */
>>>> +	struct mutex mutex;
>>>> +	/**
>>>> +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
>>>> +	 */
>>>> +	const struct i915_hdcp_component_ops *ops;
>>>> +};
>>>> +
>>>>    #endif /* _I915_COMPONENT_H_ */
>>>> -- 
>>>> 2.7.4
>>>>
On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:
> On 12/7/2018 7:59 PM, Daniel Vetter wrote:
> > On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
> > > On 12/6/2018 3:53 PM, Daniel Vetter wrote:
> > > > On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
> > > > > +static void i915_hdcp_component_master_unbind(struct device *dev)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > > > +
> > > > > +	intel_connectors_hdcp_disable(dev_priv);
> > > > Why this code? Once we've unregistered the driver, we should have shut
> > > > down the hardware completely. Which should have shut down all the hdcp
> > > > users too.
> > > This unbind might be triggered either due to master_del or component_del.
> > > if its triggered from mei through component_del, then before teardown of
> > > the i/f in component_unbind(), disable the ongoing HDCP session through
> > > hdcp_disable() for each connectors.
> > I looked at your v7 component code again. I think if we put the
> > drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
> > of that. Essentially what you're doing here is open-coding part of that
> > function. Better to just move the existing call to the right place.
> > 
> > And shutting down the entire hw is kinda what master_unbind should be
> > doing I think. We might also need to move the general hw quiescent into
> > master_unbind, that should work too.
> 
> Need some more information on this. As per v7 on master_unbind will invoke
> i915_unload_head that is i915_driver_unregister(dev_priv);
> Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv);
> 
> We are not initializing/shutting the HW in master_bind/unbind .
> But this comment is contradicting with current approach. Could you please elaborate.?

So my understanding is that we need to shut down all hdcp usage before
master_unbind completes, because otherwise we'll blow up: The mei_hdcp
component is gone already.

Now the other case for master_unbind is that the i915 pci device is
unloaded, and in that case again I think it makes sense to shut down the
hardware in master_unbind.

Now when I looked at v7, right after the component_unbind code in the i915
unload sequences comes the function calls to shut down the hardware. I
think it would make sense to them (or at least the
drm_atomic_helper_shutdown() call) into master_unbind.

This is somewhat asymetric, but that's ok: Load path doesn't enable
anything, we just keep the hardware as-is (to be able to support
flicker-free boôt-up). First modest is done by usersapce. Exception is if
you have fbcon enabled (and not use the fastboot patches that Hans just
merged), in that case the kernel will do the first modeset when we
regiseter the fbdev device.

Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
function in v7 should take care of disabling all outputs, and hence also
disabling all usage of hdcp component.
-Daniel
On 12/12/2018 4:08 PM, Daniel Vetter wrote:
> On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:
>> On 12/7/2018 7:59 PM, Daniel Vetter wrote:
>>> On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
>>>> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
>>>>> On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
>>>>>> +static void i915_hdcp_component_master_unbind(struct device *dev)
>>>>>> +{
>>>>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>>>>> +
>>>>>> +	intel_connectors_hdcp_disable(dev_priv);
>>>>> Why this code? Once we've unregistered the driver, we should have shut
>>>>> down the hardware completely. Which should have shut down all the hdcp
>>>>> users too.
>>>> This unbind might be triggered either due to master_del or component_del.
>>>> if its triggered from mei through component_del, then before teardown of
>>>> the i/f in component_unbind(), disable the ongoing HDCP session through
>>>> hdcp_disable() for each connectors.
>>> I looked at your v7 component code again. I think if we put the
>>> drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
>>> of that. Essentially what you're doing here is open-coding part of that
>>> function. Better to just move the existing call to the right place.
>>>
>>> And shutting down the entire hw is kinda what master_unbind should be
>>> doing I think. We might also need to move the general hw quiescent into
>>> master_unbind, that should work too.
>> Need some more information on this. As per v7 on master_unbind will invoke
>> i915_unload_head that is i915_driver_unregister(dev_priv);
>> Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv);
>>
>> We are not initializing/shutting the HW in master_bind/unbind .
>> But this comment is contradicting with current approach. Could you please elaborate.?
> So my understanding is that we need to shut down all hdcp usage before
> master_unbind completes, because otherwise we'll blow up: The mei_hdcp
> component is gone already.
>
> Now the other case for master_unbind is that the i915 pci device is
> unloaded, and in that case again I think it makes sense to shut down the
> hardware in master_unbind.
>
> Now when I looked at v7, right after the component_unbind code in the i915
> unload sequences comes the function calls to shut down the hardware. I
> think it would make sense to them (or at least the
> drm_atomic_helper_shutdown() call) into master_unbind.
>
> This is somewhat asymetric, but that's ok: Load path doesn't enable
> anything, we just keep the hardware as-is (to be able to support
> flicker-free boôt-up). First modest is done by usersapce. Exception is if
> you have fbcon enabled (and not use the fastboot patches that Hans just
> merged), in that case the kernel will do the first modeset when we
> regiseter the fbdev device.
>
> Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
> function in v7 should take care of disabling all outputs, and hence also
> disabling all usage of hdcp component.

But in that case master_bind should do the reverse of the drm_atomic_helper_shutdown()right?
else lets say mei_hdcp is removed, master_unbind triggered and hence all hw is shutdown.
And then mei_hdcp is loaded so master_bind should initialize the hw right? Which is not the scenario right now.

-Ram

> -Daniel
On 12/12/2018 4:34 PM, C, Ramalingam wrote:
>
>
> On 12/12/2018 4:08 PM, Daniel Vetter wrote:
>> On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:
>>> On 12/7/2018 7:59 PM, Daniel Vetter wrote:
>>>> On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:
>>>>> On 12/6/2018 3:53 PM, Daniel Vetter wrote:
>>>>>> On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:
>>>>>>> +static void i915_hdcp_component_master_unbind(struct device *dev)
>>>>>>> +{
>>>>>>> +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>>>>>>> +
>>>>>>> +	intel_connectors_hdcp_disable(dev_priv);
>>>>>> Why this code? Once we've unregistered the driver, we should have shut
>>>>>> down the hardware completely. Which should have shut down all the hdcp
>>>>>> users too.
>>>>> This unbind might be triggered either due to master_del or component_del.
>>>>> if its triggered from mei through component_del, then before teardown of
>>>>> the i/f in component_unbind(), disable the ongoing HDCP session through
>>>>> hdcp_disable() for each connectors.
>>>> I looked at your v7 component code again. I think if we put the
>>>> drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
>>>> of that. Essentially what you're doing here is open-coding part of that
>>>> function. Better to just move the existing call to the right place.
>>>>
>>>> And shutting down the entire hw is kinda what master_unbind should be
>>>> doing I think. We might also need to move the general hw quiescent into
>>>> master_unbind, that should work too.
>>> Need some more information on this. As per v7 on master_unbind will invoke
>>> i915_unload_head that is i915_driver_unregister(dev_priv);
>>> Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv);
>>>
>>> We are not initializing/shutting the HW in master_bind/unbind .
>>> But this comment is contradicting with current approach. Could you please elaborate.?
>> So my understanding is that we need to shut down all hdcp usage before
>> master_unbind completes, because otherwise we'll blow up: The mei_hdcp
>> component is gone already.
>>
>> Now the other case for master_unbind is that the i915 pci device is
>> unloaded, and in that case again I think it makes sense to shut down the
>> hardware in master_unbind.
>>
>> Now when I looked at v7, right after the component_unbind code in the i915
>> unload sequences comes the function calls to shut down the hardware. I
>> think it would make sense to them (or at least the
>> drm_atomic_helper_shutdown() call) into master_unbind.
>>
>> This is somewhat asymetric, but that's ok: Load path doesn't enable
>> anything, we just keep the hardware as-is (to be able to support
>> flicker-free boôt-up). First modest is done by usersapce. Exception is if
>> you have fbcon enabled (and not use the fastboot patches that Hans just
>> merged), in that case the kernel will do the first modeset when we
>> regiseter the fbdev device.
>>
>> Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
>> function in v7 should take care of disabling all outputs, and hence also
>> disabling all usage of hdcp component.
> But in that case master_bind should do the reverse of the drm_atomic_helper_shutdown()right?
> else lets say mei_hdcp is removed, master_unbind triggered and hence all hw is shutdown.
> And then mei_hdcp is loaded so master_bind should initialize the hw right? Which is not the scenario right now.

Summarizing the #intel-gfx IRC discussion:

As i915 load and unload  are already asymetric, there is no harm in moving
the drm_atomic_helper_shutdown() into the master_unbind(). So going
ahead with daniel's suggestion.

-Ram

>
> -Ram
>> -Daniel