Re: [PATCH 1/8] drm/i915: Handle failure from 2nd stage HDCP auth

[Ramalingam C <ramalingam.c@intel.com>]

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
>>>>>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx