[Intel-gfx] [PATCH 4/5] drm/i915: Poll hdcp register on sudden NACK

[Ramalingam C]

On Monday 26 February 2018 11:22 PM, Sean Paul wrote:
> On Mon, Feb 26, 2018 at 10:42:38PM +0530, Ramalingam C wrote:
>> In a connected state, If a HDMI HDCP sink is responded with NACK for
>> HDCP I2C register access, then HDMI HDCP spec mandates the polling
>> of any HDCP space registers for accessibility, minimum once in 2Secs
>> atleast for 4Secs.
>>
> I'm not convinced this is the right place to do this.
>
> The reason is that this is more complex than how you have it below. You can't
> access state outside of check/commit, so polling state->content_protection is
> not permissable from check_link. If the check fails, the driver will change the
> current value of content_protection, so userspace will be able to retry.
>
> In the case of enable, since it's synchronous, the error will be propagated to
> userspace and it can retry if that's the right thing to do.

Sean,

I am not grasping the concern clearly. Please help me on that.

Do you want to say why do you want to poll the hdcp space register's availability?
Because specification is asking for it. Functionally from our usecase perspective no
difference with and without this NACK handling.

Retry from userspace will not help to address the failures in the CTS tests mentioned below.

This could be done without referring connector state.

--Ram

>
> Sean
>
>> Just to make it simple, this is generically implemented for both HDMI
>> and DP. But we dont expect that this scanario will occur for DP.
>>
>> HDMI HDCP CTS Tests: 1A-04 and 1A-07A.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_hdcp.c | 74 +++++++++++++++++++++++++++++++++++----
>>   1 file changed, 67 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
>> index 95081aaa832a..14be14a45e5a 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -16,6 +16,47 @@
>>   
>>   #define KEY_LOAD_TRIES	5
>>   
>> +static
>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>> +{
>> +	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
>> +}
>> +
>> +static inline bool hdcp_port_accessible(struct intel_connector *connector)
>> +{
>> +	struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
>> +	int ret = -ENXIO;
>> +	u8 bksv[DRM_HDCP_KSV_LEN];
>> +
>> +	ret = connector->hdcp_shim->read_bksv(intel_dig_port, bksv);
>> +	if (!ret)
>> +		return true;
>> +	return false;
>> +}
>> +
>> +static bool wait_for_hdcp_port(struct intel_connector *connector)
>> +{
>> +	int i, tries = 10;
>> +
>> +	for (i = 0; i < tries; i++) {
>> +		if (connector->base.status != connector_status_connected ||
>> +		    connector->base.state->content_protection ==
>> +		    DRM_MODE_CONTENT_PROTECTION_UNDESIRED ||
>> +		    connector->hdcp_value ==
>> +		    DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>> +			return false;
>> +
>> +		if (hdcp_port_accessible(connector))
>> +			break;
>> +
>> +		msleep_interruptible(500);
>> +	}
>> +
>> +	if (i == tries)
>> +		return false;
>> +	return true;
>> +}
>> +
>>   static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
>>   				    const struct intel_hdcp_shim *shim)
>>   {
>> @@ -584,12 +625,6 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port,
>>   	return 0;
>>   }
>>   
>> -static
>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)
>> -{
>> -	return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base);
>> -}
>> -
>>   static int _intel_hdcp_disable(struct intel_connector *connector)
>>   {
>>   	struct drm_i915_private *dev_priv = connector->base.dev->dev_private;
>> @@ -719,14 +754,26 @@ int intel_hdcp_init(struct intel_connector *connector,
>>   
>>   int intel_hdcp_enable(struct intel_connector *connector)
>>   {
>> -	int ret;
>> +	int ret, tries = 2;
>>   
>>   	if (!connector->hdcp_shim)
>>   		return -ENOENT;
>>   
>>   	mutex_lock(&connector->hdcp_mutex);
>>   
>> +enable_hdcp:
>>   	ret = _intel_hdcp_enable(connector);
>> +
>> +	/*
>> +	 * Suddenly if sink is NACK-ed for the access of HDCP
>> +	 * registers, but display is still connected, poll for hdcp
>> +	 * port accessibility. One of the HDCP spec requirement.
>> +	 */
>> +	if ((ret == -EIO || ret == -ENXIO) &&
>> +	    connector->base.status == connector_status_connected &&
>> +	    !hdcp_port_accessible(connector))
>> +		if (wait_for_hdcp_port(connector) && --tries)
>> +			goto enable_hdcp;
>>   	if (ret)
>>   		goto out;
>>   
>> @@ -838,6 +885,19 @@ int intel_hdcp_check_link(struct intel_connector *connector)
>>   		goto out;
>>   	}
>>   
>> +	/*
>> +	 * Suddenly if sink is NACK-ed for the access of HDCP
>> +	 * registers, but display is still connected, poll for hdcp
>> +	 * port accessibility. One of the HDCP spec requirement.
>> +	 */
>> +	if (connector->base.status == connector_status_connected &&
>> +	    !hdcp_port_accessible(connector)) {
>> +		mutex_unlock(&connector->hdcp_mutex);
>> +		if (!wait_for_hdcp_port(connector))
>> +			return ret;
>> +		mutex_lock(&connector->hdcp_mutex);
>> +	}
>> +
>>   	ret = _intel_hdcp_enable(connector);
>>   	if (ret) {
>>   		DRM_ERROR("Failed to enable hdcp (%d)\n", ret);
>> -- 
>> 2.7.4
>>