Re: [PATCH v9 08/39] drm/i915: hdcp1.4 CP_IRQ handling and SW encryption tracking

From: Daniel Vetter <daniel@ffwll.ch>
To: Ramalingam C <ramalingam.c@intel.com>
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, seanpaul@chromium.org,
	tomas.winkler@intel.com
Subject: Re: [PATCH v9 08/39] drm/i915: hdcp1.4 CP_IRQ handling and SW encryption tracking
Date: Wed, 19 Dec 2018 16:48:00 +0100	[thread overview]
Message-ID: <20181219154800.GP21184@phenom.ffwll.local> (raw)
In-Reply-To: <1544673701-6353-9-git-send-email-ramalingam.c@intel.com>

On Thu, Dec 13, 2018 at 09:31:10AM +0530, Ramalingam C wrote:
> "hdcp_encrypted" flag is defined to denote the HDCP1.4 encryption status.
> This SW tracking is used to determine the need for real hdcp1.4 disable
> and hdcp_check_link upon CP_IRQ.
> 
> On CP_IRQ we filter the CP_IRQ related to the states like Link failure
> and reauthentication req etc and handle them in hdcp_check_link.
> CP_IRQ corresponding to the authentication msg availability are ignored.
> 
> WARN_ON is added for the abrupt stop of HDCP encryption of a port.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h  |  5 ++-
>  drivers/gpu/drm/i915/intel_hdcp.c | 89 ++++++++++++++++++++++++++++++---------
>  3 files changed, 74 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aba884c64879..89315e15fb34 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4758,7 +4758,7 @@ static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
>  		intel_dp_handle_test_request(intel_dp);
>  
>  	if (val & DP_CP_IRQ)
> -		intel_hdcp_check_link(intel_dp->attached_connector);
> +		intel_hdcp_handle_cp_irq(intel_dp->attached_connector);
>  
>  	if (val & DP_SINK_SPECIFIC_IRQ)
>  		DRM_DEBUG_DRIVER("Sink specific irq unhandled\n");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 191b6e0f086c..decd0346c6a7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -393,6 +393,9 @@ struct intel_hdcp {
>  	struct delayed_work check_work;
>  	struct work_struct prop_work;
>  
> +	/* HDCP1.4 Encryption status */
> +	u8 hdcp_encrypted;

Another bool I guess? Or unsigned : 1;

> +
>  	/* HDCP2.2 related definitions */
>  	/* Flag indicates whether this connector supports HDCP2.2 or not. */
>  	u8 hdcp2_supported;
> @@ -2058,10 +2061,10 @@ int intel_hdcp_init(struct intel_connector *connector,
>  		    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_handle_cp_irq(struct intel_connector *connector);
>  
>  /* 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 9405fce07b93..2b7814a6f12b 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -75,6 +75,16 @@ bool intel_hdcp_capable(struct intel_connector *connector)
>  	return capable;
>  }
>  
> +static inline bool intel_hdcp_in_use(struct intel_connector *connector)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> +	enum port port = connector->encoder->port;
> +	u32 reg;
> +
> +	reg = I915_READ(PORT_HDCP_STATUS(port));
> +	return reg & HDCP_STATUS_ENC;
> +}
> +
>  static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
>  				    const struct intel_hdcp_shim *shim)
>  {
> @@ -669,6 +679,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  	DRM_DEBUG_KMS("[%s:%d] HDCP is being disabled...\n",
>  		      connector->base.name, connector->base.base.id);
>  
> +	hdcp->hdcp_encrypted = 0;
>  	I915_WRITE(PORT_HDCP_CONF(port), 0);
>  	if (intel_wait_for_register(dev_priv, PORT_HDCP_STATUS(port), ~0, 0,
>  				    ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) {
> @@ -714,8 +725,10 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  	/* Incase of authentication failures, HDCP spec expects reauth. */
>  	for (i = 0; i < tries; i++) {
>  		ret = intel_hdcp_auth(conn_to_dig_port(connector), hdcp->shim);
> -		if (!ret)
> +		if (!ret) {
> +			hdcp->hdcp_encrypted = 1;
>  			return 0;
> +		}
>  
>  		DRM_DEBUG_KMS("HDCP Auth failure (%d)\n", ret);
>  
> @@ -742,16 +755,17 @@ int intel_hdcp_check_link(struct intel_connector *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)
> +	/* Check_link valid only when HDCP1.4 is enabled */
> +	if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED ||
> +	    !hdcp->hdcp_encrypted) {
> +		ret = -EINVAL;
>  		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",
> +	if (WARN_ON(!intel_hdcp_in_use(connector))) {
> +		DRM_ERROR("%s:%d HDCP link stopped encryption,%x\n",
>  			  connector->base.name, connector->base.base.id,
>  			  I915_READ(PORT_HDCP_STATUS(port)));
>  		ret = -ENXIO;
> @@ -792,18 +806,6 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  	return ret;
>  }
>  
> -static void intel_hdcp_check_work(struct work_struct *work)
> -{
> -	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> -					       struct intel_hdcp,
> -					       check_work);
> -	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
> -
> -	if (!intel_hdcp_check_link(connector))
> -		schedule_delayed_work(&hdcp->check_work,
> -				      DRM_HDCP_CHECK_PERIOD_MS);
> -}

Why move the function?
> -
>  static void intel_hdcp_prop_work(struct work_struct *work)
>  {
>  	struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp,
> @@ -1028,6 +1030,25 @@ int hdcp2_deauthenticate_port(struct intel_connector *connector)
>  	return hdcp2_close_mei_session(connector);
>  }
>  
> +static inline void _intel_hdcp_check_work(struct intel_connector *connector)
> +{
> +	struct intel_hdcp *hdcp = &connector->hdcp;
> +
> +	if (!intel_hdcp_check_link(connector))
> +		schedule_delayed_work(&hdcp->check_work,
> +				      DRM_HDCP_CHECK_PERIOD_MS);
> +}
> +
> +static void intel_hdcp_check_work(struct work_struct *work)
> +{
> +	struct intel_hdcp *hdcp = container_of(to_delayed_work(work),
> +					       struct intel_hdcp,
> +					       check_work);
> +	struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
> +
> +	_intel_hdcp_check_work(connector);
> +}
> +
>  static int i915_hdcp_component_match(struct device *dev, void *data)
>  {
>  	return !strcmp(dev->driver->name, "mei_hdcp");
> @@ -1156,7 +1177,8 @@ int intel_hdcp_disable(struct intel_connector *connector)
>  
>  	if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>  		hdcp->value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
> -		ret = _intel_hdcp_disable(connector);
> +		if (hdcp->hdcp_encrypted)
> +			ret = _intel_hdcp_disable(connector);
>  	}
>  
>  	mutex_unlock(&hdcp->mutex);
> @@ -1196,3 +1218,30 @@ void intel_hdcp_atomic_check(struct drm_connector *connector,
>  						   new_state->crtc);
>  	crtc_state->mode_changed = true;
>  }
> +
> +/* Handles the CP_IRQ raised from the DP HDCP sink */
> +void intel_hdcp_handle_cp_irq(struct intel_connector *connector)
> +{
> +	struct intel_hdcp *hdcp = &connector->hdcp;
> +
> +	if (!hdcp->shim)
> +		return;
> +
> +	/*
> +	 * As of now we want to handle CP_IRQ triggered while encryption is
> +	 * enabled. This includes the notification for Topology change,
> +	 * Link failure, reauth request etc.
> +	 * Auth msg availability notifications are ignored with mutex_trylock
> +	 * as authentication holds the lock. Lock might be taken for link check
> +	 * or hdcp_disable too. In either of state we need not handle the
> +	 * CP_IRQ, for known reasons.
> +	 */
> +	if (!mutex_trylock(&hdcp->mutex))

This is not a good way to fix recursions, because the lock might be held
by someone else for other reasons, and then you just drop events on the
floor. Worse, usually it works, except when it stops working.

Since you already have a separate worker just schedule that one, and leave
it up to the worker to filter out events we don't care about.

> +		return;
> +
> +	if (!hdcp->hdcp_encrypted)
> +		return;

This check would need to be moved intel_hdcp_check_link().
> +
> +	mutex_unlock(&hdcp->mutex);
> +	_intel_hdcp_check_work(connector);

So instead of the direct call we need a schedule_work() here (without
delay ofc). Note that if the work is already scheduled it won't be
scheduled to run twice, so should just work as-is I think.
-Daniel
> +}
> -- 
> 2.7.4
> 

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