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.
> +
> 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.
> +{
> + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
