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

Ramalingam C ramalingam.c at intel.com
Tue May 29 09:27:56 UTC 2018



On Tuesday 29 May 2018 02:12 PM, Daniel Vetter wrote:
> 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 at 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.
Daniel,

Ok. I am implementing the components for interface between I915(master) 
and mei_hdcp(client).

And Meanwhile I would like to request you to review other changes too.

Thanks,
Ram
>
> 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



More information about the Intel-gfx mailing list