Re: [PATCH v3 27/40] drm/i915: Implement HDCP2.2 link integrity check

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

On Monday 14 May 2018 03:15 PM, Shankar, Uma wrote:
>
>> -----Original Message-----
>> From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of
>> Ramalingam C
>> Sent: Tuesday, April 3, 2018 7:28 PM
>> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
>> seanpaul@chromium.org; daniel@ffwll.ch; chris@chris-wilson.co.uk;
>> jani.nikula@linux.intel.com; Winkler, Tomas <tomas.winkler@intel.com>;
>> Usyskin, Alexander <alexander.usyskin@intel.com>
>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> Subject: [PATCH v3 27/40] drm/i915: Implement HDCP2.2 link integrity check
>>
>> 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.
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_hdcp.c | 81
>> ++++++++++++++++++++++++++++++++++++++-
>> include/drm/drm_hdcp.h            |  8 ++++
>> 2 files changed, 88 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c
>> b/drivers/gpu/drm/i915/intel_hdcp.c
>> index 005627746ca5..e2aec73aefe3 100644
>> --- a/drivers/gpu/drm/i915/intel_hdcp.c
>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
>> @@ -23,6 +23,8 @@
>>
>> static int _intel_hdcp2_enable(struct intel_connector *connector);  static int
>> _intel_hdcp2_disable(struct intel_connector *connector);
>> +static void intel_hdcp2_check_work(struct work_struct *work); static
>> +int intel_hdcp2_check_link(struct intel_connector *connector);
>>
>> static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
>> 				    const struct intel_hdcp_shim *shim) @@ -
>> 1456,6 +1458,83 @@ static int _intel_hdcp2_enable(struct intel_connector
>> *connector)
>>
>> 	hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
>> 	schedule_work(&hdcp->hdcp_prop_work);
>> -
>> +	schedule_delayed_work(&hdcp->hdcp2_check_work,
>> +			      DRM_HDCP2_CHECK_PERIOD_MS);
>> 	return 0;
>> }
>> +
>> +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->hdcp_shim)
>> +		return -ENOENT;
>> +
>> +	mutex_lock(&hdcp->hdcp_mutex);
>> +
>> +	if (hdcp->hdcp_value ==
>> DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
>> +		goto out;
>> +
>> +	if (!(I915_READ(HDCP2_STATUS_DDI(port)) &
>> LINK_ENCRYPTION_STATUS)) {
>> +		DRM_ERROR("HDCP check failed: link is not encrypted, %x\n",
>> +			   I915_READ(HDCP2_STATUS_DDI(port)));
>> +		ret = -ENXIO;
>> +		hdcp->hdcp_value =
>> DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +		schedule_work(&hdcp->hdcp_prop_work);
>> +		goto out;
>> +	}
>> +
>> +	ret = hdcp->hdcp_shim->check_2_2_link(intel_dig_port);
> Check " hdcp->hdcp_shim->check_2_2_link " for NULL.
check_2_2_link is essential func ptr. Dont think it will be good to 
check on each loop.
Wondering if I need to check for the essential func ptr at init itself.

--Ram
>
>> +	if (!ret) {
Here actually we are comparing the ret == DRM_HDCP_LINK_PROTECTED 
indirectly here.
I will make it explicit, hence we can have the enum definition at this 
patch itself.
>> +		if (hdcp->hdcp_value !=
>> DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>> +			hdcp->hdcp_value =
>> DRM_MODE_CONTENT_PROTECTION_ENABLED;
>> +			schedule_work(&hdcp->hdcp_prop_work);
>> +		}
>> +		goto out;
>> +	}
>> +
>> +	DRM_INFO("[%s:%d] HDCP2.2 link failed, retrying authentication\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->hdcp_value =
>> DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +		schedule_work(&hdcp->hdcp_prop_work);
>> +		goto out;
>> +	}
>> +
>> +	ret = _intel_hdcp2_enable(connector);
>> +	if (ret) {
>> +		DRM_ERROR("[%s:%d] Failed to enable hdcp2.2 (%d)\n",
>> +			  connector->base.name, connector->base.base.id, ret);
>> +
>> +		hdcp->hdcp_value =
>> DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +		schedule_work(&hdcp->hdcp_prop_work);
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	mutex_unlock(&hdcp->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 = container_of(hdcp,
>> +						struct intel_connector,
>> +						hdcp);
>> +
>> +	if (!intel_hdcp2_check_link(connector))
> A print saying "link not active" will be good, since this was supposed to re-schedule itself and
> won't do it again if this doesn’t happen.
We dont have any usage of such log. Any link failure will be reported as 
errors already.
>
>> +		schedule_delayed_work(&hdcp->hdcp2_check_work,
>> +				      DRM_HDCP2_CHECK_PERIOD_MS);
>> +}
>> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index
>> f3f28414b189..b0601215c798 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 {
> Don't think this patch uses any of these enums. Add it where its first used.
Addressed above.

--Ram
>
>> +	DRM_HDCP_LINK_PROTECTED	= 0,
>> +	DRM_HDCP_TOPOLOGY_CHANGE,
>> +	DRM_HDCP_LINK_INTEGRITY_FAILURE,
>> +	DRM_HDCP_REAUTH_REQUEST
>> +};
>>
>> /* Shared lengths/masks between HDMI/DVI/DisplayPort */
>> #define DRM_HDCP_AN_LEN				8
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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