> -----Original Message-----
> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@xxxxxxxxx>
> Sent: Wednesday, October 30, 2024 10:20 AM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-xe@xxxxxxxxxxxxxxxxxxxxx;
> intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] drm/i915/hdcp: Add the loop only for DP encoders
>
>
>
> > -----Original Message-----
> > From: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>
> > Sent: Wednesday, October 30, 2024 8:23 AM
> > To: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah@xxxxxxxxx>
> > Subject: [PATCH] drm/i915/hdcp: Add the loop only for DP encoders
> >
> > Add the loop on the first read/write we do to give docks and dp
> > encoders some extra time to get their HDCP DPCD registers ready only
> > for DP/DPMST encoders as this issue is only observed here no need to
> > burden other encoders with extra retries as this causes the HDMI
> > connector to have some other timing issue and fails HDCP authentication.
> >
> > --v2
> > -Add intent of patch [Chaitanya]
> > -Add reasoning for loop [Jani]
> > -Make sure we forfiet the 50ms wait for non DP/DPMST encoders.
> >
> > Fixes: 9d5a05f86d2f ("drm/i915/hdcp: Retry first read and writes to
> > downstream")
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_hdcp.c | 36
> > +++++++++++++++--------
> > 1 file changed, 23 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index ed6aa87403e2..c8ba69c51cce 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -1503,6 +1503,8 @@ static int hdcp2_deauthenticate_port(struct
> > intel_connector *connector) static int
> > hdcp2_authentication_key_exchange(struct intel_connector *connector) {
> > struct intel_display *display = to_intel_display(connector);
> > + struct intel_digital_port *dig_port =
> > + intel_attached_dig_port(connector);
> > struct intel_hdcp *hdcp = &connector->hdcp;
> > union {
> > struct hdcp2_ake_init ake_init;
> > @@ -1513,31 +1515,39 @@ static int
> > hdcp2_authentication_key_exchange(struct intel_connector *connector)
> > } msgs;
> > const struct intel_hdcp_shim *shim = hdcp->shim;
> > size_t size;
> > - int ret, i;
> > + bool is_dp_encoder;
> > + int ret, i, max_retries;
> >
> > /* Init for seq_num */
> > hdcp->seq_num_v = 0;
> > hdcp->seq_num_m = 0;
> >
> > + is_dp_encoder = intel_encoder_is_dp(&dig_port->base) ||
> > + intel_encoder_is_mst(&dig_port->base);
> > + if (is_dp_encoder)
> > + max_retries = 10;
> > + else
> > + max_retries = 1;
> > +
> > ret = hdcp2_prepare_ake_init(connector, &msgs.ake_init);
> > 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(dock decides to stop
> > advertising
> > - * hdcp2 capability for some reason). The reason being that
> > - * during suspend resume dock usually keeps 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
> > - * value 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. The values of 10 and delay of 50ms was decided
> > based
> > - * on multiple trial and errors.
> > + * with a 50ms delay if not hdcp2 capable for DP/DPMST encoders
> > + * (dock decides to stop advertising hdcp2 capability for some
> > reason).
> > + * The reason being that during suspend resume dock usually keeps
> > 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 value 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. The values of 10 and delay of
> > 50ms
> > + * was decided based on multiple trial and errors.
> > */
> > - for (i = 0; i < 10; i++) {
> > - if (!intel_hdcp2_get_capability(connector)) {
> > + for (i = 0; i < max_retries; i++) {
> > + if (!intel_hdcp2_get_capability(connector) &&
> > is_dp_encoder) {
>
> Now I am a bit confused, if you are using this Boolean here. Do you still need
> different values for max_retries?
You are correct this shouldn't be here does not really make any sense to have if (!intel_hdcp2_get_capability(connector) &&
is_dp_encoder)
should have just been
(!intel_hdcp2_get_capability(connector))
Regards,
Suraj Kandpal
>
> Regards
>
> Chaitanya
>
> > msleep(50);
> > continue;
> > }
> > --
> > 2.34.1
