<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 12/19/2018 9:18 PM, Daniel Vetter wrote: </div> <blockquote type="cite" cite="mid:20181219154800.GP21184@phenom.ffwll.local"> <pre class="moz-quote-pre" wrap="">On Thu, Dec 13, 2018 at 09:31:10AM +0530, Ramalingam C wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">"hdcp_encrypted" flag is defined to denote the HDCP1.4 encryption status. This SW tracking is used to determine the need for real hdcp1.4 disable and hdcp_check_link upon CP_IRQ. On CP_IRQ we filter the CP_IRQ related to the states like Link failure and reauthentication req etc and handle them in hdcp_check_link. CP_IRQ corresponding to the authentication msg availability are ignored. WARN_ON is added for the abrupt stop of HDCP encryption of a port. Signed-off-by: Ramalingam C <a class="moz-txt-link-rfc2396E" href="mailto:ramalingam.c@intel.com"><ramalingam.c@intel.com></a> --- drivers/gpu/drm/i915/intel_dp.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 5 ++- drivers/gpu/drm/i915/intel_hdcp.c | 89 ++++++++++++++++++++++++++++++--------- 3 files changed, 74 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index aba884c64879..89315e15fb34 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4758,7 +4758,7 @@ static void intel_dp_check_service_irq(struct intel_dp *intel_dp) intel_dp_handle_test_request(intel_dp); if (val & DP_CP_IRQ) - intel_hdcp_check_link(intel_dp->attached_connector); + intel_hdcp_handle_cp_irq(intel_dp->attached_connector); if (val & DP_SINK_SPECIFIC_IRQ) DRM_DEBUG_DRIVER("Sink specific irq unhandled\n"); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 191b6e0f086c..decd0346c6a7 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -393,6 +393,9 @@ struct intel_hdcp { struct delayed_work check_work; struct work_struct prop_work; + /* HDCP1.4 Encryption status */ + u8 hdcp_encrypted; </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Another bool I guess? Or unsigned : 1;</pre> </blockquote> as per #intel-gfx discussion I will fallback to bool. <blockquote type="cite" cite="mid:20181219154800.GP21184@phenom.ffwll.local"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ /* HDCP2.2 related definitions */ /* Flag indicates whether this connector supports HDCP2.2 or not. */ u8 hdcp2_supported; @@ -2058,10 +2061,10 @@ int intel_hdcp_init(struct intel_connector *connector, bool hdcp2_supported); int intel_hdcp_enable(struct intel_connector *connector); int intel_hdcp_disable(struct intel_connector *connector); -int intel_hdcp_check_link(struct intel_connector *connector); bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port); bool intel_hdcp_capable(struct intel_connector *connector); bool is_hdcp2_supported(struct drm_i915_private *dev_priv); +void intel_hdcp_handle_cp_irq(struct intel_connector *connector); /* intel_psr.c */ #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support) diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 9405fce07b93..2b7814a6f12b 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -75,6 +75,16 @@ bool intel_hdcp_capable(struct intel_connector *connector) return capable; } +static inline bool intel_hdcp_in_use(struct intel_connector *connector) +{ + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); + enum port port = connector->encoder->port; + u32 reg; + + reg = I915_READ(PORT_HDCP_STATUS(port)); + return reg & HDCP_STATUS_ENC; +} + static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, const struct intel_hdcp_shim *shim) { @@ -669,6 +679,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector) DRM_DEBUG_KMS("[%s:%d] HDCP is being disabled...\n", connector->base.name, connector->base.base.id); + hdcp->hdcp_encrypted = 0; I915_WRITE(PORT_HDCP_CONF(port), 0); if (intel_wait_for_register(dev_priv, PORT_HDCP_STATUS(port), ~0, 0, ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) { @@ -714,8 +725,10 @@ static int _intel_hdcp_enable(struct intel_connector *connector) /* Incase of authentication failures, HDCP spec expects reauth. */ for (i = 0; i < tries; i++) { ret = intel_hdcp_auth(conn_to_dig_port(connector), hdcp->shim); - if (!ret) + if (!ret) { + hdcp->hdcp_encrypted = 1; return 0; + } DRM_DEBUG_KMS("HDCP Auth failure (%d)\n", ret); @@ -742,16 +755,17 @@ int intel_hdcp_check_link(struct intel_connector *connector) enum port port = intel_dig_port->base.port; int ret = 0; - if (!hdcp->shim) - return -ENOENT; - mutex_lock(&hdcp->mutex); - if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) + /* Check_link valid only when HDCP1.4 is enabled */ + if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED || + !hdcp->hdcp_encrypted) { + ret = -EINVAL; goto out; + } - if (!(I915_READ(PORT_HDCP_STATUS(port)) & HDCP_STATUS_ENC)) { - DRM_ERROR("%s:%d HDCP check failed: link is not encrypted,%x\n", + if (WARN_ON(!intel_hdcp_in_use(connector))) { + DRM_ERROR("%s:%d HDCP link stopped encryption,%x\n", connector->base.name, connector->base.base.id, I915_READ(PORT_HDCP_STATUS(port))); ret = -ENXIO; @@ -792,18 +806,6 @@ int intel_hdcp_check_link(struct intel_connector *connector) return ret; } -static void intel_hdcp_check_work(struct work_struct *work) -{ - struct intel_hdcp *hdcp = container_of(to_delayed_work(work), - struct intel_hdcp, - check_work); - struct intel_connector *connector = intel_hdcp_to_connector(hdcp); - - if (!intel_hdcp_check_link(connector)) - schedule_delayed_work(&hdcp->check_work, - DRM_HDCP_CHECK_PERIOD_MS); -} </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Why move the function?</pre> </blockquote> <pre>This will be used as common work fn for both 1.4 and 2.2. hdcp2_check_link will be defined below. So moving this to the group of common functions. Basically to avoid the function declarations.</pre> <blockquote type="cite" cite="mid:20181219154800.GP21184@phenom.ffwll.local"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">- static void intel_hdcp_prop_work(struct work_struct *work) { struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp, @@ -1028,6 +1030,25 @@ int hdcp2_deauthenticate_port(struct intel_connector *connector) return hdcp2_close_mei_session(connector); } +static inline void _intel_hdcp_check_work(struct intel_connector *connector) +{ + struct intel_hdcp *hdcp = &connector->hdcp; + + if (!intel_hdcp_check_link(connector)) + schedule_delayed_work(&hdcp->check_work, + DRM_HDCP_CHECK_PERIOD_MS); +} + +static void intel_hdcp_check_work(struct work_struct *work) +{ + struct intel_hdcp *hdcp = container_of(to_delayed_work(work), + struct intel_hdcp, + check_work); + struct intel_connector *connector = intel_hdcp_to_connector(hdcp); + + _intel_hdcp_check_work(connector); +} + static int i915_hdcp_component_match(struct device *dev, void *data) { return !strcmp(dev->driver->name, "mei_hdcp"); @@ -1156,7 +1177,8 @@ int intel_hdcp_disable(struct intel_connector *connector) if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { hdcp->value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED; - ret = _intel_hdcp_disable(connector); + if (hdcp->hdcp_encrypted) + ret = _intel_hdcp_disable(connector); } mutex_unlock(&hdcp->mutex); @@ -1196,3 +1218,30 @@ void intel_hdcp_atomic_check(struct drm_connector *connector, new_state->crtc); crtc_state->mode_changed = true; } + +/* Handles the CP_IRQ raised from the DP HDCP sink */ +void intel_hdcp_handle_cp_irq(struct intel_connector *connector) +{ + struct intel_hdcp *hdcp = &connector->hdcp; + + if (!hdcp->shim) + return; + + /* + * As of now we want to handle CP_IRQ triggered while encryption is + * enabled. This includes the notification for Topology change, + * Link failure, reauth request etc. + * Auth msg availability notifications are ignored with mutex_trylock + * as authentication holds the lock. Lock might be taken for link check + * or hdcp_disable too. In either of state we need not handle the + * CP_IRQ, for known reasons. + */ + if (!mutex_trylock(&hdcp->mutex)) </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> This is not a good way to fix recursions, because the lock might be held by someone else for other reasons, and then you just drop events on the floor. Worse, usually it works, except when it stops working.</pre> </blockquote> This is attempt to drop the CP_IRQ received during mutex is locked. Because of the below reasoning: This mutex is taken by authentication enable/disable and check_link worker <pre>when enable is in process this CP_IRQ will be notifying the msg availability to read. at present we dont use the CP_IRQ for msg read. when disable is in process it doesn't matter why this CP_IRQ is triggered, we just want to ignore. When check_link is in progress, that means hdcp is enabled. So this CP_IRQ is for topology change/reauth request etc, which are handled by check_link. so once again we dont care for this CP_IRQs too. So basically if the mutex is already taken we can ignore/drop the CP_IRQ. So mutex_trylock helps us to do this. May be I should add all of these in the comment!? While rethinking on that we can reschedule the check_link work on every CP_IRQ. As this is harmless i will do that. Makes things simple. </pre> <blockquote type="cite" cite="mid:20181219154800.GP21184@phenom.ffwll.local"> <pre class="moz-quote-pre" wrap=""> Since you already have a separate worker just schedule that one, and leave it up to the worker to filter out events we don't care about. </pre> </blockquote> <blockquote type="cite" cite="mid:20181219154800.GP21184@phenom.ffwll.local"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ return; + + if (!hdcp->hdcp_encrypted) + return; </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> This check would need to be moved intel_hdcp_check_link().</pre> </blockquote> Function has the check already. I can avoid the check here and directly land into check_link work fn. -Ram <blockquote type="cite" cite="mid:20181219154800.GP21184@phenom.ffwll.local"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+ + mutex_unlock(&hdcp->mutex); + _intel_hdcp_check_work(connector); </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> So instead of the direct call we need a schedule_work() here (without delay ofc). Note that if the work is already scheduled it won't be scheduled to run twice, so should just work as-is I think.</pre> </blockquote> <blockquote type="cite" cite="mid:20181219154800.GP21184@phenom.ffwll.local"> <pre class="moz-quote-pre" wrap=""> -Daniel </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+} -- 2.7.4 </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> </body> </html>