| Message ID | 1526907240-17639-26-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > --- > 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 >
On Thursday 31 May 2018 12:39 PM, Daniel Vetter wrote: > 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@intel.com> >> --- >> 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. Sure. I will move these HW status check into the WARN_ONs. -Ram. > -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 >>
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; + + 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) { + + /* 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; +}
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@intel.com> --- drivers/gpu/drm/i915/intel_hdcp.c | 57 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)