Re: [RFC COMP I/F] drm/i915: Initialize HDCP2.2 and its MEI interface

[Ramalingam C <ramalingam.c@xxxxxxxxx>]

On Monday 09 July 2018 08:40 PM, Daniel Vetter wrote:
> On Mon, Jul 09, 2018 at 07:01:09PM +0530, Ramalingam C wrote:
> > Initialize HDCP2.2 support. This includes the mei interface
> > initialization along with required component registration.
> >
> > v2:
> >    mei interface handle is protected with mutex. [Chris Wilson]
> > v3:
> >    Notifiers are used for the mei interface state.
> > v4:
> >    Poll for mei client device state
> >    Error msg for out of mem [Uma]
> >    Inline req for init function removed [Uma]
> > v5:
> >    Rebase as Part of reordering.
> >    Component is used for the I915 and MEI_HDCP interface [Daniel]
> > v6:
> >    I915 registration is moved into hdcp component bind [Daniel]
> >
> > Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.c   |  33 +++++--
> >   drivers/gpu/drm/i915/i915_drv.h   |   3 +
> >   drivers/gpu/drm/i915/intel_dp.c   |   3 +-
> >   drivers/gpu/drm/i915/intel_drv.h  |   6 +-
> >   drivers/gpu/drm/i915/intel_hdcp.c | 193 +++++++++++++++++++++++++++++++++++++-
> >   drivers/gpu/drm/i915/intel_hdmi.c |   2 +-
> >   include/drm/i915_component.h      |  88 +++++++++++++++++
> >   7 files changed, 314 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 0db3c83cce29..e6dc0f409a6e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1310,6 +1310,19 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv)
> >   		DRM_INFO("DRM_I915_DEBUG_GEM enabled\n");
> >   }
> >   
> > +void i915_driver_load_tail(struct drm_i915_private *dev_priv)
> > +{
> This isn't quite enough, we also must delay anything that might do a
> modeset. For almost everything this happens when we call
> i915_driver_register, but the fbdev emulation gets set up before that. We
> need to also include that (and everything after that point) in the
> load_tail function here.
Ok. I will check that.
> > +	i915_driver_register(dev_priv);
> > +
> > +	intel_runtime_pm_enable(dev_priv);
> > +
> > +	intel_init_ipc(dev_priv);
> > +
> > +	intel_runtime_pm_put(dev_priv);
> > +
> > +	i915_welcome_messages(dev_priv);
> > +}
> > +
> >   /**
> >    * i915_driver_load - setup chip and create an initial config
> >    * @pdev: PCI device
> > @@ -1389,16 +1402,18 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
> >   	if (ret < 0)
> >   		goto out_cleanup_hw;
> >   
> > -	i915_driver_register(dev_priv);
> > -
> > -	intel_runtime_pm_enable(dev_priv);
> > -
> > -	intel_init_ipc(dev_priv);
> > -
> > -	intel_runtime_pm_put(dev_priv);
> > -
> > -	i915_welcome_messages(dev_priv);
> > +	if (is_hdcp2_supported(dev_priv)) {
> > +		ret = intel_hdcp_component_init(dev_priv);
> > +		if (ret) {
> > +			DRM_DEV_ERROR(&pdev->dev, "HDCP comp init failed %d\n",
> > +				      ret);
> > +			goto out_cleanup_hw;
> > +		}
> > +		DRM_DEV_INFO(&pdev->dev, "I915 waits for MEI HDCP bind.\n");
> > +		return 0;
> > +	}
> >   
> > +	i915_driver_load_tail(dev_priv);
> Yup, this is roughly what I had in mind. The only slightly annoying thing
> is that this sprinkles is_hdcp2_supported() checks all over the codebase.
> What I had in mind instead to have slightly more polished code, and make
> this componentn business more useful in general for us:
>
> - Put the component master into i915_dev_priv instead of hiding it in the
>    hdcp structure.
Even now component is in dev_priv only.
> - Create a 2nd component that fires when driver loading is complete (i.e.
>    right here). This can be a really dumb one with nothing as callbacks.
You mean register another client component in i915_hdcp_master_comp bind 
which
will trigger a bind for master component that is used to call load tail?
> - Put the driver_load_tail call unconditionally into the master_bind, and
>    put the component_master_add_with_match() call also here.
>
> - Don't move the hdcp init around since that's no longer necessary.
>
> >   	return 0;
> >   
> >   out_cleanup_hw:
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 09ab12458244..febb8c8dc457 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2119,6 +2119,8 @@ struct drm_i915_private {
> >   
> >   	struct i915_pmu pmu;
> >   
> > +	struct i915_hdcp_component *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.
> > @@ -2732,6 +2734,7 @@ int intel_engines_init_mmio(struct drm_i915_private *dev_priv);
> >   int intel_engines_init(struct drm_i915_private *dev_priv);
> >   
> >   u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv);
> > +void i915_driver_load_tail(struct drm_i915_private *dev_priv);
> >   
> >   /* intel_hotplug.c */
> >   void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 5be07e1d816d..12eb5bd33b7e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6406,7 +6406,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> >   	intel_dp_add_properties(intel_dp, connector);
> >   
> >   	if (is_hdcp_supported(dev_priv, port) && !intel_dp_is_edp(intel_dp)) {
> > -		int ret = intel_hdcp_init(intel_connector, &intel_dp_hdcp_shim);
> > +		int ret = intel_hdcp_init(intel_connector, &intel_dp_hdcp_shim,
> > +					  false);
> >   		if (ret)
> >   			DRM_DEBUG_KMS("HDCP init failed, skipping.\n");
> >   	}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 5799788c8f5d..c40c8283bf79 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -443,7 +443,6 @@ struct intel_hdcp {
> >   
> >   	/* mei interface related information */
> >   	struct mei_hdcp_data mei_data;
> > -
> >   	struct delayed_work hdcp2_check_work;
> >   };
> >   
> > @@ -1963,11 +1962,14 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> >   			     struct drm_connector_state *old_state,
> >   			     struct drm_connector_state *new_state);
> >   int intel_hdcp_init(struct intel_connector *connector,
> > -		    const struct intel_hdcp_shim *hdcp_shim);
> > +		    const struct intel_hdcp_shim *hdcp_shim,
> > +		    bool hdcp2_supported);
> >   int intel_hdcp_enable(struct intel_connector *connector);
> >   int intel_hdcp_disable(struct intel_connector *connector);
> >   int intel_hdcp_check_link(struct intel_connector *connector);
> >   bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);
> > +int intel_hdcp_component_init(struct drm_i915_private *dev_priv);
> > +bool is_hdcp2_supported(struct drm_i915_private *dev_priv);
> >   
> >   /* intel_psr.c */
> >   #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support)
> > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > index 65bbe5874eee..928fcd214cf0 100644
> > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > @@ -8,13 +8,19 @@
> >   
> >   #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 GET_MEI_DDI_INDEX(port)		(((port) == PORT_A) ? DDI_A : \
> > +					 (enum hdcp_physical_port)(port))
> > +
> > +static int intel_hdcp2_init(struct intel_connector *connector);
> >   
> >   static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
> >   				    const struct intel_hdcp_shim *shim)
> > @@ -743,11 +749,15 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port)
> >   }
> >   
> >   int intel_hdcp_init(struct intel_connector *connector,
> > -		    const struct intel_hdcp_shim *hdcp_shim)
> > +		    const struct intel_hdcp_shim *hdcp_shim,
> > +		    bool hdcp2_supported)
> >   {
> >   	struct intel_hdcp *hdcp = &connector->hdcp;
> >   	int ret;
> >   
> > +	if (!hdcp_shim)
> > +		return -EINVAL;
> > +
> >   	ret = drm_connector_attach_content_protection_property(
> >   			&connector->base);
> >   	if (ret)
> > @@ -757,6 +767,10 @@ int intel_hdcp_init(struct intel_connector *connector,
> >   	mutex_init(&hdcp->hdcp_mutex);
> >   	INIT_DELAYED_WORK(&hdcp->hdcp_check_work, intel_hdcp_check_work);
> >   	INIT_WORK(&hdcp->hdcp_prop_work, intel_hdcp_prop_work);
> > +
> > +	if (hdcp2_supported)
> > +		intel_hdcp2_init(connector);
> > +
> >   	return 0;
> >   }
> >   
> > @@ -896,3 +910,180 @@ int intel_hdcp_check_link(struct intel_connector *connector)
> >   	mutex_unlock(&hdcp->hdcp_mutex);
> >   	return ret;
> >   }
> > +
> > +static int i915_hdcp_component_master_bind(struct device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > +	struct i915_hdcp_component *comp = dev_priv->hdcp_comp;
> > +	static bool i915_loaded;
> > +	int ret;
> > +
> > +	mutex_lock(&comp->mutex);
> > +	ret = component_bind_all(dev, comp);
> > +	if (ret < 0) {
> > +		mutex_unlock(&comp->mutex);
> > +		return ret;
> > +	}
> > +
> > +	if (!i915_loaded)
> I have no idea why you need this one here ...
What if the mei_hdcp device got reset multiple times? We will get master 
bind triggered multiple times.
So only first time I915_load_tail will called.
> > +		i915_driver_load_tail(dev_priv);
> > +
> > +	mutex_unlock(&comp->mutex);
> Also not clear on the need for locking here? I thought component takes
> care of that for you.
Central locking in the component frame work will handle the 
synchronization between client and master.
But what if unbind triggered when the component callbacks are in use.?
Thought a mutex protecting the access for the component callbacks and 
values will solve this issue.

Thanks,
Ram
> > +	return 0;
> > +}
> > +
> > +static void i915_hdcp_component_master_unbind(struct device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > +	struct i915_hdcp_component *comp = dev_priv->hdcp_comp;
> > +
> > +	mutex_lock(&comp->mutex);
> Same here.
>
> Cheers, Daniel
>
> > +	component_unbind_all(dev, comp);
> > +	WARN_ON(comp->ops);
> > +	mutex_unlock(&comp->mutex);
> > +}
> > +
> > +static const struct component_master_ops i915_hdcp_component_master_ops = {
> > +	.bind = i915_hdcp_component_master_bind,
> > +	.unbind = i915_hdcp_component_master_unbind,
> > +};
> > +
> > +static void intel_hdcp_component_cleanup(struct drm_i915_private *dev_priv)
> > +{
> > +	component_master_del(dev_priv->drm.dev,
> > +			     &i915_hdcp_component_master_ops);
> > +	kfree(dev_priv->hdcp_comp);
> > +}
> > +
> > +static void intel_hdcp2_exit(struct intel_connector *connector)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > +
> > +	/* TO-DO: Disable the HDCP here */
> > +	kfree(connector->hdcp.mei_data.streams);
> > +
> > +	if (dev_priv->hdcp_comp)
> > +		intel_hdcp_component_cleanup(dev_priv);
> > +}
> > +
> > +static void intel_pull_down_mei_interface(struct device *kdev)
> > +{
> > +	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > +	struct drm_device *dev = &dev_priv->drm;
> > +	struct intel_connector *intel_connector;
> > +	struct drm_connector *connector;
> > +	struct drm_connector_list_iter conn_iter;
> > +
> > +	DRM_INFO("MEI Device is Down");
> > +	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.hdcp2_supported)
> > +			continue;
> > +
> > +		intel_hdcp2_exit(intel_connector);
> > +	}
> > +	drm_connector_list_iter_end(&conn_iter);
> > +}
> > +
> > +struct i915_hdcp_component_master_ops master_ops = {
> > +	.pull_down_interface = intel_pull_down_mei_interface,
> > +};
> > +
> > +static int i915_hdcp_component_master_match(struct device *dev, void *data)
> > +{
> > +	return !strcmp(dev->driver->name, "mei_hdcp");
> > +}
> > +
> > +int intel_hdcp_component_init(struct drm_i915_private *dev_priv)
> > +{
> > +	struct i915_hdcp_component *comp;
> > +	struct component_match *match = NULL;
> > +	int ret;
> > +
> > +	if (dev_priv->hdcp_comp)
> > +		return -EINVAL;
> > +
> > +	comp = kzalloc(sizeof(*comp), GFP_KERNEL);
> > +	if (!comp)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&comp->mutex);
> > +	comp->master_ops = &master_ops;
> > +	dev_priv->hdcp_comp = comp;
> > +
> > +	component_match_add(dev_priv->drm.dev, &match,
> > +			    i915_hdcp_component_master_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 = INTEGRATED;
> > +	data->protocol = hdcp->hdcp_shim->hdcp_protocol();
> > +
> > +	data->k = 1;
> > +	if (!data->streams)
> > +		data->streams = kcalloc(data->k,
> > +					sizeof(struct hdcp2_streamid_type),
> > +					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) ||
> > +		IS_KABYLAKE(dev_priv)) && IS_ENABLED(CONFIG_INTEL_MEI_HDCP));
> > +}
> > +
> > +static int 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));
> > +	ret = initialize_mei_hdcp_data(connector);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	if (ret)
> > +		kfree(hdcp->mei_data.streams);
> > +	hdcp->hdcp2_supported = true;
> > +
> > +exit:
> > +	return ret;
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 8363fbd18ee8..7988f958d835 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -2366,7 +2366,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >   
> >   	if (is_hdcp_supported(dev_priv, port)) {
> >   		int ret = intel_hdcp_init(intel_connector,
> > -					  &intel_hdmi_hdcp_shim);
> > +					  &intel_hdmi_hdcp_shim, false);
> >   		if (ret)
> >   			DRM_DEBUG_KMS("HDCP init failed, skipping.\n");
> >   	}
> > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > index 346b1f5cb180..9986174ee090 100644
> > --- a/include/drm/i915_component.h
> > +++ b/include/drm/i915_component.h
> > @@ -24,6 +24,10 @@
> >   #ifndef _I915_COMPONENT_H_
> >   #define _I915_COMPONENT_H_
> >   
> > +#include <linux/mei_cl_bus.h>
> > +#include <linux/mei_hdcp.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
> >    */
> > @@ -121,4 +125,88 @@ struct i915_audio_component {
> >   	const struct i915_audio_component_audio_ops *audio_ops;
> >   };
> >   
> > +struct i915_hdcp_component_ops {
> > +	/**
> > +	 * @owner: mei_hdcp module
> > +	 */
> > +	struct module *owner;
> > +	int (*initiate_hdcp2_session)(struct mei_cl_device *cldev,
> > +				      struct mei_hdcp_data *data,
> > +				      struct hdcp2_ake_init *ake_data);
> > +	int
> > +	(*verify_receiver_cert_prepare_km)(struct mei_cl_device *cldev,
> > +					   struct mei_hdcp_data *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 mei_cl_device *cldev,
> > +			     struct mei_hdcp_data *data,
> > +			     struct hdcp2_ake_send_hprime *rx_hprime);
> > +	int (*store_pairing_info)(struct mei_cl_device *cldev,
> > +				  struct mei_hdcp_data *data,
> > +				  struct hdcp2_ake_send_pairing_info
> > +								*pairing_info);
> > +	int (*initiate_locality_check)(struct mei_cl_device *cldev,
> > +				       struct mei_hdcp_data *data,
> > +				       struct hdcp2_lc_init *lc_init_data);
> > +	int (*verify_lprime)(struct mei_cl_device *cldev,
> > +			     struct mei_hdcp_data *data,
> > +			     struct hdcp2_lc_send_lprime *rx_lprime);
> > +	int (*get_session_key)(struct mei_cl_device *cldev,
> > +			       struct mei_hdcp_data *data,
> > +			       struct hdcp2_ske_send_eks *ske_data);
> > +	int
> > +	(*repeater_check_flow_prepare_ack)(struct mei_cl_device *cldev,
> > +					   struct mei_hdcp_data *data,
> > +					   struct hdcp2_rep_send_receiverid_list
> > +								*rep_topology,
> > +					   struct hdcp2_rep_send_ack
> > +								*rep_send_ack);
> > +	int (*verify_mprime)(struct mei_cl_device *cldev,
> > +			     struct mei_hdcp_data *data,
> > +			     struct hdcp2_rep_stream_ready *stream_ready);
> > +	int (*enable_hdcp_authentication)(struct mei_cl_device *cldev,
> > +					  struct mei_hdcp_data *data);
> > +	int (*close_hdcp_session)(struct mei_cl_device *cldev,
> > +				  struct mei_hdcp_data *data);
> > +};
> > +
> > +struct i915_hdcp_component_master_ops {
> > +	void (*pull_down_interface)(struct device *dev);
> > +};
> > +
> > +/**
> > + * struct i915_mei_hdcp_component - Used for direct communication between i915
> > + * and mei_hdcp drivers
> > + */
> > +struct i915_hdcp_component {
> > +	/**
> > +	 * @dev: Kdev of Slave Component. Passed to bind calls.
> > +	 */
> > +	struct device *dev;
> > +	/**
> > +	 * @i915_kdev: Kdev of I915. Used from the client component for
> > +	 * removing the reference to mei_cldev.
> > +	 */
> > +	struct device *i915_kdev;
> > +	/**
> > +	 * @mei_cldev: mei client device, used as parameter for ops
> > +	 */
> > +	struct mei_cl_device *mei_cldev;
> > +	/**
> > +	 * @mutex: Mutex to protect the state of mei_cldev
> > +	 */
> > +	struct mutex mutex;
> > +	/**
> > +	 * @ops: Ops implemented by mei_hdcp driver, used by i915 driver.
> > +	 */
> > +	const struct i915_hdcp_component_ops *ops;
> > +	/**
> > +	 * @master_ops: Ops implemented by I915 driver, used by mei_hdcp driver.
> > +	 */
> > +	const struct i915_hdcp_component_master_ops *master_ops;
> > +};
> > +
> >   #endif /* _I915_COMPONENT_H_ */
> > --
> > 2.7.4

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