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

["C, Ramalingam" <ramalingam.c@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9199 bytes --]


On 12/6/2018 6:57 PM, 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?

Doable similar to how we are doing hdcp_enable. Needs the tracking of the spec in
use to pick proper timeout and functions.

I will look into it.

>
> Of course need to use the right timeout and call the right functions, but
> I think gives us less duplication in the complicated code.
>
>
>>   		}
>>   	}
>>   	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?

we could land here from CP_IRQ, with hdcp_exit called for shim removal.
But could avoid if we assert on the HDCP auth/encrypt status of the port
before check_link.

>
>> +
>> +	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.

Is this specific to drm_hdcp.h? In the previous version i had all header
changes in a single patch. But Seanpaul suggested for better understanding
of the changes, to introduce the changes at the patches where they are used.

Still should we go back on this?

--Ram

>
> Otherwise looks all reasonable.
> -Daniel
>
>
>>   
>>   /* Shared lengths/masks between HDMI/DVI/DisplayPort */
>>   #define DRM_HDCP_AN_LEN				8
>> -- 
>> 2.7.4
>>

[-- Attachment #1.2: Type: text/html, Size: 10318 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx