On Fri, Feb 02, 2018 at 08:08:44PM +0530, Ramalingam C wrote:
>
>
> On Friday 02 February 2018 07:54 PM, Sean Paul wrote:
> > On Fri, Feb 02, 2018 at 04:15:18PM +0530, Ramalingam C wrote:
> > > As a first step of HDCP authentication detects the panel's HDCP
> > > capability. This is mandated for DP HDCP1.4.
> > >
> > > For DP 0th Bit of Bcaps register indicates the panel's hdcp capability
> > > For HDMI valid BKSV indicates the panel's hdcp capability.
> > We're already checking valid bksv, so there's no need to expose that function and
> > call it twice for the HDMI case, hdcp_capable should just always be true.
> we can make the hdmi's hdcp_capable as dummy. As hdcp1.4 spec for hdmi says
> panel's capability is determined by valid bksv.
> Which we are already doing it. no sequence mandated.
> >
> > For DP, we read and check the bksv , so what do non-capable displays return for
> > the bksv?
> But for DP HDCP1.4 spec mandates that An write should entertained only after
> verifying the 0th bit(hdcp_capable) of BCSPS register.
> Else all DP compliance tests will fail :(
>
> We are not addressing any functional issue but deviation from the HDCP1.4
> spec for DP.
Hmm, that's lame. Could you please add this to the commit message so that it's
clear that BCAPS must be checked before An is written?
>
> --Ram
> >
> > Sean
> >
> > > Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 19 +++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_drv.h | 5 +++++
> > > drivers/gpu/drm/i915/intel_hdcp.c | 10 ++++++++--
> > > drivers/gpu/drm/i915/intel_hdmi.c | 17 +++++++++++++++++
> > > 4 files changed, 49 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 03d86ff9b805..2623b2beda1a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -5218,6 +5218,24 @@ bool intel_dp_hdcp_check_link(struct intel_digital_port *intel_dig_port)
> > > return !(bstatus & (DP_BSTATUS_LINK_FAILURE | DP_BSTATUS_REAUTH_REQ));
> > > }
> > > +static
> > > +int intel_dp_hdcp_capable(struct intel_digital_port *intel_dig_port,
> > > + bool *hdcp_capable)
> > > +{
> > > + ssize_t ret;
> > > + u8 bcaps;
> > > +
> > > + ret = drm_dp_dpcd_read(&intel_dig_port->dp.aux, DP_AUX_HDCP_BCAPS,
> > > + &bcaps, 1);
> > > + if (ret != 1) {
> > > + DRM_ERROR("Read bcaps from DP/AUX failed (%zd)\n", ret);
> > > + return ret >= 0 ? -EIO : ret;
> > > + }
This is duplicated in repeater_present. Please pull it out into a helper
function.
> > > +
> > > + *hdcp_capable = bcaps & DP_BCAPS_HDCP_CAPABLE;
> > > + return 0;
> > > +}
> > > +
> > > static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
> > > .write_an_aksv = intel_dp_hdcp_write_an_aksv,
> > > .read_bksv = intel_dp_hdcp_read_bksv,
> > > @@ -5229,6 +5247,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
> > > .read_v_prime_part = intel_dp_hdcp_read_v_prime_part,
> > > .toggle_signalling = intel_dp_hdcp_toggle_signalling,
> > > .check_link = intel_dp_hdcp_check_link,
> > > + .hdcp_capable = intel_dp_hdcp_capable,
> > > };
> > > static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index d6a808374dfb..409aee9010ba 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -369,6 +369,10 @@ struct intel_hdcp_shim {
> > > /* Ensures the link is still protected */
> > > bool (*check_link)(struct intel_digital_port *intel_dig_port);
> > > +
> > > + /* Detects panel's hdcp capablility */
s/capablility/capability/
Also mention that this is optional for HDMI.
> > > + int (*hdcp_capable)(struct intel_digital_port *intel_dig_port,
> > > + bool *hdcp_capable);
> > > };
> > > struct intel_connector {
> > > @@ -1848,6 +1852,7 @@ 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_is_ksv_valid(u8 *ksv);
> > > /* 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 5de9afd275b2..a3463557f0f6 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdcp.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c
> > > @@ -132,7 +132,6 @@ u32 intel_hdcp_get_repeater_ctl(struct intel_digital_port *intel_dig_port)
> > > return -EINVAL;
> > > }
> > > -static
> > > bool intel_hdcp_is_ksv_valid(u8 *ksv)
> > > {
> > > int i, ones = 0;
> > > @@ -418,13 +417,20 @@ static int intel_hdcp_auth(struct intel_connector *connector)
> > > u32 reg;
> > > u8 shim[DRM_HDCP_RI_LEN];
> > > } ri;
> > > - bool repeater_present;
> > > + bool repeater_present, hdcp_capable;
> > > const struct intel_hdcp_shim *shim = connector->hdcp_shim;
> > > struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector);
> > > dev_priv = to_i915(connector->base.dev);
> > > port = intel_dig_port->base.port;
> > > + /* Detect whether panel is HDCP capable or not */
Please change this to the following so it's very clear why this is required:
/*
* Detects whether the display is HDCP capable. Although we check for
* valid Bksv below, the HDCP over DP spec requires that we check
* whether the display supports HDCP before we write Aksv. For HDMI
* displays, this is not necessary.
*/
> > > + ret = shim->hdcp_capable(intel_dig_port, &hdcp_capable);
Check if hdcp_capable is NULL before calling this, that way you don't need to
stub it out for hdmi
> > > + if (ret)
> > > + return ret;
> > > + if (!hdcp_capable)
> > > + return -EINVAL;
Please print something here so it's obvious why HDCP auth did not succeed.
Sean
> > > +
> > > /* Initialize An with 2 random values and acquire it */
> > > for (i = 0; i < 2; i++)
> > > I915_WRITE(PORT_HDCP_ANINIT(port), get_random_u32());
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index f5d7bfb43006..dfca361ebb24 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1106,6 +1106,22 @@ bool intel_hdmi_hdcp_check_link(struct intel_digital_port *intel_dig_port)
> > > return true;
> > > }
> > > +static
> > > +int intel_hdmi_hdcp_capable(struct intel_digital_port *intel_dig_port,
> > > + bool *hdcp_capable)
> > > +{
> > > + u8 bksv[5];
> > > + int ret, retry = 1;
> > > +
> > > + do {
> > > + ret = intel_hdmi_hdcp_read_bksv(intel_dig_port, bksv);
> > > + if (!ret)
> > > + *hdcp_capable = intel_hdcp_is_ksv_valid(bksv);
> > > + } while (!(*hdcp_capable) && retry--);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
> > > .write_an_aksv = intel_hdmi_hdcp_write_an_aksv,
> > > .read_bksv = intel_hdmi_hdcp_read_bksv,
> > > @@ -1117,6 +1133,7 @@ static const struct intel_hdcp_shim intel_hdmi_hdcp_shim = {
> > > .read_v_prime_part = intel_hdmi_hdcp_read_v_prime_part,
> > > .toggle_signalling = intel_hdmi_hdcp_toggle_signalling,
> > > .check_link = intel_hdmi_hdcp_check_link,
> > > + .hdcp_capable = intel_hdmi_hdcp_capable,
> > > };
> > > static void intel_hdmi_prepare(struct intel_encoder *encoder,
> > > --
> > > 2.7.4
> > >
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
