diff mbox series

[v8,09/35] drm/i915: Implement HDCP2.2 link integrity check

Message ID 1543315413-24302-10-git-send-email-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Implement HDCP2.2 | expand

Commit Message

Ramalingam C Nov. 27, 2018, 10:43 a.m. UTC 
Implements the link integrity check once in 500mSec.

Once encryption is enabled, an ongoing Link Integrity Check is
performed by the HDCP Receiver to check that cipher synchronization
is maintained between the HDCP Transmitter and the HDCP Receiver.

On the detection of synchronization lost, the HDCP Receiver must assert
the corresponding bits of the RxStatus register. The Transmitter polls
the RxStatus register and it may initiate re-authentication.

v2:
  Rebased.
v3:
  No Changes.
v4:
  enum check_link_response is used check the link status [Uma]
v5:
  Rebased as part of patch reordering.
v6:
  Required members of intel_hdcp is defined [Sean Paul]
v7:
  hdcp2_check_link is cancelled at required places.
v8:
  Rebased for the component i/f changes.
  Errors due to the sinks are reported as DEBUG logs.

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++--
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++
 drivers/gpu/drm/i915/intel_hdcp.c    | 83 +++++++++++++++++++++++++++++++++++-
 include/drm/drm_hdcp.h               |  8 ++++
 4 files changed, 103 insertions(+), 4 deletions(-)

Comments

Daniel Vetter Dec. 6, 2018, 1:27 p.m. UTC | #1 
On Tue, Nov 27, 2018 at 04:13:07PM +0530, Ramalingam C wrote:
> Implements the link integrity check once in 500mSec.
> 
> Once encryption is enabled, an ongoing Link Integrity Check is
> performed by the HDCP Receiver to check that cipher synchronization
> is maintained between the HDCP Transmitter and the HDCP Receiver.
> 
> On the detection of synchronization lost, the HDCP Receiver must assert
> the corresponding bits of the RxStatus register. The Transmitter polls
> the RxStatus register and it may initiate re-authentication.
> 
> v2:
>   Rebased.
> v3:
>   No Changes.
> v4:
>   enum check_link_response is used check the link status [Uma]
> v5:
>   Rebased as part of patch reordering.
> v6:
>   Required members of intel_hdcp is defined [Sean Paul]
> v7:
>   hdcp2_check_link is cancelled at required places.
> v8:
>   Rebased for the component i/f changes.
>   Errors due to the sinks are reported as DEBUG logs.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++--
>  drivers/gpu/drm/i915/intel_drv.h     |  5 +++
>  drivers/gpu/drm/i915/intel_hdcp.c    | 83 +++++++++++++++++++++++++++++++++++-
>  include/drm/drm_hdcp.h               |  8 ++++
>  4 files changed, 103 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e9f4e22b2a4e..fc63babce165 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15833,15 +15833,20 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
>  {
>  	struct intel_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
> +	struct intel_hdcp *hdcp;
>  
>  	/* Kill all the work that may have been queued by hpd. */
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	for_each_intel_connector_iter(connector, &conn_iter) {
> +		hdcp = &connector->hdcp;
> +
>  		if (connector->modeset_retry_work.func)
>  			cancel_work_sync(&connector->modeset_retry_work);
> -		if (connector->hdcp.shim) {
> -			cancel_delayed_work_sync(&connector->hdcp.check_work);
> -			cancel_work_sync(&connector->hdcp.prop_work);
> +		if (hdcp->shim) {
> +			cancel_delayed_work_sync(&hdcp->check_work);
> +			cancel_work_sync(&hdcp->prop_work);
> +			if (hdcp->hdcp2_supported)
> +				cancel_delayed_work_sync(&hdcp->hdcp2_check_work);

Locking of these workers is always tricky ... why can't we use the same
worker for both checking hdcp2 and hdcp1 link status?

Of course need to use the right timeout and call the right functions, but
I think gives us less duplication in the complicated code.


>  		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 24d258488efe..e6e32bf52568 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -403,6 +403,9 @@ struct intel_hdcp_shim {
>  	 */
>  	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
>  				  void *buf, size_t size);
> +
> +	/* HDCP2.2 Link Integrity Check */
> +	int (*check_2_2_link)(struct intel_digital_port *intel_dig_port);
>  };
>  
>  struct intel_hdcp {
> @@ -445,6 +448,8 @@ struct intel_hdcp {
>  	 * over re-Auth has to be triggered.
>  	 */
>  	u32 seq_num_m;
> +
> +	struct delayed_work hdcp2_check_work;
>  };
>  
>  struct intel_connector {
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 679f3c164582..98b112395a5a 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -1626,6 +1626,81 @@ static int _intel_hdcp2_disable(struct intel_connector *connector)
>  	return ret;
>  }
>  
> +/* Implements the Link Integrity Check for HDCP2.2 */
> +static int intel_hdcp2_check_link(struct intel_connector *connector)
> +{
> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	struct intel_hdcp *hdcp = &connector->hdcp;
> +	enum port port = connector->encoder->port;
> +
> +	int ret = 0;
> +
> +	if (!hdcp->shim)
> +		return -ENOENT;

Why is this possible? Should we wrap it into at least a WARN_ON?

> +
> +	mutex_lock(&hdcp->mutex);
> +
> +	if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> +		goto out;
> +
> +	if (!(I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS)) {
> +		DRM_ERROR("HDCP2.2 check failed: link is not encrypted, %x\n",
> +			  I915_READ(HDCP2_STATUS_DDI(port)));
> +		ret = -ENXIO;
> +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +		schedule_work(&hdcp->prop_work);
> +		goto out;
> +	}
> +
> +	ret = hdcp->shim->check_2_2_link(intel_dig_port);
> +	if (ret == DRM_HDCP_LINK_PROTECTED) {
> +		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] HDCP2.2 link failed, retrying auth\n",
> +		      connector->base.name, connector->base.base.id);
> +
> +	ret = _intel_hdcp2_disable(connector);
> +	if (ret) {
> +		DRM_ERROR("[%s:%d] Failed to disable hdcp2.2 (%d)\n",
> +			  connector->base.name, connector->base.base.id, ret);
> +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> +		schedule_work(&hdcp->prop_work);
> +		goto out;
> +	}
> +
> +	ret = _intel_hdcp2_enable(connector);
> +	if (ret) {
> +		DRM_DEBUG_KMS("[%s:%d] Failed to enable hdcp2.2 (%d)\n",
> +			      connector->base.name, connector->base.base.id,
> +			      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_hdcp2_check_work(struct work_struct *work)
> +{
> +	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> +					       struct intel_hdcp,
> +					       hdcp2_check_work);
> +	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
> +
> +	if (!intel_hdcp2_check_link(connector))
> +		schedule_delayed_work(&hdcp->hdcp2_check_work,
> +				      DRM_HDCP2_CHECK_PERIOD_MS);
> +}
> +
>  static int i915_hdcp_component_master_bind(struct device *dev)
>  {
>  	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> @@ -1762,6 +1837,7 @@ static void intel_hdcp2_init(struct intel_connector *connector)
>  		return;
>  	}
>  
> +	INIT_DELAYED_WORK(&hdcp->hdcp2_check_work, intel_hdcp2_check_work);
>  	hdcp->hdcp2_supported = 1;
>  }
>  
> @@ -1836,8 +1912,12 @@ int intel_hdcp_enable(struct intel_connector *connector)
>  	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
>  	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
>  	 */
> -	if (intel_hdcp2_capable(connector))
> +	if (intel_hdcp2_capable(connector)) {
>  		ret = _intel_hdcp2_enable(connector);
> +		if (!ret)
> +			schedule_delayed_work(&hdcp->hdcp2_check_work,
> +					      DRM_HDCP2_CHECK_PERIOD_MS);
> +	}
>  
>  	/* When HDCP2.2 fails, HDCP1.4 will be attempted */
>  	if (ret && intel_hdcp_capable(connector)) {
> @@ -1876,6 +1956,7 @@ int intel_hdcp_disable(struct intel_connector *connector)
>  
>  	mutex_unlock(&hdcp->mutex);
>  	cancel_delayed_work_sync(&hdcp->check_work);
> +	cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
>  	return ret;
>  }
>  
> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> index 3e45a7618ab3..be07a2b56d23 100644
> --- a/include/drm/drm_hdcp.h
> +++ b/include/drm/drm_hdcp.h
> @@ -11,6 +11,14 @@
>  
>  /* Period of hdcp checks (to ensure we're still authenticated) */
>  #define DRM_HDCP_CHECK_PERIOD_MS		(128 * 16)
> +#define DRM_HDCP2_CHECK_PERIOD_MS		500
> +
> +enum check_link_response {
> +	DRM_HDCP_LINK_PROTECTED	= 0,
> +	DRM_HDCP_TOPOLOGY_CHANGE,
> +	DRM_HDCP_LINK_INTEGRITY_FAILURE,
> +	DRM_HDCP_REAUTH_REQUEST
> +};

The drm_hdcp.h changes need to be all split out into sepearate patches I
think. Just for highlighting of the new shared bits. Also applies to the
previous one.

Otherwise looks all reasonable.
-Daniel


>  
>  /* Shared lengths/masks between HDMI/DVI/DisplayPort */
>  #define DRM_HDCP_AN_LEN				8
> -- 
> 2.7.4
>
Daniel Vetter Dec. 6, 2018, 1:41 p.m. UTC | #2 
On Thu, Dec 06, 2018 at 02:27:21PM +0100, Daniel Vetter wrote:
> On Tue, Nov 27, 2018 at 04:13:07PM +0530, Ramalingam C wrote:
> > Implements the link integrity check once in 500mSec.
> > 
> > Once encryption is enabled, an ongoing Link Integrity Check is
> > performed by the HDCP Receiver to check that cipher synchronization
> > is maintained between the HDCP Transmitter and the HDCP Receiver.
> > 
> > On the detection of synchronization lost, the HDCP Receiver must assert
> > the corresponding bits of the RxStatus register. The Transmitter polls
> > the RxStatus register and it may initiate re-authentication.
> > 
> > v2:
> >   Rebased.
> > v3:
> >   No Changes.
> > v4:
> >   enum check_link_response is used check the link status [Uma]
> > v5:
> >   Rebased as part of patch reordering.
> > v6:
> >   Required members of intel_hdcp is defined [Sean Paul]
> > v7:
> >   hdcp2_check_link is cancelled at required places.
> > v8:
> >   Rebased for the component i/f changes.
> >   Errors due to the sinks are reported as DEBUG logs.
> > 
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 11 +++--
> >  drivers/gpu/drm/i915/intel_drv.h     |  5 +++
> >  drivers/gpu/drm/i915/intel_hdcp.c    | 83 +++++++++++++++++++++++++++++++++++-
> >  include/drm/drm_hdcp.h               |  8 ++++
> >  4 files changed, 103 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e9f4e22b2a4e..fc63babce165 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15833,15 +15833,20 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
> >  {
> >  	struct intel_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> > +	struct intel_hdcp *hdcp;
> >  
> >  	/* Kill all the work that may have been queued by hpd. */
> >  	drm_connector_list_iter_begin(dev, &conn_iter);
> >  	for_each_intel_connector_iter(connector, &conn_iter) {
> > +		hdcp = &connector->hdcp;
> > +
> >  		if (connector->modeset_retry_work.func)
> >  			cancel_work_sync(&connector->modeset_retry_work);
> > -		if (connector->hdcp.shim) {
> > -			cancel_delayed_work_sync(&connector->hdcp.check_work);
> > -			cancel_work_sync(&connector->hdcp.prop_work);
> > +		if (hdcp->shim) {
> > +			cancel_delayed_work_sync(&hdcp->check_work);
> > +			cancel_work_sync(&hdcp->prop_work);
> > +			if (hdcp->hdcp2_supported)
> > +				cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
> 
> Locking of these workers is always tricky ... why can't we use the same
> worker for both checking hdcp2 and hdcp1 link status?
> 
> Of course need to use the right timeout and call the right functions, but
> I think gives us less duplication in the complicated code.

Locking and also the logic to update the content protection property. I
think sharing the entire main function, and splitting to hdcp1 vs. hdcp2
in the check/enable/disable functions would be neat.
-Daniel

> 
> 
> >  		}
> >  	}
> >  	drm_connector_list_iter_end(&conn_iter);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 24d258488efe..e6e32bf52568 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -403,6 +403,9 @@ struct intel_hdcp_shim {
> >  	 */
> >  	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
> >  				  void *buf, size_t size);
> > +
> > +	/* HDCP2.2 Link Integrity Check */
> > +	int (*check_2_2_link)(struct intel_digital_port *intel_dig_port);
> >  };
> >  
> >  struct intel_hdcp {
> > @@ -445,6 +448,8 @@ struct intel_hdcp {
> >  	 * over re-Auth has to be triggered.
> >  	 */
> >  	u32 seq_num_m;
> > +
> > +	struct delayed_work hdcp2_check_work;
> >  };
> >  
> >  struct intel_connector {
> > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > index 679f3c164582..98b112395a5a 100644
> > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > @@ -1626,6 +1626,81 @@ static int _intel_hdcp2_disable(struct intel_connector *connector)
> >  	return ret;
> >  }
> >  
> > +/* Implements the Link Integrity Check for HDCP2.2 */
> > +static int intel_hdcp2_check_link(struct intel_connector *connector)
> > +{
> > +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > +	enum port port = connector->encoder->port;
> > +
> > +	int ret = 0;
> > +
> > +	if (!hdcp->shim)
> > +		return -ENOENT;
> 
> Why is this possible? Should we wrap it into at least a WARN_ON?
> 
> > +
> > +	mutex_lock(&hdcp->mutex);
> > +
> > +	if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > +		goto out;
> > +
> > +	if (!(I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS)) {
> > +		DRM_ERROR("HDCP2.2 check failed: link is not encrypted, %x\n",
> > +			  I915_READ(HDCP2_STATUS_DDI(port)));
> > +		ret = -ENXIO;
> > +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > +		schedule_work(&hdcp->prop_work);
> > +		goto out;
> > +	}
> > +
> > +	ret = hdcp->shim->check_2_2_link(intel_dig_port);
> > +	if (ret == DRM_HDCP_LINK_PROTECTED) {
> > +		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] HDCP2.2 link failed, retrying auth\n",
> > +		      connector->base.name, connector->base.base.id);
> > +
> > +	ret = _intel_hdcp2_disable(connector);
> > +	if (ret) {
> > +		DRM_ERROR("[%s:%d] Failed to disable hdcp2.2 (%d)\n",
> > +			  connector->base.name, connector->base.base.id, ret);
> > +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > +		schedule_work(&hdcp->prop_work);
> > +		goto out;
> > +	}
> > +
> > +	ret = _intel_hdcp2_enable(connector);
> > +	if (ret) {
> > +		DRM_DEBUG_KMS("[%s:%d] Failed to enable hdcp2.2 (%d)\n",
> > +			      connector->base.name, connector->base.base.id,
> > +			      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_hdcp2_check_work(struct work_struct *work)
> > +{
> > +	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> > +					       struct intel_hdcp,
> > +					       hdcp2_check_work);
> > +	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
> > +
> > +	if (!intel_hdcp2_check_link(connector))
> > +		schedule_delayed_work(&hdcp->hdcp2_check_work,
> > +				      DRM_HDCP2_CHECK_PERIOD_MS);
> > +}
> > +
> >  static int i915_hdcp_component_master_bind(struct device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > @@ -1762,6 +1837,7 @@ static void intel_hdcp2_init(struct intel_connector *connector)
> >  		return;
> >  	}
> >  
> > +	INIT_DELAYED_WORK(&hdcp->hdcp2_check_work, intel_hdcp2_check_work);
> >  	hdcp->hdcp2_supported = 1;
> >  }
> >  
> > @@ -1836,8 +1912,12 @@ int intel_hdcp_enable(struct intel_connector *connector)
> >  	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
> >  	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
> >  	 */
> > -	if (intel_hdcp2_capable(connector))
> > +	if (intel_hdcp2_capable(connector)) {
> >  		ret = _intel_hdcp2_enable(connector);
> > +		if (!ret)
> > +			schedule_delayed_work(&hdcp->hdcp2_check_work,
> > +					      DRM_HDCP2_CHECK_PERIOD_MS);
> > +	}
> >  
> >  	/* When HDCP2.2 fails, HDCP1.4 will be attempted */
> >  	if (ret && intel_hdcp_capable(connector)) {
> > @@ -1876,6 +1956,7 @@ int intel_hdcp_disable(struct intel_connector *connector)
> >  
> >  	mutex_unlock(&hdcp->mutex);
> >  	cancel_delayed_work_sync(&hdcp->check_work);
> > +	cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
> >  	return ret;
> >  }
> >  
> > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> > index 3e45a7618ab3..be07a2b56d23 100644
> > --- a/include/drm/drm_hdcp.h
> > +++ b/include/drm/drm_hdcp.h
> > @@ -11,6 +11,14 @@
> >  
> >  /* Period of hdcp checks (to ensure we're still authenticated) */
> >  #define DRM_HDCP_CHECK_PERIOD_MS		(128 * 16)
> > +#define DRM_HDCP2_CHECK_PERIOD_MS		500
> > +
> > +enum check_link_response {
> > +	DRM_HDCP_LINK_PROTECTED	= 0,
> > +	DRM_HDCP_TOPOLOGY_CHANGE,
> > +	DRM_HDCP_LINK_INTEGRITY_FAILURE,
> > +	DRM_HDCP_REAUTH_REQUEST
> > +};
> 
> The drm_hdcp.h changes need to be all split out into sepearate patches I
> think. Just for highlighting of the new shared bits. Also applies to the
> previous one.
> 
> Otherwise looks all reasonable.
> -Daniel
> 
> 
> >  
> >  /* Shared lengths/masks between HDMI/DVI/DisplayPort */
> >  #define DRM_HDCP_AN_LEN				8
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Ramalingam C Dec. 7, 2018, 6:46 a.m. UTC | #3 
On 12/6/2018 6:57 PM, Daniel Vetter wrote:
> On Tue, Nov 27, 2018 at 04:13:07PM +0530, Ramalingam C wrote:
>> Implements the link integrity check once in 500mSec.
>>
>> Once encryption is enabled, an ongoing Link Integrity Check is
>> performed by the HDCP Receiver to check that cipher synchronization
>> is maintained between the HDCP Transmitter and the HDCP Receiver.
>>
>> On the detection of synchronization lost, the HDCP Receiver must assert
>> the corresponding bits of the RxStatus register. The Transmitter polls
>> the RxStatus register and it may initiate re-authentication.
>>
>> v2:
>>    Rebased.
>> v3:
>>    No Changes.
>> v4:
>>    enum check_link_response is used check the link status [Uma]
>> v5:
>>    Rebased as part of patch reordering.
>> v6:
>>    Required members of intel_hdcp is defined [Sean Paul]
>> v7:
>>    hdcp2_check_link is cancelled at required places.
>> v8:
>>    Rebased for the component i/f changes.
>>    Errors due to the sinks are reported as DEBUG logs.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> Reviewed-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 11 +++--
>>   drivers/gpu/drm/i915/intel_drv.h     |  5 +++
>>   drivers/gpu/drm/i915/intel_hdcp.c    | 83 +++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_hdcp.h               |  8 ++++
>>   4 files changed, 103 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e9f4e22b2a4e..fc63babce165 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15833,15 +15833,20 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
>>   {
>>   	struct intel_connector *connector;
>>   	struct drm_connector_list_iter conn_iter;
>> +	struct intel_hdcp *hdcp;
>>   
>>   	/* Kill all the work that may have been queued by hpd. */
>>   	drm_connector_list_iter_begin(dev, &conn_iter);
>>   	for_each_intel_connector_iter(connector, &conn_iter) {
>> +		hdcp = &connector->hdcp;
>> +
>>   		if (connector->modeset_retry_work.func)
>>   			cancel_work_sync(&connector->modeset_retry_work);
>> -		if (connector->hdcp.shim) {
>> -			cancel_delayed_work_sync(&connector->hdcp.check_work);
>> -			cancel_work_sync(&connector->hdcp.prop_work);
>> +		if (hdcp->shim) {
>> +			cancel_delayed_work_sync(&hdcp->check_work);
>> +			cancel_work_sync(&hdcp->prop_work);
>> +			if (hdcp->hdcp2_supported)
>> +				cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
> Locking of these workers is always tricky ... why can't we use the same
> worker for both checking hdcp2 and hdcp1 link status?

Doable similar to how we are doing hdcp_enable. Needs the tracking of the spec in
use to pick proper timeout and functions.

I will look into it.

>
> Of course need to use the right timeout and call the right functions, but
> I think gives us less duplication in the complicated code.
>
>
>>   		}
>>   	}
>>   	drm_connector_list_iter_end(&conn_iter);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 24d258488efe..e6e32bf52568 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -403,6 +403,9 @@ struct intel_hdcp_shim {
>>   	 */
>>   	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
>>   				  void *buf, size_t size);
>> +
>> +	/* HDCP2.2 Link Integrity Check */
>> +	int (*check_2_2_link)(struct intel_digital_port *intel_dig_port);
>>   };
>>   
>>   struct intel_hdcp {
>> @@ -445,6 +448,8 @@ struct intel_hdcp {
>>   	 * over re-Auth has to be triggered.
>>   	 */
>>   	u32 seq_num_m;
>> +
>> +	struct delayed_work hdcp2_check_work;
>>   };
>>   
>>   struct intel_connector {
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index 679f3c164582..98b112395a5a 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -1626,6 +1626,81 @@ static int _intel_hdcp2_disable(struct intel_connector *connector)
>>   	return ret;
>>   }
>>   
>> +/* Implements the Link Integrity Check for HDCP2.2 */
>> +static int intel_hdcp2_check_link(struct intel_connector *connector)
>> +{
>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> +	struct intel_hdcp *hdcp = &connector->hdcp;
>> +	enum port port = connector->encoder->port;
>> +
>> +	int ret = 0;
>> +
>> +	if (!hdcp->shim)
>> +		return -ENOENT;
> Why is this possible? Should we wrap it into at least a WARN_ON?

we could land here from CP_IRQ, with hdcp_exit called for shim removal.
But could avoid if we assert on the HDCP auth/encrypt status of the port
before check_link.

>
>> +
>> +	mutex_lock(&hdcp->mutex);
>> +
>> +	if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>> +		goto out;
>> +
>> +	if (!(I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS)) {
>> +		DRM_ERROR("HDCP2.2 check failed: link is not encrypted, %x\n",
>> +			  I915_READ(HDCP2_STATUS_DDI(port)));
>> +		ret = -ENXIO;
>> +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +		schedule_work(&hdcp->prop_work);
>> +		goto out;
>> +	}
>> +
>> +	ret = hdcp->shim->check_2_2_link(intel_dig_port);
>> +	if (ret == DRM_HDCP_LINK_PROTECTED) {
>> +		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] HDCP2.2 link failed, retrying auth\n",
>> +		      connector->base.name, connector->base.base.id);
>> +
>> +	ret = _intel_hdcp2_disable(connector);
>> +	if (ret) {
>> +		DRM_ERROR("[%s:%d] Failed to disable hdcp2.2 (%d)\n",
>> +			  connector->base.name, connector->base.base.id, ret);
>> +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +		schedule_work(&hdcp->prop_work);
>> +		goto out;
>> +	}
>> +
>> +	ret = _intel_hdcp2_enable(connector);
>> +	if (ret) {
>> +		DRM_DEBUG_KMS("[%s:%d] Failed to enable hdcp2.2 (%d)\n",
>> +			      connector->base.name, connector->base.base.id,
>> +			      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_hdcp2_check_work(struct work_struct *work)
>> +{
>> +	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
>> +					       struct intel_hdcp,
>> +					       hdcp2_check_work);
>> +	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
>> +
>> +	if (!intel_hdcp2_check_link(connector))
>> +		schedule_delayed_work(&hdcp->hdcp2_check_work,
>> +				      DRM_HDCP2_CHECK_PERIOD_MS);
>> +}
>> +
>>   static int i915_hdcp_component_master_bind(struct device *dev)
>>   {
>>   	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
>> @@ -1762,6 +1837,7 @@ static void intel_hdcp2_init(struct intel_connector *connector)
>>   		return;
>>   	}
>>   
>> +	INIT_DELAYED_WORK(&hdcp->hdcp2_check_work, intel_hdcp2_check_work);
>>   	hdcp->hdcp2_supported = 1;
>>   }
>>   
>> @@ -1836,8 +1912,12 @@ int intel_hdcp_enable(struct intel_connector *connector)
>>   	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
>>   	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
>>   	 */
>> -	if (intel_hdcp2_capable(connector))
>> +	if (intel_hdcp2_capable(connector)) {
>>   		ret = _intel_hdcp2_enable(connector);
>> +		if (!ret)
>> +			schedule_delayed_work(&hdcp->hdcp2_check_work,
>> +					      DRM_HDCP2_CHECK_PERIOD_MS);
>> +	}
>>   
>>   	/* When HDCP2.2 fails, HDCP1.4 will be attempted */
>>   	if (ret && intel_hdcp_capable(connector)) {
>> @@ -1876,6 +1956,7 @@ int intel_hdcp_disable(struct intel_connector *connector)
>>   
>>   	mutex_unlock(&hdcp->mutex);
>>   	cancel_delayed_work_sync(&hdcp->check_work);
>> +	cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
>>   	return ret;
>>   }
>>   
>> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
>> index 3e45a7618ab3..be07a2b56d23 100644
>> --- a/include/drm/drm_hdcp.h
>> +++ b/include/drm/drm_hdcp.h
>> @@ -11,6 +11,14 @@
>>   
>>   /* Period of hdcp checks (to ensure we're still authenticated) */
>>   #define DRM_HDCP_CHECK_PERIOD_MS		(128 * 16)
>> +#define DRM_HDCP2_CHECK_PERIOD_MS		500
>> +
>> +enum check_link_response {
>> +	DRM_HDCP_LINK_PROTECTED	= 0,
>> +	DRM_HDCP_TOPOLOGY_CHANGE,
>> +	DRM_HDCP_LINK_INTEGRITY_FAILURE,
>> +	DRM_HDCP_REAUTH_REQUEST
>> +};
> The drm_hdcp.h changes need to be all split out into sepearate patches I
> think. Just for highlighting of the new shared bits. Also applies to the
> previous one.

Is this specific to drm_hdcp.h? In the previous version i had all header
changes in a single patch. But Seanpaul suggested for better understanding
of the changes, to introduce the changes at the patches where they are used.

Still should we go back on this?

--Ram

>
> Otherwise looks all reasonable.
> -Daniel
>
>
>>   
>>   /* Shared lengths/masks between HDMI/DVI/DisplayPort */
>>   #define DRM_HDCP_AN_LEN				8
>> -- 
>> 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 6:57 PM, Daniel Vetter
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:20181206132721.GR21184@phenom.ffwll.local">
      <pre class="moz-quote-pre" wrap="">On Tue, Nov 27, 2018 at 04:13:07PM +0530, Ramalingam C wrote:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Implements the link integrity check once in 500mSec.

Once encryption is enabled, an ongoing Link Integrity Check is
performed by the HDCP Receiver to check that cipher synchronization
is maintained between the HDCP Transmitter and the HDCP Receiver.

On the detection of synchronization lost, the HDCP Receiver must assert
the corresponding bits of the RxStatus register. The Transmitter polls
the RxStatus register and it may initiate re-authentication.

v2:
  Rebased.
v3:
  No Changes.
v4:
  enum check_link_response is used check the link status [Uma]
v5:
  Rebased as part of patch reordering.
v6:
  Required members of intel_hdcp is defined [Sean Paul]
v7:
  hdcp2_check_link is cancelled at required places.
v8:
  Rebased for the component i/f changes.
  Errors due to the sinks are reported as DEBUG logs.

Signed-off-by: Ramalingam C <a class="moz-txt-link-rfc2396E" href="mailto:ramalingam.c@intel.com">&lt;ramalingam.c@intel.com&gt;</a>
Reviewed-by: Uma Shankar <a class="moz-txt-link-rfc2396E" href="mailto:uma.shankar@intel.com">&lt;uma.shankar@intel.com&gt;</a>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++--
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++
 drivers/gpu/drm/i915/intel_hdcp.c    | 83 +++++++++++++++++++++++++++++++++++-
 include/drm/drm_hdcp.h               |  8 ++++
 4 files changed, 103 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9f4e22b2a4e..fc63babce165 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15833,15 +15833,20 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
 {
 	struct intel_connector *connector;
 	struct drm_connector_list_iter conn_iter;
+	struct intel_hdcp *hdcp;
 
 	/* Kill all the work that may have been queued by hpd. */
 	drm_connector_list_iter_begin(dev, &amp;conn_iter);
 	for_each_intel_connector_iter(connector, &amp;conn_iter) {
+		hdcp = &amp;connector-&gt;hdcp;
+
 		if (connector-&gt;modeset_retry_work.func)
 			cancel_work_sync(&amp;connector-&gt;modeset_retry_work);
-		if (connector-&gt;hdcp.shim) {
-			cancel_delayed_work_sync(&amp;connector-&gt;hdcp.check_work);
-			cancel_work_sync(&amp;connector-&gt;hdcp.prop_work);
+		if (hdcp-&gt;shim) {
+			cancel_delayed_work_sync(&amp;hdcp-&gt;check_work);
+			cancel_work_sync(&amp;hdcp-&gt;prop_work);
+			if (hdcp-&gt;hdcp2_supported)
+				cancel_delayed_work_sync(&amp;hdcp-&gt;hdcp2_check_work);
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Locking of these workers is always tricky ... why can't we use the same
worker for both checking hdcp2 and hdcp1 link status?</pre>
    </blockquote>
    <pre>Doable similar to how we are doing hdcp_enable. Needs the tracking of the spec in
use to pick proper timeout and functions.

I will look into it.
</pre>
    <blockquote type="cite"
      cite="mid:20181206132721.GR21184@phenom.ffwll.local">
      <pre class="moz-quote-pre" wrap="">

Of course need to use the right timeout and call the right functions, but
I think gives us less duplication in the complicated code.


</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap=""> 		}
 	}
 	drm_connector_list_iter_end(&amp;conn_iter);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 24d258488efe..e6e32bf52568 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -403,6 +403,9 @@ struct intel_hdcp_shim {
 	 */
 	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
 				  void *buf, size_t size);
+
+	/* HDCP2.2 Link Integrity Check */
+	int (*check_2_2_link)(struct intel_digital_port *intel_dig_port);
 };
 
 struct intel_hdcp {
@@ -445,6 +448,8 @@ struct intel_hdcp {
 	 * over re-Auth has to be triggered.
 	 */
 	u32 seq_num_m;
+
+	struct delayed_work hdcp2_check_work;
 };
 
 struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 679f3c164582..98b112395a5a 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -1626,6 +1626,81 @@ static int _intel_hdcp2_disable(struct intel_connector *connector)
 	return ret;
 }
 
+/* Implements the Link Integrity Check for HDCP2.2 */
+static int intel_hdcp2_check_link(struct intel_connector *connector)
+{
+	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
+	struct drm_i915_private *dev_priv = to_i915(connector-&gt;base.dev);
+	struct intel_hdcp *hdcp = &amp;connector-&gt;hdcp;
+	enum port port = connector-&gt;encoder-&gt;port;
+
+	int ret = 0;
+
+	if (!hdcp-&gt;shim)
+		return -ENOENT;
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Why is this possible? Should we wrap it into at least a WARN_ON?</pre>
    </blockquote>
    <pre>we could land here from CP_IRQ, with hdcp_exit called for shim removal.
But could avoid if we assert on the HDCP auth/encrypt status of the port
before check_link.
</pre>
    <blockquote type="cite"
      cite="mid:20181206132721.GR21184@phenom.ffwll.local">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+
+	mutex_lock(&amp;hdcp-&gt;mutex);
+
+	if (hdcp-&gt;value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
+		goto out;
+
+	if (!(I915_READ(HDCP2_STATUS_DDI(port)) &amp; LINK_ENCRYPTION_STATUS)) {
+		DRM_ERROR("HDCP2.2 check failed: link is not encrypted, %x\n",
+			  I915_READ(HDCP2_STATUS_DDI(port)));
+		ret = -ENXIO;
+		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		schedule_work(&amp;hdcp-&gt;prop_work);
+		goto out;
+	}
+
+	ret = hdcp-&gt;shim-&gt;check_2_2_link(intel_dig_port);
+	if (ret == DRM_HDCP_LINK_PROTECTED) {
+		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] HDCP2.2 link failed, retrying auth\n",
+		      connector-&gt;base.name, connector-&gt;base.base.id);
+
+	ret = _intel_hdcp2_disable(connector);
+	if (ret) {
+		DRM_ERROR("[%s:%d] Failed to disable hdcp2.2 (%d)\n",
+			  connector-&gt;base.name, connector-&gt;base.base.id, ret);
+		hdcp-&gt;value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		schedule_work(&amp;hdcp-&gt;prop_work);
+		goto out;
+	}
+
+	ret = _intel_hdcp2_enable(connector);
+	if (ret) {
+		DRM_DEBUG_KMS("[%s:%d] Failed to enable hdcp2.2 (%d)\n",
+			      connector-&gt;base.name, connector-&gt;base.base.id,
+			      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_hdcp2_check_work(struct work_struct *work)
+{
+	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
+					       struct intel_hdcp,
+					       hdcp2_check_work);
+	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
+
+	if (!intel_hdcp2_check_link(connector))
+		schedule_delayed_work(&amp;hdcp-&gt;hdcp2_check_work,
+				      DRM_HDCP2_CHECK_PERIOD_MS);
+}
+
 static int i915_hdcp_component_master_bind(struct device *dev)
 {
 	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
@@ -1762,6 +1837,7 @@ static void intel_hdcp2_init(struct intel_connector *connector)
 		return;
 	}
 
+	INIT_DELAYED_WORK(&amp;hdcp-&gt;hdcp2_check_work, intel_hdcp2_check_work);
 	hdcp-&gt;hdcp2_supported = 1;
 }
 
@@ -1836,8 +1912,12 @@ int intel_hdcp_enable(struct intel_connector *connector)
 	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
 	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
 	 */
-	if (intel_hdcp2_capable(connector))
+	if (intel_hdcp2_capable(connector)) {
 		ret = _intel_hdcp2_enable(connector);
+		if (!ret)
+			schedule_delayed_work(&amp;hdcp-&gt;hdcp2_check_work,
+					      DRM_HDCP2_CHECK_PERIOD_MS);
+	}
 
 	/* When HDCP2.2 fails, HDCP1.4 will be attempted */
 	if (ret &amp;&amp; intel_hdcp_capable(connector)) {
@@ -1876,6 +1956,7 @@ int intel_hdcp_disable(struct intel_connector *connector)
 
 	mutex_unlock(&amp;hdcp-&gt;mutex);
 	cancel_delayed_work_sync(&amp;hdcp-&gt;check_work);
+	cancel_delayed_work_sync(&amp;hdcp-&gt;hdcp2_check_work);
 	return ret;
 }
 
diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
index 3e45a7618ab3..be07a2b56d23 100644
--- a/include/drm/drm_hdcp.h
+++ b/include/drm/drm_hdcp.h
@@ -11,6 +11,14 @@
 
 /* Period of hdcp checks (to ensure we're still authenticated) */
 #define DRM_HDCP_CHECK_PERIOD_MS		(128 * 16)
+#define DRM_HDCP2_CHECK_PERIOD_MS		500
+
+enum check_link_response {
+	DRM_HDCP_LINK_PROTECTED	= 0,
+	DRM_HDCP_TOPOLOGY_CHANGE,
+	DRM_HDCP_LINK_INTEGRITY_FAILURE,
+	DRM_HDCP_REAUTH_REQUEST
+};
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
The drm_hdcp.h changes need to be all split out into sepearate patches I
think. Just for highlighting of the new shared bits. Also applies to the
previous one.</pre>
    </blockquote>
    <pre>Is this specific to drm_hdcp.h? In the previous version i had all header
changes in a single patch. But Seanpaul suggested for better understanding
of the changes, to introduce the changes at the patches where they are used.

Still should we go back on this?

--Ram
</pre>
    <blockquote type="cite"
      cite="mid:20181206132721.GR21184@phenom.ffwll.local">
      <pre class="moz-quote-pre" wrap="">

Otherwise looks all reasonable.
-Daniel


</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap=""> 
 /* Shared lengths/masks between HDMI/DVI/DisplayPort */
 #define DRM_HDCP_AN_LEN				8
Daniel Vetter Dec. 7, 2018, 2:36 p.m. UTC | #4 
On Fri, Dec 07, 2018 at 12:16:11PM +0530, C, Ramalingam wrote:
> 
> On 12/6/2018 6:57 PM, Daniel Vetter wrote:
> > On Tue, Nov 27, 2018 at 04:13:07PM +0530, Ramalingam C wrote:
> > > Implements the link integrity check once in 500mSec.
> > > 
> > > Once encryption is enabled, an ongoing Link Integrity Check is
> > > performed by the HDCP Receiver to check that cipher synchronization
> > > is maintained between the HDCP Transmitter and the HDCP Receiver.
> > > 
> > > On the detection of synchronization lost, the HDCP Receiver must assert
> > > the corresponding bits of the RxStatus register. The Transmitter polls
> > > the RxStatus register and it may initiate re-authentication.
> > > 
> > > v2:
> > >    Rebased.
> > > v3:
> > >    No Changes.
> > > v4:
> > >    enum check_link_response is used check the link status [Uma]
> > > v5:
> > >    Rebased as part of patch reordering.
> > > v6:
> > >    Required members of intel_hdcp is defined [Sean Paul]
> > > v7:
> > >    hdcp2_check_link is cancelled at required places.
> > > v8:
> > >    Rebased for the component i/f changes.
> > >    Errors due to the sinks are reported as DEBUG logs.
> > > 
> > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > > Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_display.c | 11 +++--
> > >   drivers/gpu/drm/i915/intel_drv.h     |  5 +++
> > >   drivers/gpu/drm/i915/intel_hdcp.c    | 83 +++++++++++++++++++++++++++++++++++-
> > >   include/drm/drm_hdcp.h               |  8 ++++
> > >   4 files changed, 103 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index e9f4e22b2a4e..fc63babce165 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -15833,15 +15833,20 @@ static void intel_hpd_poll_fini(struct drm_device *dev)
> > >   {
> > >   	struct intel_connector *connector;
> > >   	struct drm_connector_list_iter conn_iter;
> > > +	struct intel_hdcp *hdcp;
> > >   	/* Kill all the work that may have been queued by hpd. */
> > >   	drm_connector_list_iter_begin(dev, &conn_iter);
> > >   	for_each_intel_connector_iter(connector, &conn_iter) {
> > > +		hdcp = &connector->hdcp;
> > > +
> > >   		if (connector->modeset_retry_work.func)
> > >   			cancel_work_sync(&connector->modeset_retry_work);
> > > -		if (connector->hdcp.shim) {
> > > -			cancel_delayed_work_sync(&connector->hdcp.check_work);
> > > -			cancel_work_sync(&connector->hdcp.prop_work);
> > > +		if (hdcp->shim) {
> > > +			cancel_delayed_work_sync(&hdcp->check_work);
> > > +			cancel_work_sync(&hdcp->prop_work);
> > > +			if (hdcp->hdcp2_supported)
> > > +				cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
> > Locking of these workers is always tricky ... why can't we use the same
> > worker for both checking hdcp2 and hdcp1 link status?
> 
> Doable similar to how we are doing hdcp_enable. Needs the tracking of the spec in
> use to pick proper timeout and functions.

Yeah we definitely want separate enable/disable functions for hdcp1 and
hdcp2. I was thinking of only sharing the content_protection status logic,
and the worker scheduling logic. That looks the same, and imo that's the
tricky part. I.e. separate the drm interfacing side more from the hdcp hw
flow.

> I will look into it.
> 
> > 
> > Of course need to use the right timeout and call the right functions, but
> > I think gives us less duplication in the complicated code.
> > 
> > 
> > >   		}
> > >   	}
> > >   	drm_connector_list_iter_end(&conn_iter);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 24d258488efe..e6e32bf52568 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -403,6 +403,9 @@ struct intel_hdcp_shim {
> > >   	 */
> > >   	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
> > >   				  void *buf, size_t size);
> > > +
> > > +	/* HDCP2.2 Link Integrity Check */
> > > +	int (*check_2_2_link)(struct intel_digital_port *intel_dig_port);
> > >   };
> > >   struct intel_hdcp {
> > > @@ -445,6 +448,8 @@ struct intel_hdcp {
> > >   	 * over re-Auth has to be triggered.
> > >   	 */
> > >   	u32 seq_num_m;
> > > +
> > > +	struct delayed_work hdcp2_check_work;
> > >   };
> > >   struct intel_connector {
> > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> > > index 679f3c164582..98b112395a5a 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -1626,6 +1626,81 @@ static int _intel_hdcp2_disable(struct intel_connector *connector)
> > >   	return ret;
> > >   }
> > > +/* Implements the Link Integrity Check for HDCP2.2 */
> > > +static int intel_hdcp2_check_link(struct intel_connector *connector)
> > > +{
> > > +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > > +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > > +	struct intel_hdcp *hdcp = &connector->hdcp;
> > > +	enum port port = connector->encoder->port;
> > > +
> > > +	int ret = 0;
> > > +
> > > +	if (!hdcp->shim)
> > > +		return -ENOENT;
> > Why is this possible? Should we wrap it into at least a WARN_ON?
> 
> we could land here from CP_IRQ, with hdcp_exit called for shim removal.
> But could avoid if we assert on the HDCP auth/encrypt status of the port
> before check_link.
> 
> > 
> > > +
> > > +	mutex_lock(&hdcp->mutex);
> > > +
> > > +	if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > > +		goto out;
> > > +
> > > +	if (!(I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS)) {
> > > +		DRM_ERROR("HDCP2.2 check failed: link is not encrypted, %x\n",
> > > +			  I915_READ(HDCP2_STATUS_DDI(port)));
> > > +		ret = -ENXIO;
> > > +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > +		schedule_work(&hdcp->prop_work);
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = hdcp->shim->check_2_2_link(intel_dig_port);
> > > +	if (ret == DRM_HDCP_LINK_PROTECTED) {
> > > +		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] HDCP2.2 link failed, retrying auth\n",
> > > +		      connector->base.name, connector->base.base.id);
> > > +
> > > +	ret = _intel_hdcp2_disable(connector);
> > > +	if (ret) {
> > > +		DRM_ERROR("[%s:%d] Failed to disable hdcp2.2 (%d)\n",
> > > +			  connector->base.name, connector->base.base.id, ret);
> > > +		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
> > > +		schedule_work(&hdcp->prop_work);
> > > +		goto out;
> > > +	}
> > > +
> > > +	ret = _intel_hdcp2_enable(connector);
> > > +	if (ret) {
> > > +		DRM_DEBUG_KMS("[%s:%d] Failed to enable hdcp2.2 (%d)\n",
> > > +			      connector->base.name, connector->base.base.id,
> > > +			      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_hdcp2_check_work(struct work_struct *work)
> > > +{
> > > +	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> > > +					       struct intel_hdcp,
> > > +					       hdcp2_check_work);
> > > +	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
> > > +
> > > +	if (!intel_hdcp2_check_link(connector))
> > > +		schedule_delayed_work(&hdcp->hdcp2_check_work,
> > > +				      DRM_HDCP2_CHECK_PERIOD_MS);
> > > +}
> > > +
> > >   static int i915_hdcp_component_master_bind(struct device *dev)
> > >   {
> > >   	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
> > > @@ -1762,6 +1837,7 @@ static void intel_hdcp2_init(struct intel_connector *connector)
> > >   		return;
> > >   	}
> > > +	INIT_DELAYED_WORK(&hdcp->hdcp2_check_work, intel_hdcp2_check_work);
> > >   	hdcp->hdcp2_supported = 1;
> > >   }
> > > @@ -1836,8 +1912,12 @@ int intel_hdcp_enable(struct intel_connector *connector)
> > >   	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
> > >   	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
> > >   	 */
> > > -	if (intel_hdcp2_capable(connector))
> > > +	if (intel_hdcp2_capable(connector)) {
> > >   		ret = _intel_hdcp2_enable(connector);
> > > +		if (!ret)
> > > +			schedule_delayed_work(&hdcp->hdcp2_check_work,
> > > +					      DRM_HDCP2_CHECK_PERIOD_MS);
> > > +	}
> > >   	/* When HDCP2.2 fails, HDCP1.4 will be attempted */
> > >   	if (ret && intel_hdcp_capable(connector)) {
> > > @@ -1876,6 +1956,7 @@ int intel_hdcp_disable(struct intel_connector *connector)
> > >   	mutex_unlock(&hdcp->mutex);
> > >   	cancel_delayed_work_sync(&hdcp->check_work);
> > > +	cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
> > >   	return ret;
> > >   }
> > > diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> > > index 3e45a7618ab3..be07a2b56d23 100644
> > > --- a/include/drm/drm_hdcp.h
> > > +++ b/include/drm/drm_hdcp.h
> > > @@ -11,6 +11,14 @@
> > >   /* Period of hdcp checks (to ensure we're still authenticated) */
> > >   #define DRM_HDCP_CHECK_PERIOD_MS		(128 * 16)
> > > +#define DRM_HDCP2_CHECK_PERIOD_MS		500
> > > +
> > > +enum check_link_response {
> > > +	DRM_HDCP_LINK_PROTECTED	= 0,
> > > +	DRM_HDCP_TOPOLOGY_CHANGE,
> > > +	DRM_HDCP_LINK_INTEGRITY_FAILURE,
> > > +	DRM_HDCP_REAUTH_REQUEST
> > > +};
> > The drm_hdcp.h changes need to be all split out into sepearate patches I
> > think. Just for highlighting of the new shared bits. Also applies to the
> > previous one.
> 
> Is this specific to drm_hdcp.h? In the previous version i had all header
> changes in a single patch. But Seanpaul suggested for better understanding
> of the changes, to introduce the changes at the patches where they are used.

Hm, conflicting review comments :-/

> Still should we go back on this?

We need acks for all drm changes when we want to merge them through
drm-intel (which I think is the best tree for this series). So it helps to
split them out. You can still have them all just split out right before
the i915 patch that uses the new functions (instead of one overall
drm_hdcp.h patch, which I agree with Sean can be tricky to understand).

Separate patch is more to highlight to non-intel people that we're
touching core code here too.
-Daniel

> 
> --Ram
> 
> > 
> > Otherwise looks all reasonable.
> > -Daniel
> > 
> > 
> > >   /* Shared lengths/masks between HDMI/DVI/DisplayPort */
> > >   #define DRM_HDCP_AN_LEN				8
> > > -- 
> > > 2.7.4
> > >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9f4e22b2a4e..fc63babce165 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15833,15 +15833,20 @@  static void intel_hpd_poll_fini(struct drm_device *dev)
 {
 	struct intel_connector *connector;
 	struct drm_connector_list_iter conn_iter;
+	struct intel_hdcp *hdcp;
 
 	/* Kill all the work that may have been queued by hpd. */
 	drm_connector_list_iter_begin(dev, &conn_iter);
 	for_each_intel_connector_iter(connector, &conn_iter) {
+		hdcp = &connector->hdcp;
+
 		if (connector->modeset_retry_work.func)
 			cancel_work_sync(&connector->modeset_retry_work);
-		if (connector->hdcp.shim) {
-			cancel_delayed_work_sync(&connector->hdcp.check_work);
-			cancel_work_sync(&connector->hdcp.prop_work);
+		if (hdcp->shim) {
+			cancel_delayed_work_sync(&hdcp->check_work);
+			cancel_work_sync(&hdcp->prop_work);
+			if (hdcp->hdcp2_supported)
+				cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
 		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 24d258488efe..e6e32bf52568 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -403,6 +403,9 @@  struct intel_hdcp_shim {
 	 */
 	int (*config_stream_type)(struct intel_digital_port *intel_dig_port,
 				  void *buf, size_t size);
+
+	/* HDCP2.2 Link Integrity Check */
+	int (*check_2_2_link)(struct intel_digital_port *intel_dig_port);
 };
 
 struct intel_hdcp {
@@ -445,6 +448,8 @@  struct intel_hdcp {
 	 * over re-Auth has to be triggered.
 	 */
 	u32 seq_num_m;
+
+	struct delayed_work hdcp2_check_work;
 };
 
 struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
index 679f3c164582..98b112395a5a 100644
--- a/drivers/gpu/drm/i915/intel_hdcp.c
+++ b/drivers/gpu/drm/i915/intel_hdcp.c
@@ -1626,6 +1626,81 @@  static int _intel_hdcp2_disable(struct intel_connector *connector)
 	return ret;
 }
 
+/* Implements the Link Integrity Check for HDCP2.2 */
+static int intel_hdcp2_check_link(struct intel_connector *connector)
+{
+	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	struct intel_hdcp *hdcp = &connector->hdcp;
+	enum port port = connector->encoder->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(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS)) {
+		DRM_ERROR("HDCP2.2 check failed: link is not encrypted, %x\n",
+			  I915_READ(HDCP2_STATUS_DDI(port)));
+		ret = -ENXIO;
+		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		schedule_work(&hdcp->prop_work);
+		goto out;
+	}
+
+	ret = hdcp->shim->check_2_2_link(intel_dig_port);
+	if (ret == DRM_HDCP_LINK_PROTECTED) {
+		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] HDCP2.2 link failed, retrying auth\n",
+		      connector->base.name, connector->base.base.id);
+
+	ret = _intel_hdcp2_disable(connector);
+	if (ret) {
+		DRM_ERROR("[%s:%d] Failed to disable hdcp2.2 (%d)\n",
+			  connector->base.name, connector->base.base.id, ret);
+		hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		schedule_work(&hdcp->prop_work);
+		goto out;
+	}
+
+	ret = _intel_hdcp2_enable(connector);
+	if (ret) {
+		DRM_DEBUG_KMS("[%s:%d] Failed to enable hdcp2.2 (%d)\n",
+			      connector->base.name, connector->base.base.id,
+			      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_hdcp2_check_work(struct work_struct *work)
+{
+	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
+					       struct intel_hdcp,
+					       hdcp2_check_work);
+	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
+
+	if (!intel_hdcp2_check_link(connector))
+		schedule_delayed_work(&hdcp->hdcp2_check_work,
+				      DRM_HDCP2_CHECK_PERIOD_MS);
+}
+
 static int i915_hdcp_component_master_bind(struct device *dev)
 {
 	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
@@ -1762,6 +1837,7 @@  static void intel_hdcp2_init(struct intel_connector *connector)
 		return;
 	}
 
+	INIT_DELAYED_WORK(&hdcp->hdcp2_check_work, intel_hdcp2_check_work);
 	hdcp->hdcp2_supported = 1;
 }
 
@@ -1836,8 +1912,12 @@  int intel_hdcp_enable(struct intel_connector *connector)
 	 * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup
 	 * is capable of HDCP2.2, it is preferred to use HDCP2.2.
 	 */
-	if (intel_hdcp2_capable(connector))
+	if (intel_hdcp2_capable(connector)) {
 		ret = _intel_hdcp2_enable(connector);
+		if (!ret)
+			schedule_delayed_work(&hdcp->hdcp2_check_work,
+					      DRM_HDCP2_CHECK_PERIOD_MS);
+	}
 
 	/* When HDCP2.2 fails, HDCP1.4 will be attempted */
 	if (ret && intel_hdcp_capable(connector)) {
@@ -1876,6 +1956,7 @@  int intel_hdcp_disable(struct intel_connector *connector)
 
 	mutex_unlock(&hdcp->mutex);
 	cancel_delayed_work_sync(&hdcp->check_work);
+	cancel_delayed_work_sync(&hdcp->hdcp2_check_work);
 	return ret;
 }
 
diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
index 3e45a7618ab3..be07a2b56d23 100644
--- a/include/drm/drm_hdcp.h
+++ b/include/drm/drm_hdcp.h
@@ -11,6 +11,14 @@ 
 
 /* Period of hdcp checks (to ensure we're still authenticated) */
 #define DRM_HDCP_CHECK_PERIOD_MS		(128 * 16)
+#define DRM_HDCP2_CHECK_PERIOD_MS		500
+
+enum check_link_response {
+	DRM_HDCP_LINK_PROTECTED	= 0,
+	DRM_HDCP_TOPOLOGY_CHANGE,
+	DRM_HDCP_LINK_INTEGRITY_FAILURE,
+	DRM_HDCP_REAUTH_REQUEST
+};
 
 /* Shared lengths/masks between HDMI/DVI/DisplayPort */
 #define DRM_HDCP_AN_LEN				8