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.
Note that you can unload the driver without having to unload the module,
those 2 things aren't coupled.
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.
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
