On Sat, Dec 08, 2018 at 06:47:40PM +0000, Winkler, Tomas wrote: > > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel Vetter > > Sent: Friday, December 07, 2018 16:17 > > To: C, Ramalingam <ramalingam.c@xxxxxxxxx> > > Cc: Daniel Vetter <daniel@xxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- > > devel@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; Winkler, Tomas > > <tomas.winkler@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx> > > 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@xxxxxxxxx> > > > > > --- > > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
