On Thu, Dec 06, 2018 at 02:27:21PM +0100, 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@xxxxxxxxx>
> > Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> > ---
> > 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?
>
> Of course need to use the right timeout and call the right functions, but
> I think gives us less duplication in the complicated code.
Locking and also the logic to update the content protection property. I
think sharing the entire main function, and splitting to hdcp1 vs. hdcp2
in the check/enable/disable functions would be neat.
-Daniel
>
>
> > }
> > }
> > 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?
>
> > +
> > + 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.
>
> Otherwise looks all reasonable.
> -Daniel
>
>
> >
> > /* Shared lengths/masks between HDMI/DVI/DisplayPort */
> > #define DRM_HDCP_AN_LEN 8
> > --
> > 2.7.4
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
