| Message ID | 1517568320-15579-2-git-send-email-ramalingam.c@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote: > We enable the HDCP encryption as a part of first stage authentication. > So when second stage authentication fails, we need to disable the HDCP > encryption and signalling. > > This patch handles above requirement. > > For reusing the _intel_hdcp_disable from generic authentication flow, > this patch retain the reference to connector till the generic hdcp flow. > This will be handy to provide the connector details in HDCP enable > debug info. > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++---------------- > 1 file changed, 24 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c > index b97184eccd9c..0021511ae4d7 100644 > --- a/drivers/gpu/drm/i915/intel_hdcp.c > +++ b/drivers/gpu/drm/i915/intel_hdcp.c > @@ -16,6 +16,8 @@ > > #define KEY_LOAD_TRIES 5 > > +static int _intel_hdcp_disable(struct intel_connector *connector); > + > static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, > const struct intel_hdcp_shim *shim) > { > @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) > return true; > } > > +static > +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) > +{ > + return enc_to_dig_port(&connector->encoder->base); > +} > + > /* Implements Part 2 of the HDCP authorization procedure */ > static > -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, > - const struct intel_hdcp_shim *shim) > +int intel_hdcp_auth_downstream(struct intel_connector *connector) > { > struct drm_i915_private *dev_priv; > u32 vprime, sha_text, sha_leftovers, rep_ctl; > u8 bstatus[2], num_downstream, *ksv_fifo; > int ret, i, j, sha_idx; > + const struct intel_hdcp_shim *shim = connector->hdcp_shim; > + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); > > - dev_priv = intel_dig_port->base.base.dev->dev_private; > + dev_priv = to_i915(connector->base.dev); > > ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); > if (ret) { > @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, > } > > /* Implements Part 1 of the HDCP authorization procedure */ > -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > - const struct intel_hdcp_shim *shim) > +static int intel_hdcp_auth(struct intel_connector *connector) > { > struct drm_i915_private *dev_priv; > enum port port; > @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > u8 shim[DRM_HDCP_RI_LEN]; > } ri; > bool repeater_present; > + const struct intel_hdcp_shim *shim = connector->hdcp_shim; > + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); > > - dev_priv = intel_dig_port->base.base.dev->dev_private; > - > + dev_priv = to_i915(connector->base.dev); > port = intel_dig_port->base.port; > > /* Initialize An with 2 random values and acquire it */ > @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > * on those as well. > */ > > - if (repeater_present) > - return intel_hdcp_auth_downstream(intel_dig_port, shim); > + if (repeater_present) { > + ret = intel_hdcp_auth_downstream(connector); > + if (ret) { > + _intel_hdcp_disable(connector); If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you can avoid all of this code, it'd just be one line. > + return ret; > + } > + } > > DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); > return 0; > } > > -static > -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) > -{ > - return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); > -} > - > static int _intel_hdcp_disable(struct intel_connector *connector) > { > struct drm_i915_private *dev_priv = connector->base.dev->dev_private; > @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector) > return ret; > } > > - ret = intel_hdcp_auth(conn_to_dig_port(connector), > - connector->hdcp_shim); > + ret = intel_hdcp_auth(connector); > if (ret) { > DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret); > return ret; > -- > 2.7.4 >
On Friday 02 February 2018 07:39 PM, Sean Paul wrote: > On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote: >> We enable the HDCP encryption as a part of first stage authentication. >> So when second stage authentication fails, we need to disable the HDCP >> encryption and signalling. >> >> This patch handles above requirement. >> >> For reusing the _intel_hdcp_disable from generic authentication flow, >> this patch retain the reference to connector till the generic hdcp flow. >> This will be handy to provide the connector details in HDCP enable >> debug info. >> >> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >> --- >> drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++---------------- >> 1 file changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c >> index b97184eccd9c..0021511ae4d7 100644 >> --- a/drivers/gpu/drm/i915/intel_hdcp.c >> +++ b/drivers/gpu/drm/i915/intel_hdcp.c >> @@ -16,6 +16,8 @@ >> >> #define KEY_LOAD_TRIES 5 >> >> +static int _intel_hdcp_disable(struct intel_connector *connector); >> + >> static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, >> const struct intel_hdcp_shim *shim) >> { >> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) >> return true; >> } >> >> +static >> +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) >> +{ >> + return enc_to_dig_port(&connector->encoder->base); >> +} >> + >> /* Implements Part 2 of the HDCP authorization procedure */ >> static >> -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, >> - const struct intel_hdcp_shim *shim) >> +int intel_hdcp_auth_downstream(struct intel_connector *connector) >> { >> struct drm_i915_private *dev_priv; >> u32 vprime, sha_text, sha_leftovers, rep_ctl; >> u8 bstatus[2], num_downstream, *ksv_fifo; >> int ret, i, j, sha_idx; >> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >> + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); >> >> - dev_priv = intel_dig_port->base.base.dev->dev_private; >> + dev_priv = to_i915(connector->base.dev); >> >> ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); >> if (ret) { >> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, >> } >> >> /* Implements Part 1 of the HDCP authorization procedure */ >> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >> - const struct intel_hdcp_shim *shim) >> +static int intel_hdcp_auth(struct intel_connector *connector) >> { >> struct drm_i915_private *dev_priv; >> enum port port; >> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >> u8 shim[DRM_HDCP_RI_LEN]; >> } ri; >> bool repeater_present; >> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >> + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); >> >> - dev_priv = intel_dig_port->base.base.dev->dev_private; >> - >> + dev_priv = to_i915(connector->base.dev); >> port = intel_dig_port->base.port; >> >> /* Initialize An with 2 random values and acquire it */ >> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >> * on those as well. >> */ >> >> - if (repeater_present) >> - return intel_hdcp_auth_downstream(intel_dig_port, shim); >> + if (repeater_present) { >> + ret = intel_hdcp_auth_downstream(connector); >> + if (ret) { >> + _intel_hdcp_disable(connector); > If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you > can avoid all of this code, it'd just be one line. Yes. Thought about that option too. Actually I would prefer having the reference of intel_connector untill generic hdcp auth implementation. we can pass the intel digital port for encoder specific shims. This will help me with providing the connector details for state transitions as in next patches next steps like SRM passing in for revocation and downstream topology gathering (perhaps will start with availability of complementing user space) At this point if we dont want this code, I will disable hdcp from enable functions. -Ram > > >> + return ret; >> + } >> + } >> >> DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); >> return 0; >> } >> >> -static >> -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) >> -{ >> - return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); >> -} >> - >> static int _intel_hdcp_disable(struct intel_connector *connector) >> { >> struct drm_i915_private *dev_priv = connector->base.dev->dev_private; >> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector) >> return ret; >> } >> >> - ret = intel_hdcp_auth(conn_to_dig_port(connector), >> - connector->hdcp_shim); >> + ret = intel_hdcp_auth(connector); >> if (ret) { >> DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret); >> return ret; >> -- >> 2.7.4 >>
On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote: > > > On Friday 02 February 2018 07:39 PM, Sean Paul wrote: > > On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote: > > > We enable the HDCP encryption as a part of first stage authentication. > > > So when second stage authentication fails, we need to disable the HDCP > > > encryption and signalling. > > > > > > This patch handles above requirement. > > > > > > For reusing the _intel_hdcp_disable from generic authentication flow, > > > this patch retain the reference to connector till the generic hdcp flow. > > > This will be handy to provide the connector details in HDCP enable > > > debug info. > > > > > > Signed-off-by: Ramalingam C <ramalingam.c@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++---------------- > > > 1 file changed, 24 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c > > > index b97184eccd9c..0021511ae4d7 100644 > > > --- a/drivers/gpu/drm/i915/intel_hdcp.c > > > +++ b/drivers/gpu/drm/i915/intel_hdcp.c > > > @@ -16,6 +16,8 @@ > > > #define KEY_LOAD_TRIES 5 > > > +static int _intel_hdcp_disable(struct intel_connector *connector); > > > + > > > static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, > > > const struct intel_hdcp_shim *shim) > > > { > > > @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) > > > return true; > > > } > > > +static > > > +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) > > > +{ > > > + return enc_to_dig_port(&connector->encoder->base); > > > +} > > > + > > > /* Implements Part 2 of the HDCP authorization procedure */ > > > static > > > -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, > > > - const struct intel_hdcp_shim *shim) > > > +int intel_hdcp_auth_downstream(struct intel_connector *connector) > > > { > > > struct drm_i915_private *dev_priv; > > > u32 vprime, sha_text, sha_leftovers, rep_ctl; > > > u8 bstatus[2], num_downstream, *ksv_fifo; > > > int ret, i, j, sha_idx; > > > + const struct intel_hdcp_shim *shim = connector->hdcp_shim; > > > + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); > > > - dev_priv = intel_dig_port->base.base.dev->dev_private; > > > + dev_priv = to_i915(connector->base.dev); > > > ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); > > > if (ret) { > > > @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, > > > } > > > /* Implements Part 1 of the HDCP authorization procedure */ > > > -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > > > - const struct intel_hdcp_shim *shim) > > > +static int intel_hdcp_auth(struct intel_connector *connector) > > > { > > > struct drm_i915_private *dev_priv; > > > enum port port; > > > @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > > > u8 shim[DRM_HDCP_RI_LEN]; > > > } ri; > > > bool repeater_present; > > > + const struct intel_hdcp_shim *shim = connector->hdcp_shim; > > > + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); > > > - dev_priv = intel_dig_port->base.base.dev->dev_private; > > > - > > > + dev_priv = to_i915(connector->base.dev); > > > port = intel_dig_port->base.port; > > > /* Initialize An with 2 random values and acquire it */ > > > @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, > > > * on those as well. > > > */ > > > - if (repeater_present) > > > - return intel_hdcp_auth_downstream(intel_dig_port, shim); > > > + if (repeater_present) { > > > + ret = intel_hdcp_auth_downstream(connector); > > > + if (ret) { > > > + _intel_hdcp_disable(connector); > > If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you > > can avoid all of this code, it'd just be one line. > Yes. Thought about that option too. Actually I would prefer having the > reference of intel_connector untill generic hdcp auth implementation. > we can pass the intel digital port for encoder specific shims. > > This will help me with > providing the connector details for state transitions as in next patches > next steps like SRM passing in for revocation and downstream topology > gathering (perhaps will start with availability of complementing user space) > > At this point if we dont want this code, I will disable hdcp from enable > functions. Yes please. Let's not try to predict the future and keep things as simple as possible. Sean > > -Ram > > > > > > > + return ret; > > > + } > > > + } > > > DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); > > > return 0; > > > } > > > -static > > > -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) > > > -{ > > > - return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); > > > -} > > > - > > > static int _intel_hdcp_disable(struct intel_connector *connector) > > > { > > > struct drm_i915_private *dev_priv = connector->base.dev->dev_private; > > > @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector) > > > return ret; > > > } > > > - ret = intel_hdcp_auth(conn_to_dig_port(connector), > > > - connector->hdcp_shim); > > > + ret = intel_hdcp_auth(connector); > > > if (ret) { > > > DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret); > > > return ret; > > > -- > > > 2.7.4 > > > >
On Friday 02 February 2018 08:15 PM, Sean Paul wrote: > On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote: >> >> On Friday 02 February 2018 07:39 PM, Sean Paul wrote: >>> On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote: >>>> We enable the HDCP encryption as a part of first stage authentication. >>>> So when second stage authentication fails, we need to disable the HDCP >>>> encryption and signalling. >>>> >>>> This patch handles above requirement. >>>> >>>> For reusing the _intel_hdcp_disable from generic authentication flow, >>>> this patch retain the reference to connector till the generic hdcp flow. >>>> This will be handy to provide the connector details in HDCP enable >>>> debug info. >>>> >>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++---------------- >>>> 1 file changed, 24 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c >>>> index b97184eccd9c..0021511ae4d7 100644 >>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c >>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c >>>> @@ -16,6 +16,8 @@ >>>> #define KEY_LOAD_TRIES 5 >>>> +static int _intel_hdcp_disable(struct intel_connector *connector); >>>> + >>>> static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, >>>> const struct intel_hdcp_shim *shim) >>>> { >>>> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) >>>> return true; >>>> } >>>> +static >>>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) >>>> +{ >>>> + return enc_to_dig_port(&connector->encoder->base); >>>> +} >>>> + >>>> /* Implements Part 2 of the HDCP authorization procedure */ >>>> static >>>> -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, >>>> - const struct intel_hdcp_shim *shim) >>>> +int intel_hdcp_auth_downstream(struct intel_connector *connector) >>>> { >>>> struct drm_i915_private *dev_priv; >>>> u32 vprime, sha_text, sha_leftovers, rep_ctl; >>>> u8 bstatus[2], num_downstream, *ksv_fifo; >>>> int ret, i, j, sha_idx; >>>> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >>>> + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); >>>> - dev_priv = intel_dig_port->base.base.dev->dev_private; >>>> + dev_priv = to_i915(connector->base.dev); >>>> ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); >>>> if (ret) { >>>> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, >>>> } >>>> /* Implements Part 1 of the HDCP authorization procedure */ >>>> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >>>> - const struct intel_hdcp_shim *shim) >>>> +static int intel_hdcp_auth(struct intel_connector *connector) >>>> { >>>> struct drm_i915_private *dev_priv; >>>> enum port port; >>>> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >>>> u8 shim[DRM_HDCP_RI_LEN]; >>>> } ri; >>>> bool repeater_present; >>>> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >>>> + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); >>>> - dev_priv = intel_dig_port->base.base.dev->dev_private; >>>> - >>>> + dev_priv = to_i915(connector->base.dev); >>>> port = intel_dig_port->base.port; >>>> /* Initialize An with 2 random values and acquire it */ >>>> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >>>> * on those as well. >>>> */ >>>> - if (repeater_present) >>>> - return intel_hdcp_auth_downstream(intel_dig_port, shim); >>>> + if (repeater_present) { >>>> + ret = intel_hdcp_auth_downstream(connector); >>>> + if (ret) { >>>> + _intel_hdcp_disable(connector); >>> If you just call this from _intel_hdcp_enable when intel_hdcp_auth() fails, you >>> can avoid all of this code, it'd just be one line. >> Yes. Thought about that option too. Actually I would prefer having the >> reference of intel_connector untill generic hdcp auth implementation. >> we can pass the intel digital port for encoder specific shims. >> >> This will help me with >> providing the connector details for state transitions as in next patches >> next steps like SRM passing in for revocation and downstream topology >> gathering (perhaps will start with availability of complementing user space) >> >> At this point if we dont want this code, I will disable hdcp from enable >> functions. > Yes please. Let's not try to predict the future and keep things as simple as > possible. > > Sean but patch 3(adding connector info into hdcp state transition debug logs) still needs conenctor ref at auth. Do we still want to drop the connector ref passing? If so we will lose patch 3 too. -Ram > >> -Ram >>> >>>> + return ret; >>>> + } >>>> + } >>>> DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); >>>> return 0; >>>> } >>>> -static >>>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) >>>> -{ >>>> - return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); >>>> -} >>>> - >>>> static int _intel_hdcp_disable(struct intel_connector *connector) >>>> { >>>> struct drm_i915_private *dev_priv = connector->base.dev->dev_private; >>>> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector) >>>> return ret; >>>> } >>>> - ret = intel_hdcp_auth(conn_to_dig_port(connector), >>>> - connector->hdcp_shim); >>>> + ret = intel_hdcp_auth(connector); >>>> if (ret) { >>>> DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret); >>>> return ret; >>>> -- >>>> 2.7.4 >>>>
On Fri, Feb 2, 2018 at 9:51 AM, Ramalingam C <ramalingam.c@intel.com> wrote: > > > On Friday 02 February 2018 08:15 PM, Sean Paul wrote: >> >> On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote: >>> >>> >>> On Friday 02 February 2018 07:39 PM, Sean Paul wrote: >>>> >>>> On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote: >>>>> >>>>> We enable the HDCP encryption as a part of first stage authentication. >>>>> So when second stage authentication fails, we need to disable the HDCP >>>>> encryption and signalling. >>>>> >>>>> This patch handles above requirement. >>>>> >>>>> For reusing the _intel_hdcp_disable from generic authentication flow, >>>>> this patch retain the reference to connector till the generic hdcp >>>>> flow. >>>>> This will be handy to provide the connector details in HDCP enable >>>>> debug info. >>>>> >>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_hdcp.c | 41 >>>>> +++++++++++++++++++++++---------------- >>>>> 1 file changed, 24 insertions(+), 17 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c >>>>> b/drivers/gpu/drm/i915/intel_hdcp.c >>>>> index b97184eccd9c..0021511ae4d7 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c >>>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c >>>>> @@ -16,6 +16,8 @@ >>>>> #define KEY_LOAD_TRIES 5 >>>>> +static int _intel_hdcp_disable(struct intel_connector *connector); >>>>> + >>>>> static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port >>>>> *intel_dig_port, >>>>> const struct intel_hdcp_shim *shim) >>>>> { >>>>> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) >>>>> return true; >>>>> } >>>>> +static >>>>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector >>>>> *connector) >>>>> +{ >>>>> + return enc_to_dig_port(&connector->encoder->base); >>>>> +} >>>>> + >>>>> /* Implements Part 2 of the HDCP authorization procedure */ >>>>> static >>>>> -int intel_hdcp_auth_downstream(struct intel_digital_port >>>>> *intel_dig_port, >>>>> - const struct intel_hdcp_shim *shim) >>>>> +int intel_hdcp_auth_downstream(struct intel_connector *connector) >>>>> { >>>>> struct drm_i915_private *dev_priv; >>>>> u32 vprime, sha_text, sha_leftovers, rep_ctl; >>>>> u8 bstatus[2], num_downstream, *ksv_fifo; >>>>> int ret, i, j, sha_idx; >>>>> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >>>>> + struct intel_digital_port *intel_dig_port = >>>>> conn_to_dig_port(connector); >>>>> - dev_priv = intel_dig_port->base.base.dev->dev_private; >>>>> + dev_priv = to_i915(connector->base.dev); >>>>> ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); >>>>> if (ret) { >>>>> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct >>>>> intel_digital_port *intel_dig_port, >>>>> } >>>>> /* Implements Part 1 of the HDCP authorization procedure */ >>>>> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >>>>> - const struct intel_hdcp_shim *shim) >>>>> +static int intel_hdcp_auth(struct intel_connector *connector) >>>>> { >>>>> struct drm_i915_private *dev_priv; >>>>> enum port port; >>>>> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct >>>>> intel_digital_port *intel_dig_port, >>>>> u8 shim[DRM_HDCP_RI_LEN]; >>>>> } ri; >>>>> bool repeater_present; >>>>> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >>>>> + struct intel_digital_port *intel_dig_port = >>>>> conn_to_dig_port(connector); >>>>> - dev_priv = intel_dig_port->base.base.dev->dev_private; >>>>> - >>>>> + dev_priv = to_i915(connector->base.dev); >>>>> port = intel_dig_port->base.port; >>>>> /* Initialize An with 2 random values and acquire it */ >>>>> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct >>>>> intel_digital_port *intel_dig_port, >>>>> * on those as well. >>>>> */ >>>>> - if (repeater_present) >>>>> - return intel_hdcp_auth_downstream(intel_dig_port, >>>>> shim); >>>>> + if (repeater_present) { >>>>> + ret = intel_hdcp_auth_downstream(connector); >>>>> + if (ret) { >>>>> + _intel_hdcp_disable(connector); >>>> >>>> If you just call this from _intel_hdcp_enable when intel_hdcp_auth() >>>> fails, you >>>> can avoid all of this code, it'd just be one line. >>> >>> Yes. Thought about that option too. Actually I would prefer having the >>> reference of intel_connector untill generic hdcp auth implementation. >>> we can pass the intel digital port for encoder specific shims. >>> >>> This will help me with >>> providing the connector details for state transitions as in next >>> patches >>> next steps like SRM passing in for revocation and downstream >>> topology >>> gathering (perhaps will start with availability of complementing user >>> space) >>> >>> At this point if we dont want this code, I will disable hdcp from enable >>> functions. >> >> Yes please. Let's not try to predict the future and keep things as simple >> as >> possible. >> >> Sean > > but patch 3(adding connector info into hdcp state transition debug logs) > still needs conenctor ref at auth. > Do we still want to drop the connector ref passing? If so we will lose patch > 3 too. I was thinking about this further, and it doesn't really make sense to change some of the messages and not all of them. So perhaps middle-of-the-road solution would be to add a new DEBUG_KMS message at the beginning of both _intel_hdcp_enable and _intel_hdcp_disable listing the connector name/id and reporting that hdcp is being enabled/disabled. That way anything that follows can be attributed to that connector. This also avoids having to shuffle the arguments around. Sean > > -Ram > > >> >>> -Ram >>>> >>>> >>>>> + return ret; >>>>> + } >>>>> + } >>>>> DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); >>>>> return 0; >>>>> } >>>>> -static >>>>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector >>>>> *connector) >>>>> -{ >>>>> - return >>>>> enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); >>>>> -} >>>>> - >>>>> static int _intel_hdcp_disable(struct intel_connector *connector) >>>>> { >>>>> struct drm_i915_private *dev_priv = >>>>> connector->base.dev->dev_private; >>>>> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct >>>>> intel_connector *connector) >>>>> return ret; >>>>> } >>>>> - ret = intel_hdcp_auth(conn_to_dig_port(connector), >>>>> - connector->hdcp_shim); >>>>> + ret = intel_hdcp_auth(connector); >>>>> if (ret) { >>>>> DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret); >>>>> return ret; >>>>> -- >>>>> 2.7.4 >>>>> >
On Friday 02 February 2018 08:52 PM, Sean Paul wrote: > On Fri, Feb 2, 2018 at 9:51 AM, Ramalingam C <ramalingam.c@intel.com> wrote: >> >> On Friday 02 February 2018 08:15 PM, Sean Paul wrote: >>> On Fri, Feb 02, 2018 at 07:52:24PM +0530, Ramalingam C wrote: >>>> >>>> On Friday 02 February 2018 07:39 PM, Sean Paul wrote: >>>>> On Fri, Feb 02, 2018 at 04:15:13PM +0530, Ramalingam C wrote: >>>>>> We enable the HDCP encryption as a part of first stage authentication. >>>>>> So when second stage authentication fails, we need to disable the HDCP >>>>>> encryption and signalling. >>>>>> >>>>>> This patch handles above requirement. >>>>>> >>>>>> For reusing the _intel_hdcp_disable from generic authentication flow, >>>>>> this patch retain the reference to connector till the generic hdcp >>>>>> flow. >>>>>> This will be handy to provide the connector details in HDCP enable >>>>>> debug info. >>>>>> >>>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_hdcp.c | 41 >>>>>> +++++++++++++++++++++++---------------- >>>>>> 1 file changed, 24 insertions(+), 17 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdcp.c >>>>>> b/drivers/gpu/drm/i915/intel_hdcp.c >>>>>> index b97184eccd9c..0021511ae4d7 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_hdcp.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_hdcp.c >>>>>> @@ -16,6 +16,8 @@ >>>>>> #define KEY_LOAD_TRIES 5 >>>>>> +static int _intel_hdcp_disable(struct intel_connector *connector); >>>>>> + >>>>>> static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port >>>>>> *intel_dig_port, >>>>>> const struct intel_hdcp_shim *shim) >>>>>> { >>>>>> @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) >>>>>> return true; >>>>>> } >>>>>> +static >>>>>> +struct intel_digital_port *conn_to_dig_port(struct intel_connector >>>>>> *connector) >>>>>> +{ >>>>>> + return enc_to_dig_port(&connector->encoder->base); >>>>>> +} >>>>>> + >>>>>> /* Implements Part 2 of the HDCP authorization procedure */ >>>>>> static >>>>>> -int intel_hdcp_auth_downstream(struct intel_digital_port >>>>>> *intel_dig_port, >>>>>> - const struct intel_hdcp_shim *shim) >>>>>> +int intel_hdcp_auth_downstream(struct intel_connector *connector) >>>>>> { >>>>>> struct drm_i915_private *dev_priv; >>>>>> u32 vprime, sha_text, sha_leftovers, rep_ctl; >>>>>> u8 bstatus[2], num_downstream, *ksv_fifo; >>>>>> int ret, i, j, sha_idx; >>>>>> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >>>>>> + struct intel_digital_port *intel_dig_port = >>>>>> conn_to_dig_port(connector); >>>>>> - dev_priv = intel_dig_port->base.base.dev->dev_private; >>>>>> + dev_priv = to_i915(connector->base.dev); >>>>>> ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); >>>>>> if (ret) { >>>>>> @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct >>>>>> intel_digital_port *intel_dig_port, >>>>>> } >>>>>> /* Implements Part 1 of the HDCP authorization procedure */ >>>>>> -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, >>>>>> - const struct intel_hdcp_shim *shim) >>>>>> +static int intel_hdcp_auth(struct intel_connector *connector) >>>>>> { >>>>>> struct drm_i915_private *dev_priv; >>>>>> enum port port; >>>>>> @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct >>>>>> intel_digital_port *intel_dig_port, >>>>>> u8 shim[DRM_HDCP_RI_LEN]; >>>>>> } ri; >>>>>> bool repeater_present; >>>>>> + const struct intel_hdcp_shim *shim = connector->hdcp_shim; >>>>>> + struct intel_digital_port *intel_dig_port = >>>>>> conn_to_dig_port(connector); >>>>>> - dev_priv = intel_dig_port->base.base.dev->dev_private; >>>>>> - >>>>>> + dev_priv = to_i915(connector->base.dev); >>>>>> port = intel_dig_port->base.port; >>>>>> /* Initialize An with 2 random values and acquire it */ >>>>>> @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct >>>>>> intel_digital_port *intel_dig_port, >>>>>> * on those as well. >>>>>> */ >>>>>> - if (repeater_present) >>>>>> - return intel_hdcp_auth_downstream(intel_dig_port, >>>>>> shim); >>>>>> + if (repeater_present) { >>>>>> + ret = intel_hdcp_auth_downstream(connector); >>>>>> + if (ret) { >>>>>> + _intel_hdcp_disable(connector); >>>>> If you just call this from _intel_hdcp_enable when intel_hdcp_auth() >>>>> fails, you >>>>> can avoid all of this code, it'd just be one line. >>>> Yes. Thought about that option too. Actually I would prefer having the >>>> reference of intel_connector untill generic hdcp auth implementation. >>>> we can pass the intel digital port for encoder specific shims. >>>> >>>> This will help me with >>>> providing the connector details for state transitions as in next >>>> patches >>>> next steps like SRM passing in for revocation and downstream >>>> topology >>>> gathering (perhaps will start with availability of complementing user >>>> space) >>>> >>>> At this point if we dont want this code, I will disable hdcp from enable >>>> functions. >>> Yes please. Let's not try to predict the future and keep things as simple >>> as >>> possible. >>> >>> Sean >> but patch 3(adding connector info into hdcp state transition debug logs) >> still needs conenctor ref at auth. >> Do we still want to drop the connector ref passing? If so we will lose patch >> 3 too. > I was thinking about this further, and it doesn't really make sense to > change some of the messages and not all of them. So perhaps > middle-of-the-road solution would be to add a new DEBUG_KMS message at > the beginning of both _intel_hdcp_enable and _intel_hdcp_disable > listing the connector name/id and reporting that hdcp is being > enabled/disabled. That way anything that follows can be attributed to > that connector. This also avoids having to shuffle the arguments > around. > > Sean Ok I will add that as v2 of patch 3. --Ram > >> -Ram >> >> >>>> -Ram >>>>> >>>>>> + return ret; >>>>>> + } >>>>>> + } >>>>>> DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); >>>>>> return 0; >>>>>> } >>>>>> -static >>>>>> -struct intel_digital_port *conn_to_dig_port(struct intel_connector >>>>>> *connector) >>>>>> -{ >>>>>> - return >>>>>> enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); >>>>>> -} >>>>>> - >>>>>> static int _intel_hdcp_disable(struct intel_connector *connector) >>>>>> { >>>>>> struct drm_i915_private *dev_priv = >>>>>> connector->base.dev->dev_private; >>>>>> @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct >>>>>> intel_connector *connector) >>>>>> return ret; >>>>>> } >>>>>> - ret = intel_hdcp_auth(conn_to_dig_port(connector), >>>>>> - connector->hdcp_shim); >>>>>> + ret = intel_hdcp_auth(connector); >>>>>> if (ret) { >>>>>> DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret); >>>>>> return ret; >>>>>> -- >>>>>> 2.7.4 >>>>>>
diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index b97184eccd9c..0021511ae4d7 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -16,6 +16,8 @@ #define KEY_LOAD_TRIES 5 +static int _intel_hdcp_disable(struct intel_connector *connector); + static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, const struct intel_hdcp_shim *shim) { @@ -138,17 +140,24 @@ bool intel_hdcp_is_ksv_valid(u8 *ksv) return true; } +static +struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) +{ + return enc_to_dig_port(&connector->encoder->base); +} + /* Implements Part 2 of the HDCP authorization procedure */ static -int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, - const struct intel_hdcp_shim *shim) +int intel_hdcp_auth_downstream(struct intel_connector *connector) { struct drm_i915_private *dev_priv; u32 vprime, sha_text, sha_leftovers, rep_ctl; u8 bstatus[2], num_downstream, *ksv_fifo; int ret, i, j, sha_idx; + const struct intel_hdcp_shim *shim = connector->hdcp_shim; + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); - dev_priv = intel_dig_port->base.base.dev->dev_private; + dev_priv = to_i915(connector->base.dev); ret = intel_hdcp_poll_ksv_fifo(intel_dig_port, shim); if (ret) { @@ -385,8 +394,7 @@ int intel_hdcp_auth_downstream(struct intel_digital_port *intel_dig_port, } /* Implements Part 1 of the HDCP authorization procedure */ -static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, - const struct intel_hdcp_shim *shim) +static int intel_hdcp_auth(struct intel_connector *connector) { struct drm_i915_private *dev_priv; enum port port; @@ -405,9 +413,10 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, u8 shim[DRM_HDCP_RI_LEN]; } ri; bool repeater_present; + const struct intel_hdcp_shim *shim = connector->hdcp_shim; + struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); - dev_priv = intel_dig_port->base.base.dev->dev_private; - + dev_priv = to_i915(connector->base.dev); port = intel_dig_port->base.port; /* Initialize An with 2 random values and acquire it */ @@ -498,19 +507,18 @@ static int intel_hdcp_auth(struct intel_digital_port *intel_dig_port, * on those as well. */ - if (repeater_present) - return intel_hdcp_auth_downstream(intel_dig_port, shim); + if (repeater_present) { + ret = intel_hdcp_auth_downstream(connector); + if (ret) { + _intel_hdcp_disable(connector); + return ret; + } + } DRM_DEBUG_KMS("HDCP is enabled (no repeater present)\n"); return 0; } -static -struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) -{ - return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); -} - static int _intel_hdcp_disable(struct intel_connector *connector) { struct drm_i915_private *dev_priv = connector->base.dev->dev_private; @@ -558,8 +566,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector) return ret; } - ret = intel_hdcp_auth(conn_to_dig_port(connector), - connector->hdcp_shim); + ret = intel_hdcp_auth(connector); if (ret) { DRM_ERROR("Failed to authenticate HDCP (%d)\n", ret); return ret;
We enable the HDCP encryption as a part of first stage authentication. So when second stage authentication fails, we need to disable the HDCP encryption and signalling. This patch handles above requirement. For reusing the _intel_hdcp_disable from generic authentication flow, this patch retain the reference to connector till the generic hdcp flow. This will be handy to provide the connector details in HDCP enable debug info. Signed-off-by: Ramalingam C <ramalingam.c@intel.com> --- drivers/gpu/drm/i915/intel_hdcp.c | 41 +++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 17 deletions(-)