[PATCH] drm/i915/hdcp: Retry first read and writes to downstream

[Kandpal, Suraj]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Wednesday, September 25, 2024 2:26 PM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>; Kandpal, Suraj
> <suraj.kandpal at intel.com>
> Subject: Re: [PATCH] drm/i915/hdcp: Retry first read and writes to
> downstream
> 
> On Wed, 25 Sep 2024, Suraj Kandpal <suraj.kandpal at intel.com> wrote:
> > Retry the first read and write to downstream at least 10 times with a
> > 50ms delay if not hdcp2 capable. The reason being that
> 
> What are the 10 times and 50 ms delay based on?
> 

Multiple trial and errors and getting it working of wd19tb dock, hp type c and Lenovo
Dock,

> > during suspend resume Dock usually keep the HDCP2 registers
> > inaccesible causing AUX error. This wouldn't be a big problem if the
> > userspace just kept retrying with some delay while it continues to
> > play low values content but most userpace applications end up throwing
> > an error when it receives one from KMD. This makes sure we give the
> > dock and the sink devices to complete its power cycle and then try
> > HDCP authentication.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_hdcp.c | 26
> > +++++++++++++++++------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index 2afa92321b08..5f2383c219e8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -1512,7 +1512,7 @@ static int
> hdcp2_authentication_key_exchange(struct intel_connector *connector)
> >  	} msgs;
> >  	const struct intel_hdcp_shim *shim = hdcp->shim;
> >  	size_t size;
> > -	int ret;
> > +	int ret, i;
> >
> >  	/* Init for seq_num */
> >  	hdcp->seq_num_v = 0;
> > @@ -1522,13 +1522,25 @@ static int
> hdcp2_authentication_key_exchange(struct intel_connector *connector)
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	ret = shim->write_2_2_msg(connector, &msgs.ake_init,
> > -				  sizeof(msgs.ake_init));
> > -	if (ret < 0)
> > -		return ret;
> > +	for (i = 0; i <= 10; i++) {
> 
> I'm pretty nitpicky about this. Please avoid <= in the for loop if you can. Just
> make it the obvious (i = 0; i < 10; i++), or if you have 1 try
> + 10 retries, then i < 10 + 1 or i < 11.
> 
> If the 10 retries is just made up, then maybe just try 10 times total.

Sure will update it to have just < 10

> 
> > +		if (!intel_hdcp2_get_capability(connector)) {
> > +			msleep(50);
> > +			continue;
> > +		}
> 
> This changes behaviour for the first try too. Is that intentional? No mention
> in the commit message?

Yes normally when we se that the aux returns a NACK we also see that hdcp2 capability suddenly
gets reported as not capable hence we add a little delay to give dock a little time to sort itself out.
Will add this in the commit message too.

> 
> > +
> > +		ret = shim->write_2_2_msg(connector, &msgs.ake_init,
> > +					  sizeof(msgs.ake_init));
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		ret = shim->read_2_2_msg(connector,
> HDCP_2_2_AKE_SEND_CERT,
> > +					 &msgs.send_cert,
> sizeof(msgs.send_cert));
> > +		if (ret < 0)
> > +			continue;
> > +		else
> 
> The else is redundant here.

Ohkay we can the if(ret > 0) break
Since we need to come out of the loop as soon as the first write and the read pass

Regards,
Suraj Kandpal
 
> 
> > +			break;
> > +	}
> >
> > -	ret = shim->read_2_2_msg(connector, HDCP_2_2_AKE_SEND_CERT,
> > -				 &msgs.send_cert, sizeof(msgs.send_cert));
> >  	if (ret < 0)
> >  		return ret;
> 
> --
> Jani Nikula, Intel