[PATCH] drm/i915/hdcp: Add the loop only for DP encoders

[Kandpal, Suraj]

> -----Original Message-----
> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah at intel.com>
> Sent: Wednesday, October 30, 2024 10:20 AM
> To: Kandpal, Suraj <suraj.kandpal at intel.com>; intel-xe at lists.freedesktop.org;
> intel-gfx at lists.freedesktop.org
> Subject: RE: [PATCH] drm/i915/hdcp: Add the loop only for DP encoders
> 
> 
> 
> > -----Original Message-----
> > From: Kandpal, Suraj <suraj.kandpal at intel.com>
> > Sent: Wednesday, October 30, 2024 8:23 AM
> > To: intel-xe at lists.freedesktop.org; intel-gfx at lists.freedesktop.org
> > Cc: Kandpal, Suraj <suraj.kandpal at intel.com>; Borah, Chaitanya Kumar
> > <chaitanya.kumar.borah at intel.com>
> > 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 at intel.com>
> > Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah at intel.com>
> > ---
> >  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