[Intel-gfx] [PATCH v8 05/35] drm/i915: MEI interface definition

C, Ramalingam ramalingam.c at intel.com
Wed Dec 12 08:58:29 UTC 2018


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 at intel.com>
>>>> Signed-off-by: Tomas Winkler <tomas.winkler at 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
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20181212/7f6f7461/attachment-0001.html>


More information about the Intel-gfx mailing list