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

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