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

["C, Ramalingam" <ramalingam.c@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 25757 bytes --]


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

[-- Attachment #1.2: Type: text/html, Size: 28956 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx