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@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
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
