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

Commit Message

Ramalingam C Nov. 27, 2018, 10:43 a.m. UTC

Add the HDCP2.2 initialization to the existing HDCP1.4 stack.

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 <[e-mail]>
---
 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(-)

Comments

Daniel Vetter Dec. 6, 2018, 10:03 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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
>

Ramalingam C Dec. 7, 2018, 4:54 a.m. UTC | #2

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 <[e-mail]>
>> ---
>>   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.

>
>> +
>>   	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.

--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
>>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 12/6/2018 3:33 PM, Daniel Vetter
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:[e-mail]">
      <pre class="moz-quote-pre" wrap="">On Tue, Nov 27, 2018 at 04:13:02PM +0530, Ramalingam C wrote:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Add the HDCP2.2 initialization to the existing HDCP1.4 stack.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
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.

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
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 <a class="moz-txt-link-rfc2396E" href="mailto:[e-mail]">&lt;[e-mail]&gt;</a>
---
 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);
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
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.</pre>
    </blockquote>
    <br>
    <pre>As we discussed, during v7--&gt;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.</pre>
    <blockquote type="cite"
      cite="mid:[e-mail]">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+
 	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) &amp;&amp; !intel_dp_is_edp(intel_dp)) {
-		int ret = intel_hdcp_init(intel_connector, &amp;intel_dp_hdcp_shim);
+		int ret = intel_hdcp_init(intel_connector, &amp;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) &amp;&amp; dev_priv-&gt;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)
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
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.
</pre>
    </blockquote>
    <br>
    <pre>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.

--Ram
</pre>
    <blockquote type="cite"
      cite="mid:[e-mail]">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+{
+	struct intel_hdcp *hdcp = &amp;connector-&gt;hdcp;
+	struct drm_i915_private *dev_priv = connector-&gt;base.dev-&gt;dev_private;
+	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
+	enum port port = intel_dig_port-&gt;base.port;
+	int ret = 0;
+
+	if (!hdcp-&gt;shim)
+		return -ENOENT;
+
+	mutex_lock(&amp;hdcp-&gt;mutex);
+
+	if (hdcp-&gt;value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
+		goto out;
+
+	if (!(I915_READ(PORT_HDCP_STATUS(port)) &amp; HDCP_STATUS_ENC)) {
+		DRM_ERROR("%s:%d HDCP check failed: link is not encrypted,%x\n",
+			  connector-&gt;base.name, connector-&gt;base.base.id,
+			  I915_READ(PORT_HDCP_STATUS(port)));
+		ret = -ENXIO;
+		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		schedule_work(&amp;hdcp-&gt;prop_work);
+		goto out;
+	}
+
+	if (hdcp-&gt;shim-&gt;check_link(intel_dig_port)) {
+		if (hdcp-&gt;value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
+			hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
+			schedule_work(&amp;hdcp-&gt;prop_work);
+		}
+		goto out;
+	}
+
+	DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying authentication\n",
+		      connector-&gt;base.name, connector-&gt;base.base.id);
+
+	ret = _intel_hdcp_disable(connector);
+	if (ret) {
+		DRM_ERROR("Failed to disable hdcp (%d)\n", ret);
+		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		schedule_work(&amp;hdcp-&gt;prop_work);
+		goto out;
+	}
+
+	ret = _intel_hdcp_enable(connector);
+	if (ret) {
+		DRM_ERROR("Failed to enable hdcp (%d)\n", ret);
+		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		schedule_work(&amp;hdcp-&gt;prop_work);
+		goto out;
+	}
+
+out:
+	mutex_unlock(&amp;hdcp-&gt;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) &amp;&amp; port &lt; PORT_E);
 }
 
+bool is_hdcp2_supported(struct drm_i915_private *dev_priv)
+{
+	return ((INTEL_GEN(dev_priv) &gt;= 10 || IS_GEMINILAKE(dev_priv) ||
+		 IS_KABYLAKE(dev_priv)) &amp;&amp; IS_ENABLED(CONFIG_INTEL_MEI_HDCP));
+}
+
+static void intel_hdcp2_init(struct intel_connector *connector)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector-&gt;base.dev);
+	struct intel_hdcp *hdcp = &amp;connector-&gt;hdcp;
+
+	WARN_ON(!is_hdcp2_supported(dev_priv));
+
+	/* TODO: MEI interface needs to be initialized here */
+	hdcp-&gt;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 = &amp;connector-&gt;hdcp;
 	int ret;
 
-	ret = drm_connector_attach_content_protection_property(
-			&amp;connector-&gt;base);
+	if (!shim)
+		return -EINVAL;
+
+	ret = drm_connector_attach_content_protection_property(&amp;connector-&gt;base);
 	if (ret)
 		return ret;
 
@@ -789,9 +868,37 @@ int intel_hdcp_init(struct intel_connector *connector,
 	mutex_init(&amp;hdcp-&gt;mutex);
 	INIT_DELAYED_WORK(&amp;hdcp-&gt;check_work, intel_hdcp_check_work);
 	INIT_WORK(&amp;hdcp-&gt;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 = &amp;dev_priv-&gt;drm;
+	struct intel_connector *intel_connector;
+	struct drm_connector *connector;
+	struct drm_connector_list_iter conn_iter;
+
+	drm_connector_list_iter_begin(dev, &amp;conn_iter);
+	drm_for_each_connector_iter(connector, &amp;conn_iter) {
+		intel_connector = to_intel_connector(connector);
+
+		if (!intel_connector-&gt;hdcp.shim)
+			continue;
+
+		intel_hdcp_disable(intel_connector);
+
+		mutex_lock(&amp;intel_connector-&gt;hdcp.mutex);
+		intel_connector-&gt;hdcp.hdcp2_supported = 0;
+		intel_connector-&gt;hdcp.shim = NULL;
+		mutex_unlock(&amp;intel_connector-&gt;hdcp.mutex);
+	}
+	drm_connector_list_iter_end(&amp;conn_iter);
+}
+
 int intel_hdcp_enable(struct intel_connector *connector)
 {
 	struct intel_hdcp *hdcp = &amp;connector-&gt;hdcp;
@@ -867,62 +974,3 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
 						   new_state-&gt;crtc);
 	crtc_state-&gt;mode_changed = true;
 }
-
-/* Implements Part 3 of the HDCP authorization procedure */
-int intel_hdcp_check_link(struct intel_connector *connector)
-{
-	struct intel_hdcp *hdcp = &amp;connector-&gt;hdcp;
-	struct drm_i915_private *dev_priv = connector-&gt;base.dev-&gt;dev_private;
-	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
-	enum port port = intel_dig_port-&gt;base.port;
-	int ret = 0;
-
-	if (!hdcp-&gt;shim)
-		return -ENOENT;
-
-	mutex_lock(&amp;hdcp-&gt;mutex);
-
-	if (hdcp-&gt;value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
-		goto out;
-
-	if (!(I915_READ(PORT_HDCP_STATUS(port)) &amp; HDCP_STATUS_ENC)) {
-		DRM_ERROR("%s:%d HDCP check failed: link is not encrypted,%x\n",
-			  connector-&gt;base.name, connector-&gt;base.base.id,
-			  I915_READ(PORT_HDCP_STATUS(port)));
-		ret = -ENXIO;
-		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
-		schedule_work(&amp;hdcp-&gt;prop_work);
-		goto out;
-	}
-
-	if (hdcp-&gt;shim-&gt;check_link(intel_dig_port)) {
-		if (hdcp-&gt;value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
-			hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
-			schedule_work(&amp;hdcp-&gt;prop_work);
-		}
-		goto out;
-	}
-
-	DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying authentication\n",
-		      connector-&gt;base.name, connector-&gt;base.base.id);
-
-	ret = _intel_hdcp_disable(connector);
-	if (ret) {
-		DRM_ERROR("Failed to disable hdcp (%d)\n", ret);
-		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
-		schedule_work(&amp;hdcp-&gt;prop_work);
-		goto out;
-	}
-
-	ret = _intel_hdcp_enable(connector);
-	if (ret) {
-		DRM_DEBUG_KMS("Failed to enable hdcp (%d)\n", ret);
-		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
-		schedule_work(&amp;hdcp-&gt;prop_work);
-		goto out;
-	}
-
-out:
-	mutex_unlock(&amp;hdcp-&gt;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,
-					  &amp;intel_hdmi_hdcp_shim);
+					  &amp;intel_hdmi_hdcp_shim, false);
 		if (ret)
 			DRM_DEBUG_KMS("HDCP init failed, skipping.\n");
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
The actual change left to wire hdcp2_supported seems ok with me, but hard
to tell without seeing more yet :-)
-Daniel

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap=""> 	}

Daniel Vetter Dec. 7, 2018, 2:16 p.m. UTC | #3

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 <[e-mail]>
> > > ---
> > >   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:
  - CrOS won't use ME
  - 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.

  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
> > >

Winkler, Tomas Dec. 8, 2018, 6:47 p.m. UTC | #4

> -----Original Message-----
> From: Daniel Vetter [mailto:[e-mail]] On Behalf Of Daniel Vetter
> Sent: Friday, December 07, 2018 16:17
> To: C, Ramalingam <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>; [e-mail]; dri-
> [e-mail]; [e-mail]; Winkler, Tomas
> <[e-mail]>; Shankar, Uma <[e-mail]>
> 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 <[e-mail]>
> > > > ---
> > > >   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. 
>   - CrOS won't use ME
I'm not so sure, we are working on it.

>   - 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 Dec. 10, 2018, 9:28 a.m. UTC | #5

On Sat, Dec 08, 2018 at 06:47:40PM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:[e-mail]] On Behalf Of Daniel Vetter
> > Sent: Friday, December 07, 2018 16:17
> > To: C, Ramalingam <[e-mail]>
> > Cc: Daniel Vetter <[e-mail]>; [e-mail]; dri-
> > [e-mail]; [e-mail]; Winkler, Tomas
> > <[e-mail]>; Shankar, Uma <[e-mail]>
> > 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 <[e-mail]>
> > > > > ---
> > > > >   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

Patch

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);
+
 	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)
+{
+	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");
 	}