| Message ID | 20240925065658.1269767-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/hdcp: Retry first read and writes to downstream | expand |
On Wed, 25 Sep 2024, Suraj Kandpal <suraj.kandpal@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? > 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@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. > + 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? > + > + 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. > + 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;
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Wednesday, September 25, 2024 2:26 PM > To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Kandpal, Suraj > <suraj.kandpal@intel.com> > Subject: Re: [PATCH] drm/i915/hdcp: Retry first read and writes to > downstream > > On Wed, 25 Sep 2024, Suraj Kandpal <suraj.kandpal@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@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
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++) { + if (!intel_hdcp2_get_capability(connector)) { + msleep(50); + continue; + } + + 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 + 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;
Retry the first read and write to downstream at least 10 times with a 50ms delay if not hdcp2 capable. The reason being that 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@intel.com> --- drivers/gpu/drm/i915/display/intel_hdcp.c | 26 +++++++++++++++++------ 1 file changed, 19 insertions(+), 7 deletions(-)