[PATCH v9 08/39] drm/i915: hdcp1.4 CP_IRQ handling and SW encryption tracking
Daniel Vetter
daniel at ffwll.ch
Thu Dec 20 14:53:20 UTC 2018
On Thu, Dec 20, 2018 at 04:59:13PM +0530, C, Ramalingam wrote:
>
> On 12/19/2018 9:18 PM, Daniel Vetter wrote:
> > On Thu, Dec 13, 2018 at 09:31:10AM +0530, Ramalingam C wrote:
> > > "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 <ramalingam.c at intel.com>
> > > ---
> > > 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;
> > Another bool I guess? Or unsigned : 1;
> as per #intel-gfx discussion I will fallback to bool.
> >
> > > +
> > > /* 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);
> > > -}
> > Why move the function?
>
> 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.
Yeah, but moving the function down is never required to avoid a forward
declaration. If you want to move it for better grouping that makes sense,
but as-is it was confusing.
> > > -
> > > 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))
> > 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.
>
> 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
>
> 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.
Yeah that was my point: If the work is already running then re-scheduling
it won't cause additional work. Whereas your mutex_trylock trick above
sets of all kinds of alarm bell about races and causing issues when we
_do_ want to handle the CP_IRQ.
If we do need this optimization, then we need to rework the CP_IRQ
handling to do check the interrupt sources before scheduling the work,
which is going to be complicated because locking. And then filter out the
right interrupt sources, and not use something which is only correct in
99% of all cases like mutex_trylock.
We've had other such code in drm/i915 abusing mutex_trylock, works great
in a quick test locally, fails in production, so I don't like that trick
:-)
> > 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.
> > > + return;
> > > +
> > > + if (!hdcp->hdcp_encrypted)
> > > + return;
> > This check would need to be moved intel_hdcp_check_link().
>
> Function has the check already. I can avoid the check here and directly land
> into check_link work fn.
Yeah that's what I meant.
-Daniel
>
> -Ram
>
> > > +
> > > + mutex_unlock(&hdcp->mutex);
> > > + _intel_hdcp_check_work(connector);
> > 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.
> > -Daniel
> > > +}
> > > --
> > > 2.7.4
> > >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list