Re: [PATCH v8 04/35] drm/i915: Initialize HDCP2.2

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Dec 08, 2018 at 06:47:40PM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Friday, December 07, 2018 16:17
> > To: C, Ramalingam <ramalingam.c@intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>; intel-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org; daniel.vetter@ffwll.ch; Winkler, Tomas
> > <tomas.winkler@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> > Subject: Re: [PATCH v8 04/35] drm/i915: Initialize HDCP2.2
> > 
> > On Fri, Dec 07, 2018 at 10:24:26AM +0530, C, Ramalingam wrote:
> > >
> > > On 12/6/2018 3:33 PM, Daniel Vetter wrote:
> > > > On Tue, Nov 27, 2018 at 04:13:02PM +0530, Ramalingam C wrote:
> > > > > Add the HDCP2.2 initialization to the existing HDCP1.4 stack.
> > > > With the comments below addressed the commit message is a bit
> > > > untrue, since this just wires up a basic hdcp2_supported flag in a few
> > places.
> > > > Please make that clear.
> > > >
> > > > > 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]
> > > > > v5:
> > > > >    Rebase as Part of reordering.
> > > > >    Component is used for the I915 and MEI_HDCP interface [Daniel]
> > > > > v6:
> > > > >    HDCP2.2 uses the I915 component master to communicate with
> > mei_hdcp
> > > > > 	- [Daniel]
> > > > >    Required HDCP2.2 variables defined [Sean Paul]
> > > > > v7:
> > > > >    intel_hdcp2.2_init returns void [Uma]
> > > > >    Realigning the codes.
> > > > > v8:
> > > > >    Avoid using bool structure members.
> > > > >    MEI interface related changes are moved into separate patch.
> > > > >    Commit msg is updated accordingly.
> > > > >    intel_hdcp_exit is defined and used from i915_unload
> > > > >
> > > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/i915/i915_drv.c   |   2 +
> > > > >   drivers/gpu/drm/i915/intel_dp.c   |   3 +-
> > > > >   drivers/gpu/drm/i915/intel_drv.h  |  16 +++-
> > > > >   drivers/gpu/drm/i915/intel_hdcp.c | 172 ++++++++++++++++++++++++--
> > ------------
> > > > >   drivers/gpu/drm/i915/intel_hdmi.c |   2 +-
> > > > >   5 files changed, 130 insertions(+), 65 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > > b/drivers/gpu/drm/i915/i915_drv.c index b1d23c73c147..fbedd5024afe
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > > @@ -1755,6 +1755,8 @@ void i915_driver_unload(struct drm_device
> > *dev)
> > > > >   	disable_rpm_wakeref_asserts(dev_priv);
> > > > > +	intel_hdcp_exit(dev_priv);
> > > > This smells like a separate patch. Needs to be split out. Looking at
> > > > the implementation of intel_hdcp_exit I think it's papering over
> > > > some unload trouble. We should be shutting down all the outputs on
> > > > driver unload, which mei should be triggering (with the component
> > > > stuff), which means this code should be unecessary. But I'm not sure.
> > > >
> > > > Either way needs to be split out, but I think proper solution is to
> > > > drop it.
> > >
> > > As we discussed, during v7-->v8 i changed the component usage such that it
> > wont affect i915 load/unload.
> > > During the first connector init, component master will be added. And
> > > during the mei_client dev and driver binding, component will be added hence
> > the binding will happen with interface initialization from mei.
> > >
> > > Upon HDCP2.2 request, component binding will be checked before
> > attempting for HDCP2.2 auth.
> > > So component master unbind triggered due to mei component_del, will
> > teardown the HDCP2.2 capability of the I915.
> > >
> > > So in case of I915 unload trigger, from whatsoever reason, we need to
> > > clear the HDCP activities and bring down the i915_hdcp_component_master
> > and the interface with mei. For this purpose only intel_hdcp_exit is written
> > here.
> > 
> > Summarizing our irc discussion:
> > 
> > - I like the component usage of v7 much more.
> > 
> > - I still don't think we have a use-case for loading/unloading mei_hdcp on
> >   demand, or at least not in lockstep with i915.ko:
> We are testing all the MEI modules  reloading, this would be suddenly an exception. 

We're doing the same. And with the component stuff you can do that too in
a clean way. But there's a difference between a useful feature for
developers and something we need in production.

What I mean here is that I don't see a use-case for unloading mei_hdcp,
while i915 still needs to keep working. That's the only difference between
v7 and v8 in features support wrt driver loading/unloading.

> >   - CrOS won't use ME
> I'm not so sure, we are working on it.

Hm, that's interesting. We can't support HDCP2 on CrOS because of the ME
thing, and I haven't heard anything new from CrOS kernel engineers.
-Daniel

> >   - usual linux distros don't care about content protection, so won't even
> >     enable the MEI_HDCP driver
> >   - iotg/embedded either wants it (and then probably always) or doesn't
> >     want it (in which case it won't be built in). Plus embedded tends to
> >     have all built-in drivers anyway, so they all load in order.
> It really depends, there product range si really wide. 
> 
> >   And I don't want to review the validation coverage and locking and
> >   runtime consequences of making this possible, since I'm lazy :-)
> > 
> > - I looked lockdep splat in v7 again in more detail. It's not actually an
> >   issue with component usage, but just unlucky to now run into a
> >   preexisting issue: Any driver you unbind through the sysfs file will
> >   generate a lockdep splat if it removes any sysfs files itself. That's a
> >   pre-existing bug, easily reproduced by unbinding i915.
> > 
> >   Only thing new is that component connects the snd-hda-intel and i915
> >   unload sequence through the component lock (but does _not_ create a
> >   loop), and since we unbind snd-hda-intel through sysfs and i919
> >   unregisters _lots_ of sysfs files lockdep now sees the cycle. It's
> >   always been there.
> > 
> >   I'm working on a separate patch to fix this.
> > 
> > Cheers, Daniel
> > >
> > > >
> > > > > +
> > > > >   	i915_driver_unregister(dev_priv);
> > > > >   	if (i915_gem_suspend(dev_priv)) diff --git
> > > > > a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c index 18e3a5a3d873..ac62af073688
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -6675,7 +6675,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 a62d77b76291..85a526598096 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -388,6 +388,17 @@ struct intel_hdcp {
> > > > >   	u64 value;
> > > > >   	struct delayed_work check_work;
> > > > >   	struct work_struct prop_work;
> > > > > +
> > > > > +	/* HDCP2.2 related definitions */
> > > > > +	/* Flag indicates whether this connector supports HDCP2.2 or not. */
> > > > > +	u8 hdcp2_supported;
> > > > > +
> > > > > +	/*
> > > > > +	 * Content Stream Type defined by content owner. TYPE0(0x0) content
> > can
> > > > > +	 * flow in the link protected by HDCP2.2 or HDCP1.4, where as
> > TYPE1(0x1)
> > > > > +	 * content can flow only through a link protected by HDCP2.2.
> > > > > +	 */
> > > > > +	u8 content_type;
> > > > >   };
> > > > >   struct intel_connector {
> > > > > @@ -2016,12 +2027,15 @@ 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);
> > > > >   bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port
> > port);
> > > > >   bool intel_hdcp_capable(struct intel_connector *connector);
> > > > > +bool is_hdcp2_supported(struct drm_i915_private *dev_priv); void
> > > > > +intel_hdcp_exit(struct drm_i915_private *dev_priv);
> > > > >   /* intel_psr.c */
> > > > >   #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) &&
> > > > > dev_priv->psr.sink_support) diff --git
> > > > > a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > index 04ba6c13e715..99dddb540958 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > > > @@ -730,6 +730,65 @@ struct intel_connector
> > *intel_hdcp_to_connector(struct intel_hdcp *hdcp)
> > > > >   	return container_of(hdcp, struct intel_connector, hdcp);
> > > > >   }
> > > > > +/* Implements Part 3 of the HDCP authorization procedure */ int
> > > > > +intel_hdcp_check_link(struct intel_connector *connector)
> > > > Spurious movement of this function. Please drop, or if you need this
> > > > function moved, split it out into a separate patch that only moves
> > > > the function around.
> > >
> > > Intention is gathering the hdcp1.4 functions together before implementing
> > the hdcp2.2 code.
> > > But as you mentioned i will do it as a separate patch.
> > 
> > Ah yeah that makes sense, but commit message didn't explain that. Separate
> > patch sounds good.
> > -Daniel
> > 
> > >
> > > --Ram
> > >
> > > > > +{
> > > > > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > > > > +	struct drm_i915_private *dev_priv = connector->base.dev-
> > >dev_private;
> > > > > +	struct intel_digital_port *intel_dig_port =
> > conn_to_dig_port(connector);
> > > > > +	enum port port = intel_dig_port->base.port;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	if (!hdcp->shim)
> > > > > +		return -ENOENT;
> > > > > +
> > > > > +	mutex_lock(&hdcp->mutex);
> > > > > +
> > > > > +	if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > > > > +		goto out;
> > > > > +
> > > > > +	if (!(I915_READ(PORT_HDCP_STATUS(port)) & HDCP_STATUS_ENC)) {
> > > > > +		DRM_ERROR("%s:%d HDCP check failed: link is not
> > encrypted,%x\n",
> > > > > +			  connector->base.name, connector->base.base.id,
> > > > > +			  I915_READ(PORT_HDCP_STATUS(port)));
> > > > > +		ret = -ENXIO;
> > > > > +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > > > +		schedule_work(&hdcp->prop_work);
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	if (hdcp->shim->check_link(intel_dig_port)) {
> > > > > +		if (hdcp->value !=
> > DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > +			hdcp->value =
> > DRM_MODE_CONTENT_PROTECTION_ENABLED;
> > > > > +			schedule_work(&hdcp->prop_work);
> > > > > +		}
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying
> > authentication\n",
> > > > > +		      connector->base.name, connector->base.base.id);
> > > > > +
> > > > > +	ret = _intel_hdcp_disable(connector);
> > > > > +	if (ret) {
> > > > > +		DRM_ERROR("Failed to disable hdcp (%d)\n", ret);
> > > > > +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > > > +		schedule_work(&hdcp->prop_work);
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	ret = _intel_hdcp_enable(connector);
> > > > > +	if (ret) {
> > > > > +		DRM_ERROR("Failed to enable hdcp (%d)\n", ret);
> > > > > +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > > > +		schedule_work(&hdcp->prop_work);
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +out:
> > > > > +	mutex_unlock(&hdcp->mutex);
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > >   static void intel_hdcp_check_work(struct work_struct *work)
> > > > >   {
> > > > >   	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> > > > > @@ -774,14 +833,34 @@ bool is_hdcp_supported(struct
> > drm_i915_private *dev_priv, enum port port)
> > > > >   		!IS_CHERRYVIEW(dev_priv) && port < PORT_E);
> > > > >   }
> > > > > +bool is_hdcp2_supported(struct drm_i915_private *dev_priv) {
> > > > > +	return ((INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv) ||
> > > > > +		 IS_KABYLAKE(dev_priv)) &&
> > IS_ENABLED(CONFIG_INTEL_MEI_HDCP));
> > > > > +}
> > > > > +
> > > > > +static void intel_hdcp2_init(struct intel_connector *connector) {
> > > > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > > > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > > > > +
> > > > > +	WARN_ON(!is_hdcp2_supported(dev_priv));
> > > > > +
> > > > > +	/* TODO: MEI interface needs to be initialized here */
> > > > > +	hdcp->hdcp2_supported = 1;
> > > > > +}
> > > > > +
> > > > >   int intel_hdcp_init(struct intel_connector *connector,
> > > > > -		    const struct intel_hdcp_shim *shim)
> > > > > +		    const struct intel_hdcp_shim *shim,
> > > > > +		    bool hdcp2_supported)
> > > > >   {
> > > > >   	struct intel_hdcp *hdcp = &connector->hdcp;
> > > > >   	int ret;
> > > > > -	ret = drm_connector_attach_content_protection_property(
> > > > > -			&connector->base);
> > > > > +	if (!shim)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	ret =
> > > > > +drm_connector_attach_content_protection_property(&connector->base
> > > > > +);
> > > > >   	if (ret)
> > > > >   		return ret;
> > > > > @@ -789,9 +868,37 @@ int intel_hdcp_init(struct intel_connector
> > *connector,
> > > > >   	mutex_init(&hdcp->mutex);
> > > > >   	INIT_DELAYED_WORK(&hdcp->check_work, intel_hdcp_check_work);
> > > > >   	INIT_WORK(&hdcp->prop_work, intel_hdcp_prop_work);
> > > > > +
> > > > > +	if (hdcp2_supported)
> > > > > +		intel_hdcp2_init(connector);
> > > > > +
> > > > >   	return 0;
> > > > >   }
> > > > > +void intel_hdcp_exit(struct drm_i915_private *dev_priv) {
> > > > > +	struct drm_device *dev = &dev_priv->drm;
> > > > > +	struct intel_connector *intel_connector;
> > > > > +	struct drm_connector *connector;
> > > > > +	struct drm_connector_list_iter conn_iter;
> > > > > +
> > > > > +	drm_connector_list_iter_begin(dev, &conn_iter);
> > > > > +	drm_for_each_connector_iter(connector, &conn_iter) {
> > > > > +		intel_connector = to_intel_connector(connector);
> > > > > +
> > > > > +		if (!intel_connector->hdcp.shim)
> > > > > +			continue;
> > > > > +
> > > > > +		intel_hdcp_disable(intel_connector);
> > > > > +
> > > > > +		mutex_lock(&intel_connector->hdcp.mutex);
> > > > > +		intel_connector->hdcp.hdcp2_supported = 0;
> > > > > +		intel_connector->hdcp.shim = NULL;
> > > > > +		mutex_unlock(&intel_connector->hdcp.mutex);
> > > > > +	}
> > > > > +	drm_connector_list_iter_end(&conn_iter);
> > > > > +}
> > > > > +
> > > > >   int intel_hdcp_enable(struct intel_connector *connector)
> > > > >   {
> > > > >   	struct intel_hdcp *hdcp = &connector->hdcp; @@ -867,62 +974,3
> > > > > @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
> > > > >   						   new_state->crtc);
> > > > >   	crtc_state->mode_changed = true;
> > > > >   }
> > > > > -
> > > > > -/* Implements Part 3 of the HDCP authorization procedure */ -int
> > > > > intel_hdcp_check_link(struct intel_connector *connector) -{
> > > > > -	struct intel_hdcp *hdcp = &connector->hdcp;
> > > > > -	struct drm_i915_private *dev_priv = connector->base.dev-
> > >dev_private;
> > > > > -	struct intel_digital_port *intel_dig_port =
> > conn_to_dig_port(connector);
> > > > > -	enum port port = intel_dig_port->base.port;
> > > > > -	int ret = 0;
> > > > > -
> > > > > -	if (!hdcp->shim)
> > > > > -		return -ENOENT;
> > > > > -
> > > > > -	mutex_lock(&hdcp->mutex);
> > > > > -
> > > > > -	if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > > > > -		goto out;
> > > > > -
> > > > > -	if (!(I915_READ(PORT_HDCP_STATUS(port)) & HDCP_STATUS_ENC)) {
> > > > > -		DRM_ERROR("%s:%d HDCP check failed: link is not
> > encrypted,%x\n",
> > > > > -			  connector->base.name, connector->base.base.id,
> > > > > -			  I915_READ(PORT_HDCP_STATUS(port)));
> > > > > -		ret = -ENXIO;
> > > > > -		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > > > -		schedule_work(&hdcp->prop_work);
> > > > > -		goto out;
> > > > > -	}
> > > > > -
> > > > > -	if (hdcp->shim->check_link(intel_dig_port)) {
> > > > > -		if (hdcp->value !=
> > DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > > > > -			hdcp->value =
> > DRM_MODE_CONTENT_PROTECTION_ENABLED;
> > > > > -			schedule_work(&hdcp->prop_work);
> > > > > -		}
> > > > > -		goto out;
> > > > > -	}
> > > > > -
> > > > > -	DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying
> > authentication\n",
> > > > > -		      connector->base.name, connector->base.base.id);
> > > > > -
> > > > > -	ret = _intel_hdcp_disable(connector);
> > > > > -	if (ret) {
> > > > > -		DRM_ERROR("Failed to disable hdcp (%d)\n", ret);
> > > > > -		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > > > -		schedule_work(&hdcp->prop_work);
> > > > > -		goto out;
> > > > > -	}
> > > > > -
> > > > > -	ret = _intel_hdcp_enable(connector);
> > > > > -	if (ret) {
> > > > > -		DRM_DEBUG_KMS("Failed to enable hdcp (%d)\n", ret);
> > > > > -		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > > > -		schedule_work(&hdcp->prop_work);
> > > > > -		goto out;
> > > > > -	}
> > > > > -
> > > > > -out:
> > > > > -	mutex_unlock(&hdcp->mutex);
> > > > > -	return ret;
> > > > > -}
> > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > index e2c6a2b3e8f2..e755a3370bca 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > > > @@ -2417,7 +2417,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");
> > > > The actual change left to wire hdcp2_supported seems ok with me, but
> > > > hard to tell without seeing more yet :-) -Daniel
> > > >
> > > > >   	}
> > > > > --
> > > > > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel