Re: [PATCH] drm/i915: convert HDCP DRM_ERROR into DRM_DEBUG

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 25, 2018 at 11:47:52PM +0530, Ramalingam C wrote:
> Conceptually user should be knowing the feature status through uAPI.
> So HDCP authentication failure information need not DRM_ERRORS.
> They are needed only for ENG debugging.
> 
> And also in HDCP we tolerate the retries for HDCP authentication.
> Hence if we print the failure info initial attempts as ERRORs CI will
> fail the IGT.
> 
> So we present the information of the failures in form of DRM_DEBUG
> msgs for debugging purpose.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_hdcp.c | 46 +++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index 5b423a78518d..d92b04c3ed4e 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -166,8 +166,8 @@ static int intel_hdcp_load_keys(struct drm_i915_private *dev_priv)
>  					      SKL_PCODE_LOAD_HDCP_KEYS, 1);
>  		mutex_unlock(&dev_priv->pcu_lock);
>  		if (ret) {
> -			DRM_ERROR("Failed to initiate HDCP key load (%d)\n",
> -			          ret);
> +			DRM_DEBUG_KMS("Failed to initiate HDCP key load (%d)\n",
> +				      ret);

This one indicates an i915 hw failure, not anything wrong with the sink.
So should stay at DRM_ERROR.

Btw I noticed that 2 DRM_DEBUG_KMS in intel_hdcp_validate_v_prime also
indicate programming/i915-side hw errors. So should be upgraded to
DRM_ERROR I think (but in a separate patch).

>  			return ret;
>  		}
>  	} else {
> @@ -195,7 +195,7 @@ static int intel_write_sha_text(struct drm_i915_private *dev_priv, u32 sha_text)
>  	I915_WRITE(HDCP_SHA_TEXT, sha_text);
>  	if (intel_wait_for_register(dev_priv, HDCP_REP_CTL,
>  				    HDCP_SHA1_READY, HDCP_SHA1_READY, 1)) {
> -		DRM_ERROR("Timed out waiting for SHA1 ready\n");
> +		DRM_DEBUG_KMS("Timed out waiting for SHA1 ready\n");

Same here.

>  		return -ETIMEDOUT;
>  	}
>  	return 0;
> @@ -219,7 +219,7 @@ u32 intel_hdcp_get_repeater_ctl(struct intel_digital_port *intel_dig_port)
>  	default:
>  		break;
>  	}
> -	DRM_ERROR("Unknown port %d\n", port);
> +	DRM_DEBUG_KMS("Unknown port %d\n", port);

Also a programming error.

>  	return -EINVAL;
>  }
>  
> @@ -448,7 +448,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>  
>  	ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim);
>  	if (ret) {
> -		DRM_ERROR("KSV list failed to become ready (%d)\n", ret);
> +		DRM_DEBUG_KMS("KSV list failed to become ready (%d)\n", ret);

This one is a sink issue, so correct to change to DRM_DEBUG_KMS. If I
don't comment, assume that I agree with your change.

>  		return ret;
>  	}
>  
> @@ -458,7 +458,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>  
>  	if (DRM_HDCP_MAX_DEVICE_EXCEEDED(bstatus[0]) ||
>  	    DRM_HDCP_MAX_CASCADE_EXCEEDED(bstatus[1])) {
> -		DRM_ERROR("Max Topology Limit Exceeded\n");
> +		DRM_DEBUG_KMS("Max Topology Limit Exceeded\n");
>  		return -EPERM;
>  	}
>  
> @@ -494,7 +494,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port,
>  	}
>  
>  	if (i == tries) {
> -		DRM_ERROR("V Prime validation failed.(%d)\n", ret);
> +		DRM_DEBUG_KMS("V Prime validation failed.(%d)\n", ret);
>  		goto err;
>  	}
>  
> @@ -543,7 +543,7 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>  		if (ret)
>  			return ret;
>  		if (!hdcp_capable) {
> -			DRM_ERROR("Panel is not HDCP capable\n");
> +			DRM_DEBUG_KMS("Panel is not HDCP capable\n");
>  			return -EINVAL;
>  		}
>  	}
> @@ -557,7 +557,7 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>  	if (intel_wait_for_register(dev_priv, PORT_HDCP_STATUS(port),
>  				    HDCP_STATUS_AN_READY,
>  				    HDCP_STATUS_AN_READY, 1)) {
> -		DRM_ERROR("Timed out waiting for An\n");
> +		DRM_DEBUG_KMS("Timed out waiting for An\n");

Again this would indicate an i915 hw issue, so better to keep it at
DRM_ERROR.

>  		return -ETIMEDOUT;
>  	}
>  
> @@ -594,7 +594,7 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>  	/* Wait for R0 ready */
>  	if (wait_for(I915_READ(PORT_HDCP_STATUS(port)) &
>  		     (HDCP_STATUS_R0_READY | HDCP_STATUS_ENC), 1)) {
> -		DRM_ERROR("Timed out waiting for R0 ready\n");
> +		DRM_DEBUG_KMS("Timed out waiting for R0 ready\n");

Same, all the wait_for and wait_for_register indicate i915-side hw issues,
and should stay at DRM_ERROR. Otherwise we just reduce test coverage.
Please leave all of them as DRM_ERROR.

>  		return -ETIMEDOUT;
>  	}
>  
> @@ -629,15 +629,15 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>  	}
>  
>  	if (i == tries) {
> -		DRM_ERROR("Timed out waiting for Ri prime match (%x)\n",
> -			  I915_READ(PORT_HDCP_STATUS(port)));
> +		DRM_DEBUG_KMS("Timed out waiting for Ri prime match (%x)\n",
> +			      I915_READ(PORT_HDCP_STATUS(port)));
>  		return -ETIMEDOUT;
>  	}
>  
>  	/* Wait for encryption confirmation */
>  	if (intel_wait_for_register(dev_priv, PORT_HDCP_STATUS(port),
>  				    HDCP_STATUS_ENC, HDCP_STATUS_ENC, 20)) {
> -		DRM_ERROR("Timed out waiting for encryption\n");
> +		DRM_DEBUG_KMS("Timed out waiting for encryption\n");
>  		return -ETIMEDOUT;
>  	}
>  
> @@ -666,13 +666,13 @@ static int _intel_hdcp_disable(struct intel_connector *connector)
>  	I915_WRITE(PORT_HDCP_CONF(port), 0);
>  	if (intel_wait_for_register(dev_priv, PORT_HDCP_STATUS(port), ~0, 0,
>  				    20)) {
> -		DRM_ERROR("Failed to disable HDCP, timeout clearing status\n");
> +		DRM_DEBUG_KMS("Failed to disable HDCP, timeout.\n");
>  		return -ETIMEDOUT;
>  	}
>  
>  	ret = connector->hdcp_shim->toggle_signalling(intel_dig_port, false);
>  	if (ret) {
> -		DRM_ERROR("Failed to disable HDCP signalling\n");
> +		DRM_DEBUG_KMS("Failed to disable HDCP signalling\n");
>  		return ret;
>  	}
>  
> @@ -689,7 +689,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  		      connector->base.name, connector->base.base.id);
>  
>  	if (!hdcp_key_loadable(dev_priv)) {
> -		DRM_ERROR("HDCP key Load is not possible\n");
> +		DRM_DEBUG_KMS("HDCP key Load is not possible\n");
>  		return -ENXIO;
>  	}
>  
> @@ -700,7 +700,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  		intel_hdcp_clear_keys(dev_priv);
>  	}
>  	if (ret) {
> -		DRM_ERROR("Could not load HDCP keys, (%d)\n", ret);
> +		DRM_DEBUG_KMS("Could not load HDCP keys, (%d)\n", ret);
>  		return ret;
>  	}
>  
> @@ -717,7 +717,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector)
>  		_intel_hdcp_disable(connector);
>  	}
>  
> -	DRM_ERROR("HDCP authentication failed (%d tries/%d)\n", tries, ret);
> +	DRM_DEBUG_KMS("HDCP authentication failed (%d tries/%d)\n", tries, ret);
>  	return ret;
>  }
>  
> @@ -871,9 +871,9 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  		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)));
> +		DRM_DEBUG_KMS("%s:%d HDCP Link is not encrypted,%x\n",
> +			      connector->base.name, connector->base.base.id,
> +			      I915_READ(PORT_HDCP_STATUS(port)));

Again I think this would indicate an i915 hw issue, let's leave at
DRM_ERROR.

>  		ret = -ENXIO;
>  		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>  		schedule_work(&connector->hdcp_prop_work);
> @@ -895,7 +895,7 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  
>  	ret = _intel_hdcp_disable(connector);
>  	if (ret) {
> -		DRM_ERROR("Failed to disable hdcp (%d)\n", ret);
> +		DRM_DEBUG_KMS("Failed to disable hdcp (%d)\n", ret);

Disabling hdcp should always succeeds, again let's leave at DRM_ERROR.

>  		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>  		schedule_work(&connector->hdcp_prop_work);
>  		goto out;
> @@ -903,7 +903,7 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>  
>  	ret = _intel_hdcp_enable(connector);
>  	if (ret) {
> -		DRM_ERROR("Failed to enable hdcp (%d)\n", ret);
> +		DRM_DEBUG_KMS("Failed to enable hdcp (%d)\n", ret);

Yeah, this can fail when the sink is gone.

When you respin this, can you pls include the added patch to make the 2
debug outputs into DRM_ERROR, and the 4 patches you've identified we can
merge already? I want to make sure CI is still approving of all of them,
now that the kms_content_protection testcase has landed.

Thanks, Daniel

>  		connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
>  		schedule_work(&connector->hdcp_prop_work);
>  		goto out;
> -- 
> 2.7.4
> 

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux