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

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel