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

[Daniel Vetter <daniel@ffwll.ch>]

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.
-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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx