[Intel-gfx] [PATCH v4 25/41] drm/i915: Enable and Disable HDCP2.2 port encryption

[Ramalingam C]

On Thursday 31 May 2018 12:39 PM, Daniel Vetter wrote:
> On Mon, May 21, 2018 at 06:23:44PM +0530, Ramalingam C wrote:
>> Implements the enable and disable functions for HDCP2.2 encryption
>> of the PORT.
>>
>> v2:
>>    intel_wait_for_register is used instead of wait_for. [Chris Wilson]
>> v3:
>>    No Changes.
>> v4:
>>    Debug msg is added for timeout at Disable of Encryption [Uma]
>>    %s/HDCP2_CTL/HDCP2_CTL
>>
>> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdcp.c | 57 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index bd0bfcfd5b8f..0386a67c3e32 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -19,6 +19,7 @@
>>   					 (enum hdcp_physical_port) (port))
>>   #define KEY_LOAD_TRIES	5
>>   #define HDCP2_LC_RETRY_CNT		3
>> +#define TIME_FOR_ENCRYPT_STATUS_CHANGE	32
>>   
>>   static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
>>   				    const struct intel_hdcp_shim *shim)
>> @@ -1397,3 +1398,59 @@ static int hdcp2_authenticate_sink(struct intel_connector *connector)
>>   
>>   	return ret;
>>   }
>> +
>> +static int hdcp2_enable_encryption(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;
>> +
>> +	if (I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS)
>> +		return 0;
> Relying on hw status for state decisions is in my experience bad - it
> papers over bugs in our state handling. We should know what the expected
> state of the hardware is.
>
> If you want to write a bit more robust code, then keep these checks, but
> wrap them in a WARN_ON. That way we'll know if there's a regression in the
> code (e.g. unbalanced enable/disable) right away, instead of these checks
> silently papering over them.
>
>> +
>> +	if (hdcp->hdcp_shim->toggle_signalling)
>> +		hdcp->hdcp_shim->toggle_signalling(intel_dig_port, true);
>> +
>> +	if (I915_READ(HDCP2_STATUS_DDI(port)) & LINK_AUTH_STATUS) {
> Same for this check here. In general control flow that depends upon
> register values which can change at runtime is very suspect from a design
> robustness point of view.

Sure. I will move these HW status check into the WARN_ONs.

-Ram.
> -Daniel
>
>> +
>> +		/* Link is Authenticated. Now set for Encryption */
>> +		I915_WRITE(HDCP2_CTL_DDI(port),
>> +			   I915_READ(HDCP2_CTL_DDI(port)) |
>> +			   CTL_LINK_ENCRYPTION_REQ);
>> +	}
>> +
>> +	ret = intel_wait_for_register(dev_priv, HDCP2_STATUS_DDI(port),
>> +				      LINK_ENCRYPTION_STATUS,
>> +				      LINK_ENCRYPTION_STATUS,
>> +				      TIME_FOR_ENCRYPT_STATUS_CHANGE);
>> +
>> +	return ret;
>> +}
>> +
>> +static int hdcp2_disable_encryption(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;
>> +
>> +	if (!(I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS))
>> +		return 0;
>> +
>> +	I915_WRITE(HDCP2_CTL_DDI(port),
>> +		   I915_READ(HDCP2_CTL_DDI(port)) & ~CTL_LINK_ENCRYPTION_REQ);
>> +
>> +	ret = intel_wait_for_register(dev_priv, HDCP2_STATUS_DDI(port),
>> +				      LINK_ENCRYPTION_STATUS, 0x0,
>> +				      TIME_FOR_ENCRYPT_STATUS_CHANGE);
>> +	if (ret == -ETIMEDOUT)
>> +		DRM_DEBUG_KMS("Disable Encryption Timedout");
>> +
>> +	if (hdcp->hdcp_shim->toggle_signalling)
>> +		hdcp->hdcp_shim->toggle_signalling(intel_dig_port, false);
>> +
>> +	return ret;
>> +}
>> -- 
>> 2.7.4
>>