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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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

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.

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.

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


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux