Re: [PATCH v4 30/41] drm/i915: Initialize HDCP2.2 and its MEI interface

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 29, 2018 at 08:53:37AM +0200, Daniel Vetter wrote:
> On Fri, May 25, 2018 at 04:42:52PM +0530, Ramalingam C wrote:
> > 
> > 
> > On Thursday 24 May 2018 01:36 PM, Daniel Vetter wrote:
> > > On Mon, May 21, 2018 at 06:23:49PM +0530, Ramalingam C wrote:
> > > > Initialize HDCP2.2 support. This includes the mei interface
> > > > initialization along with required notifier 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]
> > > > 
> > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_dp.c   |   3 +-
> > > >   drivers/gpu/drm/i915/intel_drv.h  |   5 +-
> > > >   drivers/gpu/drm/i915/intel_hdcp.c | 117 +++++++++++++++++++++++++++++++++++++-
> > > >   drivers/gpu/drm/i915/intel_hdmi.c |   2 +-
> > > >   4 files changed, 122 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 62f82c4298ac..276eb49113e9 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -6368,7 +6368,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 ac943ec73987..7aaaa50fc19f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -442,7 +442,7 @@ struct intel_hdcp {
> > > >   	/* mei interface related information */
> > > >   	struct mei_cl_device *cldev;
> > > >   	struct mei_hdcp_data mei_data;
> > > > -
> > > > +	struct notifier_block mei_cldev_nb;
> > > >   	struct delayed_work hdcp2_check_work;
> > > >   };
> > > > @@ -1940,7 +1940,8 @@ 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);
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > index f3f935046c31..9948e4b4e203 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > @@ -11,6 +11,7 @@
> > > >   #include <linux/i2c.h>
> > > >   #include <linux/random.h>
> > > >   #include <linux/mei_hdcp.h>
> > > > +#include <linux/notifier.h>
> > > >   #include "intel_drv.h"
> > > >   #include "i915_reg.h"
> > > > @@ -25,6 +26,7 @@ static int _intel_hdcp2_enable(struct intel_connector *connector);
> > > >   static int _intel_hdcp2_disable(struct intel_connector *connector);
> > > >   static void intel_hdcp2_check_work(struct work_struct *work);
> > > >   static int intel_hdcp2_check_link(struct intel_connector *connector);
> > > > +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)
> > > > @@ -766,11 +768,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)
> > > > @@ -779,7 +785,12 @@ int intel_hdcp_init(struct intel_connector *connector,
> > > >   	hdcp->hdcp_shim = hdcp_shim;
> > > >   	mutex_init(&hdcp->hdcp_mutex);
> > > >   	INIT_DELAYED_WORK(&hdcp->hdcp_check_work, intel_hdcp_check_work);
> > > > +	INIT_DELAYED_WORK(&hdcp->hdcp2_check_work, intel_hdcp2_check_work);
> > > >   	INIT_WORK(&hdcp->hdcp_prop_work, intel_hdcp_prop_work);
> > > > +
> > > > +	if (hdcp2_supported)
> > > > +		intel_hdcp2_init(connector);
> > > > +
> > > >   	return 0;
> > > >   }
> > > > @@ -1637,3 +1648,107 @@ static void intel_hdcp2_check_work(struct work_struct *work)
> > > >   		schedule_delayed_work(&hdcp->hdcp2_check_work,
> > > >   				      DRM_HDCP2_CHECK_PERIOD_MS);
> > > >   }
> > > > +
> > > > +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;
> > > Ok there's 0 locking here. If there's no locking then of course
> > > CONFIG_PROVE_LOCKING will not spot any issues.
> > > 
> > > It also means you're code is racy, e.g. if mei and i915 load at the same
> > > time, things could get corrupted in interesting ways. Same holds if you
> > > have ongoing hdcp2 operations going on in the intel_hdcp code, while the
> > > mei driver is being unloaded.
> > Daniel,
> > 
> > Both of these cases are not possible as MEI symbols are used in I915,
> > So I915 can't be loaded before or in parallel to MEI drivers.
> > Similarly we can't unload the MEI before I915 or in parallel. So I guess we
> > are out of above race mentioned
> 
> That's not how it works unfortunately. You can unbind drivers while the
> module is still there. And symbol availability doesn't guarantee you that
> the driver itself has loaded already. Same holds for i915.
> 
> Yes symbol availability not matching up with driver state is one of the
> neatest pitfalls of the linux driver model. Symbol availability only
> mostly matches driver state, but it can be different.
> 
> > MEI related data in intel_connector->hdcp is per connector basis. And per
> > connector data's are protected with mutex in authentication path.
> > 
> > In MEI HDCP driver's APIs too, there is no shared variable except mei_cldev
> > that also not modified in those mei_hdcp APIs.
> > So I am not seeing any race conditions as such. So there is no need for any
> > explicit locking between I915 and MEI.
> 
> Your notifier callback needs some lock to protect against i915-side
> modeset calls, in case the mei disappears while we try to use them.
> 
> I think at least, I need to look at the overall picture from your git tree
> again.
> 
> > > Note that you can unload the driver without having to unload the module,
> > > those 2 things aren't coupled.
> > Can you  please help me to understand more on this?
> > If you are referring to mei device removal, notification will be sent to
> > I915.
> > Even if a hdcp service is placed for a mei device, that is
> > unbinded/disabled, mei bus will gracefully decline the request.
> > 
> > So I don't expect any possible issues. Help me if you otherwise.
> > 
> > > Another reason for not using notifiers is that it's another reinvented
> > > wheel to make multi-driver stuff work. We already have the component
> > > framework for this, and we're already using the component framework for
> > > the snd-hda vs. i915 coordination.
> > If we need to we can do with component. Exploring what is needed here.
> > At First glance I915 will be component master and mei will be component
> > client.
> 
> I think you can hand-roll it with notifiers, just need to be careful that
> you don't hold locks too much. And that's kinda reinventing component
> framework in that case. Especially since you've used direct function
> calls, making mei a static dependency anyway.

Ok I looked at the overall picture with the git tree and I think:
- You need locking, at least for hdcp->cldev, or some other ordering
  guarantees around accessing that pointer.

- Given that you have a link-time dependency (by using the exported
  functions from mei_hdcp directly) we already have a strong dependency,
  and using the component framework makes sense I think. That should give
  you the required ordering guarantees around hdpc->cldev.

Cheers, Daniel

> -Daniel
> 
> > 
> > Thanks and Regards,
> > Ram
> > > 
> > > So here's my recommendation for getting this sorted out:
> > > 
> > > - Use the component framework to coordinate the loading of i915 and the
> > >    mei_hdcp driver.
> > > 
> > > - Bonus points for using device_link to tell the driver core about this,
> > >    which optimizes the load sequence (unfortunately we haven't done that
> > >    for snd-hda yet, instead have to work around ordering issues in the
> > >    suspend/resume handlers a bit).
> > > 
> > > - Drop all the EXPORT_SYMBOL and hard links between the two drivers.
> > >    Instead have a vtable, like we're using for the audio side.
> > > 
> > > - Make sure that any shared state is appropriately protected with a mutex.
> > >    Sprinkle lots of assert_lock_held over the code base to make sure you
> > >    didn't miss anything, and use CONFIG_PROVE_LOCKING to make sure there's
> > >    no deadlocks and oversights.
> > > 
> > > - Extend the igt drv_module_reload testcase to make sure you're exercising
> > >    the new EPROBE_DEFER point fully (should just need a kernel change to
> > >    add that as a potential load failure path).
> > > 
> > > I think with that we have a solid solution here which is aligned with how
> > > we handle this in other cases.
> > > 
> > > Thanks, Daniel
> > > 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void intel_hdcp2_exit(struct intel_connector *connector)
> > > > +{
> > > > +	intel_hdcp_disable(connector);
> > > > +	kfree(connector->hdcp.mei_data.streams);
> > > > +}
> > > > +
> > > > +static int mei_cldev_notify(struct notifier_block *nb, unsigned long event,
> > > > +			    void *cldev)
> > > > +{
> > > > +	struct intel_hdcp *hdcp = container_of(nb, struct intel_hdcp,
> > > > +					       mei_cldev_nb);
> > > > +	struct intel_connector *intel_connector = container_of(hdcp,
> > > > +							struct intel_connector,
> > > > +							hdcp);
> > > > +
> > > > +	DRM_DEBUG_KMS("[%s:%d] MEI_HDCP Notification. Interface: %s\n",
> > > > +		      intel_connector->base.name, intel_connector->base.base.id,
> > > > +		      cldev ? "UP" : "Down");
> > > > +
> > > > +	if (event == MEI_CLDEV_ENABLED) {
> > > > +		hdcp->cldev = cldev;
> > > > +		initialize_mei_hdcp_data(intel_connector);
> > > > +	} else {
> > > > +		hdcp->cldev = NULL;
> > > > +		intel_hdcp2_exit(intel_connector);
> > > > +	}
> > > > +
> > > > +	return NOTIFY_OK;
> > > > +}
> > > > +
> > > > +static inline
> > > > +bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
> > > > +{
> > > > +	return (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
> > > > +		IS_KABYLAKE(dev_priv));
> > > > +}
> > > > +
> > > > +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;
> > > > +
> > > > +	if (!is_hdcp2_supported(dev_priv))
> > > > +		return -EINVAL;
> > > > +
> > > > +	hdcp->hdcp2_supported = true;
> > > > +
> > > > +	hdcp->mei_cldev_nb.notifier_call = mei_cldev_notify;
> > > > +	ret = mei_cldev_register_notify(&hdcp->mei_cldev_nb);
> > > > +	if (ret) {
> > > > +		DRM_ERROR("mei_cldev not available. %d\n", ret);
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	ret = initialize_mei_hdcp_data(connector);
> > > > +	if (ret) {
> > > > +		mei_cldev_unregister_notify(&hdcp->mei_cldev_nb);
> > > > +		goto exit;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Incase notifier is triggered well before this point, request for
> > > > +	 * notification of the mei client device state.
> > > > +	 */
> > > > +	mei_cldev_poll_notification();
> > > > +
> > > > +exit:
> > > > +	if (ret)
> > > > +		hdcp->hdcp2_supported = false;
> > > > +
> > > > +	return ret;
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > index ee929f31f7db..a5cc73101acb 100644
> > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > @@ -2334,7 +2334,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");
> > > >   	}
> > > > -- 
> > > > 2.7.4
> > > > 
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
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