[PATCH v8 09/35] drm/i915: Implement HDCP2.2 link integrity check
C, Ramalingam
ramalingam.c at intel.com
Fri Dec 7 06:46:11 UTC 2018
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 at intel.com>
>> Reviewed-by: Uma Shankar <uma.shankar at 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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20181207/4497eb71/attachment-0001.html>
More information about the dri-devel
mailing list