On Mon, May 21, 2018 at 06:23:44PM +0530, Ramalingam C wrote:
> Implements the enable and disable functions for HDCP2.2 encryption
> of the PORT.
>
> v2:
> intel_wait_for_register is used instead of wait_for. [Chris Wilson]
> v3:
> No Changes.
> v4:
> Debug msg is added for timeout at Disable of Encryption [Uma]
> %s/HDCP2_CTL/HDCP2_CTL
>
> Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_hdcp.c | 57 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c
> index bd0bfcfd5b8f..0386a67c3e32 100644
> --- a/drivers/gpu/drm/i915/intel_hdcp.c
> +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> @@ -19,6 +19,7 @@
> (enum hdcp_physical_port) (port))
> #define KEY_LOAD_TRIES 5
> #define HDCP2_LC_RETRY_CNT 3
> +#define TIME_FOR_ENCRYPT_STATUS_CHANGE 32
>
> static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port,
> const struct intel_hdcp_shim *shim)
> @@ -1397,3 +1398,59 @@ static int hdcp2_authenticate_sink(struct intel_connector *connector)
>
> return ret;
> }
> +
> +static int hdcp2_enable_encryption(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;
> +
> + if (I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS)
> + return 0;
Relying on hw status for state decisions is in my experience bad - it
papers over bugs in our state handling. We should know what the expected
state of the hardware is.
If you want to write a bit more robust code, then keep these checks, but
wrap them in a WARN_ON. That way we'll know if there's a regression in the
code (e.g. unbalanced enable/disable) right away, instead of these checks
silently papering over them.
> +
> + if (hdcp->hdcp_shim->toggle_signalling)
> + hdcp->hdcp_shim->toggle_signalling(intel_dig_port, true);
> +
> + if (I915_READ(HDCP2_STATUS_DDI(port)) & LINK_AUTH_STATUS) {
Same for this check here. In general control flow that depends upon
register values which can change at runtime is very suspect from a design
robustness point of view.
-Daniel
> +
> + /* Link is Authenticated. Now set for Encryption */
> + I915_WRITE(HDCP2_CTL_DDI(port),
> + I915_READ(HDCP2_CTL_DDI(port)) |
> + CTL_LINK_ENCRYPTION_REQ);
> + }
> +
> + ret = intel_wait_for_register(dev_priv, HDCP2_STATUS_DDI(port),
> + LINK_ENCRYPTION_STATUS,
> + LINK_ENCRYPTION_STATUS,
> + TIME_FOR_ENCRYPT_STATUS_CHANGE);
> +
> + return ret;
> +}
> +
> +static int hdcp2_disable_encryption(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;
> +
> + if (!(I915_READ(HDCP2_STATUS_DDI(port)) & LINK_ENCRYPTION_STATUS))
> + return 0;
> +
> + I915_WRITE(HDCP2_CTL_DDI(port),
> + I915_READ(HDCP2_CTL_DDI(port)) & ~CTL_LINK_ENCRYPTION_REQ);
> +
> + ret = intel_wait_for_register(dev_priv, HDCP2_STATUS_DDI(port),
> + LINK_ENCRYPTION_STATUS, 0x0,
> + TIME_FOR_ENCRYPT_STATUS_CHANGE);
> + if (ret == -ETIMEDOUT)
> + DRM_DEBUG_KMS("Disable Encryption Timedout");
> +
> + if (hdcp->hdcp_shim->toggle_signalling)
> + hdcp->hdcp_shim->toggle_signalling(intel_dig_port, false);
> +
> + return ret;
> +}
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
