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

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx