[PATCH v3 08/12] drm/i915: Don't fully disable HDCP on a port if multiple pipes are using it
Sean Paul
sean at poorly.run
Tue Feb 18 22:07:50 UTC 2020
On Sun, Feb 2, 2020 at 2:03 PM Ramalingam C <ramalingam.c at intel.com> wrote:
>
> On 2020-01-17 at 14:30:59 -0500, Sean Paul wrote:
> > From: Sean Paul <seanpaul at chromium.org>
> >
> > This patch is required for HDCP over MST. If a port is being used for
> > multiple HDCP streams, we don't want to fully disable HDCP on a port if
> > one of them is disabled. Instead, we just disable the HDCP signalling on
> > that particular pipe and exit early. The last pipe to disable HDCP will
> > also bring down HDCP on the port.
> >
> > In order to achieve this, we need to keep a refcount in intel_digital_port
> > and protect it using a new hdcp_mutex.
>
> Sean,
>
> Sorry for late review.
> But got a question in my head. May be due to lag of MST understanding,
> which i will try to gain for this review. But still before my study of MST
> I am posting my question here.
>
> As you know, though hdcp state machine is associated to the connector at
> SW, HDCP engine is at DDI for <= Gen11 and for gen12+ it is at
> transcoder.
>
> So when a transcoder and DDI is paired(1:1) for normal DP, we configure the
> HDCP engine (either in DDI/Trancoder) from the connector->hdcp struct.
>
> In case of MST, with <=gen11, this series will take care of required
> changes as HDCP engine is at DDI and to which multiple transcoder's data
> streams are channelised.
>
> But with gen12+, on MST, single SW instance(connector->hdcp associated to
> the DDI used) need to enable the many HDCP engines situated at all
> transcoders, those feed the data streams to the single DDI ?
>
> Considering the MST addition, possibily we might need to revisit the
> HDCP support we have for gen12+.
>
> I am not seeing this scenario addressed in this series. Am I missing something?
Hey Ram,
To be completely honest, I have no clue. I don't have gen12 hardware
or bspec, so you're in a much better position to answer that than I
am.
As mentioned in a previous version, if someone wants to send me the
HDCP bspec for gen12, I'm happy to implement it. Otherwise, I suppose
that falls on Intel.
Sean
>
> -Ram
> >
> > Cc: Ramalingam C <ramalingam.c at intel.com>
> > Signed-off-by: Sean Paul <seanpaul at chromium.org>
> > Link: https://patchwork.freedesktop.org/patch/msgid/20191203173638.94919-8-sean@poorly.run #v1
> > Link: https://patchwork.freedesktop.org/patch/msgid/20191212190230.188505-9-sean@poorly.run #v2
> >
> > Changes in v2:
> > - Move the toggle_signalling call into _intel_hdcp_disable so it's called from check_work
> > Changes in v3:
> > - None
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 3 ++
> > .../drm/i915/display/intel_display_types.h | 5 ++
> > drivers/gpu/drm/i915/display/intel_dp.c | 2 +
> > drivers/gpu/drm/i915/display/intel_hdcp.c | 52 +++++++++++++++----
> > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +
> > 5 files changed, 55 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index a90ce672d2cf..4073dd96b83e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -4815,6 +4815,9 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
> >
> > encoder = &intel_dig_port->base;
> >
> > + mutex_init(&intel_dig_port->hdcp_mutex);
> > + intel_dig_port->num_hdcp_streams = 0;
> > +
> > drm_encoder_init(&dev_priv->drm, &encoder->base, &intel_ddi_funcs,
> > DRM_MODE_ENCODER_TMDS, "DDI %c", port_name(port));
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 8a96fedefad3..b488e8b1478e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1355,6 +1355,11 @@ struct intel_digital_port {
> > enum phy_fia tc_phy_fia;
> > u8 tc_phy_fia_idx;
> >
> > + /* protects num_hdcp_streams reference count */
> > + struct mutex hdcp_mutex;
> > + /* the number of pipes using HDCP signalling out of this port */
> > + unsigned int num_hdcp_streams;
> > +
> > void (*write_infoframe)(struct intel_encoder *encoder,
> > const struct intel_crtc_state *crtc_state,
> > unsigned int type,
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 89501b7525a9..a91f65ba35a1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -7583,6 +7583,8 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
> > intel_encoder = &intel_dig_port->base;
> > encoder = &intel_encoder->base;
> >
> > + mutex_init(&intel_dig_port->hdcp_mutex);
> > +
> > if (drm_encoder_init(&dev_priv->drm, &intel_encoder->base,
> > &intel_dp_enc_funcs, DRM_MODE_ENCODER_TMDS,
> > "DP %c", port_name(port)))
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index fabacfb1b644..dc83d81c6df0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -779,6 +779,19 @@ 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);
> >
> > + /*
> > + * If there are other connectors on this port using HDCP, don't disable
> > + * it. Instead, toggle the HDCP signalling off on that particular
> > + * connector/pipe and exit.
> > + */
> > + if (intel_dig_port->num_hdcp_streams > 0) {
> > + ret = hdcp->shim->toggle_signalling(intel_dig_port,
> > + cpu_transcoder, false);
> > + if (ret)
> > + DRM_ERROR("Failed to disable HDCP signalling\n");
> > + return ret;
> > + }
> > +
> > hdcp->hdcp_encrypted = false;
> > I915_WRITE(HDCP_CONF(dev_priv, cpu_transcoder, port), 0);
> > if (intel_de_wait_for_clear(dev_priv,
> > @@ -855,6 +868,7 @@ struct intel_connector *intel_hdcp_to_connector(struct intel_hdcp *hdcp)
> > static void intel_hdcp_update_value(struct intel_connector *connector,
> > u64 value, bool update_property)
> > {
> > + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > struct intel_hdcp *hdcp = &connector->hdcp;
> >
> > WARN_ON(!mutex_is_locked(&hdcp->mutex));
> > @@ -862,6 +876,15 @@ static void intel_hdcp_update_value(struct intel_connector *connector,
> > if (hdcp->value == value)
> > return;
> >
> > + WARN_ON(!mutex_is_locked(&intel_dig_port->hdcp_mutex));
> > +
> > + if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > + if (!WARN_ON(intel_dig_port->num_hdcp_streams == 0))
> > + intel_dig_port->num_hdcp_streams--;
> > + } else if (value == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
> > + intel_dig_port->num_hdcp_streams++;
> > + }
> > +
> > hdcp->value = value;
> > if (update_property) {
> > drm_connector_get(&connector->base);
> > @@ -880,6 +903,8 @@ static int intel_hdcp_check_link(struct intel_connector *connector)
> > int ret = 0;
> >
> > mutex_lock(&hdcp->mutex);
> > + mutex_lock(&intel_dig_port->hdcp_mutex);
> > +
> > cpu_transcoder = hdcp->cpu_transcoder;
> >
> > /* Check_link valid only when HDCP1.4 is enabled */
> > @@ -931,6 +956,7 @@ static int intel_hdcp_check_link(struct intel_connector *connector)
> > }
> >
> > out:
> > + mutex_unlock(&intel_dig_port->hdcp_mutex);
> > mutex_unlock(&hdcp->mutex);
> > return ret;
> > }
> > @@ -1996,6 +2022,7 @@ int intel_hdcp_enable(struct intel_connector *connector,
> > enum transcoder cpu_transcoder, u8 content_type)
> > {
> > struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > struct intel_hdcp *hdcp = &connector->hdcp;
> > unsigned long check_link_interval = DRM_HDCP_CHECK_PERIOD_MS;
> > int ret = -EINVAL;
> > @@ -2004,6 +2031,7 @@ int intel_hdcp_enable(struct intel_connector *connector,
> > return -ENOENT;
> >
> > mutex_lock(&hdcp->mutex);
> > + mutex_lock(&intel_dig_port->hdcp_mutex);
> > WARN_ON(hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED);
> > hdcp->content_type = content_type;
> >
> > @@ -2038,12 +2066,14 @@ int intel_hdcp_enable(struct intel_connector *connector,
> > true);
> > }
> >
> > + mutex_unlock(&intel_dig_port->hdcp_mutex);
> > mutex_unlock(&hdcp->mutex);
> > return ret;
> > }
> >
> > int intel_hdcp_disable(struct intel_connector *connector)
> > {
> > + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > struct intel_hdcp *hdcp = &connector->hdcp;
> > int ret = 0;
> >
> > @@ -2051,17 +2081,21 @@ int intel_hdcp_disable(struct intel_connector *connector)
> > return -ENOENT;
> >
> > mutex_lock(&hdcp->mutex);
> > + mutex_lock(&intel_dig_port->hdcp_mutex);
> >
> > - if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > - intel_hdcp_update_value(connector,
> > - DRM_MODE_CONTENT_PROTECTION_UNDESIRED,
> > - false);
> > - if (hdcp->hdcp2_encrypted)
> > - ret = _intel_hdcp2_disable(connector);
> > - else if (hdcp->hdcp_encrypted)
> > - ret = _intel_hdcp_disable(connector);
> > - }
> > + if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)
> > + goto out;
> > +
> > + intel_hdcp_update_value(connector,
> > + DRM_MODE_CONTENT_PROTECTION_UNDESIRED, false);
> >
> > + if (hdcp->hdcp2_encrypted)
> > + ret = _intel_hdcp2_disable(connector);
> > + else if (hdcp->hdcp_encrypted)
> > + ret = _intel_hdcp_disable(connector);
> > +
> > +out:
> > + mutex_unlock(&intel_dig_port->hdcp_mutex);
> > mutex_unlock(&hdcp->mutex);
> > cancel_delayed_work_sync(&hdcp->check_work);
> > return ret;
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index 6b28c00d7289..68b648ec6abb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -3252,6 +3252,8 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv,
> >
> > intel_encoder = &intel_dig_port->base;
> >
> > + mutex_init(&intel_dig_port->hdcp_mutex);
> > +
> > drm_encoder_init(&dev_priv->drm, &intel_encoder->base,
> > &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS,
> > "HDMI %c", port_name(port));
> > --
> > Sean Paul, Software Engineer, Google / Chromium OS
> >
More information about the dri-devel
mailing list