> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Wednesday, September 25, 2024 2:26 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Kandpal, Suraj
> <suraj.kandpal@xxxxxxxxx>
> Subject: Re: [PATCH] drm/i915/hdcp: Retry first read and writes to
> downstream
>
> On Wed, 25 Sep 2024, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
