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 <ramalinga...@intel.com>
> > > ---
> > >   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to