On 2020-08-12 at 13:21:51 -0400, Sean Paul wrote:
> On Wed, Aug 12, 2020 at 3:15 AM Anshuman Gupta <anshuman.gu...@intel.com>
> wrote:
> >
> > On 2020-08-11 at 13:28:46 -0400, Sean Paul wrote:
> > > On Thu, Jul 9, 2020 at 8:40 AM Anshuman Gupta <anshuman.gu...@intel.com>
> > > wrote:
> > > >
> > >
> > > \snip
> > >
> > > > > > +static int
> > > > > > +intel_dp_mst_hdcp_toggle_signalling(struct intel_digital_port
> > > > > > *intel_dig_port,
> > > > > > + enum transcoder cpu_transcoder,
> > > > > > + bool enable)
> > > > > > +{
> > > > > > + struct drm_i915_private *i915 =
> > > > > > to_i915(intel_dig_port->base.base.dev);
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (!enable)
> > > > > > + usleep_range(6, 60); /* Bspec says >= 6us */
> > > > > > +
> > > > > > + ret = intel_ddi_toggle_hdcp_signalling(&intel_dig_port->base,
> > > > > > + cpu_transcoder, enable);
> > > > > Sean,
> > > > >
> > > > > This function toggles the TRANS_DDI_HDCP_SIGNALLING (9th)bit of
> > > > > TRANS_DDI_FUNC_CTL(tran)
> > > > > But in the hw specification this bit is mentioned to be ignored for
> > > > > non
> > > > > HDMI/DVI modes of the TRANS DDI.
> > > > >
> > > > > Any reason why we need this? Did you try with out this function?
> > > > >
> > >
> > > Under "Authentication Part 1 for Multi-stream DisplayPort", bspec says:
> > > 2. Select HDCP for the desired stream using the Pipe DDI Function
> > > Control register.
> > This is the 5th bit (Multistream HDCP Select) of Pipe DDI Function Control
> > register i.e
> > TRANS_DDI_FUNC_CTL register. This bit ensures HDCP encryption to this
> > transcoder stream when used in
> > multistream DisplayPort mode.
> > Unfortunately public specs of Gen11 has discrepency and doesn't describe
> > this bit.
> > https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-icllp-vol02c-commandreference-registers-part2_0.pdf
> > Page No.1026 TRANS_DDI_FUNC_CTL
> > >
> > > > > Apart from that Patch looks good to me.
> > > > IMHO it seems we are still missing to enable the Multistream HDCP Select
> > > > bit (5) in TRANS_DDI_FUNC_CTL register which is required to enable the
> > > > stream encryption.
> > > >
> > >
> > > Could you send me some docs on this? I don't see have info on this bit.
> > This bit is not described in above mentioned public spec, but neither bit
> > TRANS_DDI_HDCP_SIGNALLING bit (9th)
> > which used in this patch, what is the source of public B.spec you are
> > following?
>
> I think it's been removed from public now, but there was a public doc
> on Intel's site when I originally wrote these which described the
> TRANS_DDI_FUNC_CTL register.
>
> So do I just need to set bit 5 at the same time as the HDCP SIGNALLING bit?
Yes that should be the ideal way with respect to B.Specs Documentation but
as you had already tested it and stream level encryption is confirmed via
QUERY_STREAM_ENCRYPTION_STATUS sideband message, considering that it is really
confusing
whether HDCP select bit required to be set for each stream.
I feel based upon your test results this patch should be good to be merged.
if any iterative development will be required, it can be pursued later.
Either way,
Reviewed-by: Anshuman Gupta <anshuman.gu...@intel.com>
>
> Sean
>
> > Thanks,
> > Anshuman Gupta.
> > >
> > > Sean
> > >
> > > > Thanks,
> > > > Anshuman Gupta.
> > > > >
> > > > > -Ram
> > > > >
> > > > > > + if (ret)
> > > > > > + drm_dbg_kms(&i915->drm, "%s HDCP signalling failed
> > > > > > (%d)\n",
> > > > > > + enable ? "Enable" : "Disable", ret);
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static
> > > > > > +bool intel_dp_mst_hdcp_check_link(struct intel_digital_port
> > > > > > *intel_dig_port,
> > > > > > + struct intel_connector *connector)
> > > > > > +{
> > > > > > + struct drm_i915_private *i915 =
> > > > > > to_i915(intel_dig_port->base.base.dev);
> > > > > > + struct intel_dp *intel_dp = &intel_dig_port->dp;
> > > > > > + struct drm_dp_query_stream_enc_status_ack_reply reply;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (!intel_dp_hdcp_check_link(intel_dig_port, connector))
> > > > > > + return false;
> > > > > > +
> > > > > > + ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr,
> > > > > > + connector->port,
> > > > > > &reply);
> > > > > > + if (ret) {
> > > > > > + drm_dbg_kms(&i915->drm,
> > > > > > + "[CONNECTOR:%d:%s] failed QSES ret=%d\n",
> > > > > > + connector->base.base.id,
> > > > > > connector->base.name, ret);
> > > > > > + return false;
> > > > > > + }
> > > > > > +
> > > > > > + return reply.auth_completed && reply.encryption_enabled;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct intel_hdcp_shim intel_dp_mst_hdcp_shim = {
> > > > > > + .write_an_aksv = intel_dp_hdcp_write_an_aksv,
> > > > > > + .read_bksv = intel_dp_hdcp_read_bksv,
> > > > > > + .read_bstatus = intel_dp_hdcp_read_bstatus,
> > > > > > + .repeater_present = intel_dp_hdcp_repeater_present,
> > > > > > + .read_ri_prime = intel_dp_hdcp_read_ri_prime,
> > > > > > + .read_ksv_ready = intel_dp_hdcp_read_ksv_ready,
> > > > > > + .read_ksv_fifo = intel_dp_hdcp_read_ksv_fifo,
> > > > > > + .read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> > > > > > + .toggle_signalling = intel_dp_mst_hdcp_toggle_signalling,
> > > > > > + .check_link = intel_dp_mst_hdcp_check_link,
> > > > > > + .hdcp_capable = intel_dp_hdcp_capable,
> > > > > > +
> > > > > > + .protocol = HDCP_PROTOCOL_DP,
> > > > > > +};
> > > > > > +
> > > > > > int intel_dp_init_hdcp(struct intel_digital_port *intel_dig_port,
> > > > > > struct intel_connector *intel_connector)
> > > > > > {
> > > > > > @@ -630,7 +691,10 @@ int intel_dp_init_hdcp(struct
> > > > > > intel_digital_port *intel_dig_port,
> > > > > > if (!is_hdcp_supported(dev_priv, port))
> > > > > > return 0;
> > > > > >
> > > > > > - if (!intel_dp_is_edp(intel_dp))
> > > > > > + if (intel_connector->mst_port)
> > > > > > + return intel_hdcp_init(intel_connector, port,
> > > > > > + &intel_dp_mst_hdcp_shim);
> > > > > > + else if (!intel_dp_is_edp(intel_dp))
> > > > > > return intel_hdcp_init(intel_connector, port,
> > > > > > &intel_dp_hdcp_shim);
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > index 0675825dcc20..abaaeeb963d2 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > > > @@ -37,6 +37,7 @@
> > > > > > #include "intel_dp.h"
> > > > > > #include "intel_dp_mst.h"
> > > > > > #include "intel_dpio_phy.h"
> > > > > > +#include "intel_hdcp.h"
> > > > > >
> > > > > > static int intel_dp_mst_compute_link_config(struct intel_encoder
> > > > > > *encoder,
> > > > > > struct intel_crtc_state
> > > > > > *crtc_state,
> > > > > > @@ -352,6 +353,8 @@ static void intel_mst_disable_dp(struct
> > > > > > intel_atomic_state *state,
> > > > > > drm_dbg_kms(&i915->drm, "active links %d\n",
> > > > > > intel_dp->active_mst_links);
> > > > > >
> > > > > > + intel_hdcp_disable(intel_mst->connector);
> > > > > > +
> > > > > > drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr,
> > > > > > connector->port);
> > > > > >
> > > > > > ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> > > > > > @@ -548,6 +551,13 @@ static void intel_mst_enable_dp(struct
> > > > > > intel_atomic_state *state,
> > > > > >
> > > > > > if (pipe_config->has_audio)
> > > > > > intel_audio_codec_enable(encoder, pipe_config,
> > > > > > conn_state);
> > > > > > +
> > > > > > + /* Enable hdcp if it's desired */
> > > > > > + if (conn_state->content_protection ==
> > > > > > + DRM_MODE_CONTENT_PROTECTION_DESIRED)
> > > > > > +
> > > > > > intel_hdcp_enable(to_intel_connector(conn_state->connector),
> > > > > > + pipe_config->cpu_transcoder,
> > > > > > + (u8)conn_state->hdcp_content_type);
> > > > > > }
> > > > > >
> > > > > > static bool intel_dp_mst_enc_get_hw_state(struct intel_encoder
> > > > > > *encoder,
> > > > > > @@ -770,6 +780,14 @@ static struct drm_connector
> > > > > > *intel_dp_add_mst_connector(struct drm_dp_mst_topolo
> > > > > > intel_attach_force_audio_property(connector);
> > > > > > intel_attach_broadcast_rgb_property(connector);
> > > > > >
> > > > > > +
> > > > > > + /* TODO: Figure out how to make HDCP work on GEN12+ */
> > > > > > + if (INTEL_GEN(dev_priv) < 12) {
> > > > > > + ret = intel_dp_init_hdcp(intel_dig_port,
> > > > > > intel_connector);
> > > > > > + if (ret)
> > > > > > + DRM_DEBUG_KMS("HDCP init failed, skipping.\n");
> > > > > > + }
> > > > > > +
> > > > > > /*
> > > > > > * Reuse the prop from the SST connector because we're
> > > > > > * not allowed to create new props after device registration.
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > > > index 6bd0e4616ee1..ddc9db8de2bc 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > > > > > @@ -2060,7 +2060,7 @@ int intel_hdcp_init(struct intel_connector
> > > > > > *connector,
> > > > > > if (!shim)
> > > > > > return -EINVAL;
> > > > > >
> > > > > > - if (is_hdcp2_supported(dev_priv))
> > > > > > + if (is_hdcp2_supported(dev_priv) && !connector->mst_port)
> > > > > > intel_hdcp2_init(connector, port, shim);
> > > > > >
> > > > > > ret =
> > > > > > --
> > > > > > Sean Paul, Software Engineer, Google / Chromium OS
> > > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
