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

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

[Suraj Kandpal]

Add the loop on the first read and write only for DP/DPMST encoders
as this issue is only observed here no need to burden other
encoders with extra retries.

Fixes: 9d5a05f86d2f ("drm/i915/hdcp: Retry first read and writes to downstream")
Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 drivers/gpu/drm/i915/display/intel_hdcp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c
index ed6aa87403e2..fcfb5a9f303a 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,12 +1515,18 @@ 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;
+	int ret, i, max_retries;
 
 	/* Init for seq_num */
 	hdcp->seq_num_v = 0;
 	hdcp->seq_num_m = 0;
 
+	if (intel_encoder_is_dp(&dig_port->base) ||
+	    intel_encoder_is_mst(&dig_port->base))
+		max_retries = 10;
+	else
+		max_retries = 1;
+
 	ret = hdcp2_prepare_ake_init(connector, &msgs.ake_init);
 	if (ret < 0)
 		return ret;
@@ -1536,7 +1544,7 @@ static int hdcp2_authentication_key_exchange(struct intel_connector *connector)
 	 * authentication. The values of 10 and delay of 50ms was decided based
 	 * on multiple trial and errors.
 	 */
-	for (i = 0; i < 10; i++) {
+	for (i = 0; i < max_retries; i++) {
 		if (!intel_hdcp2_get_capability(connector)) {
 			msleep(50);
 			continue;
-- 
2.34.1

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

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> Sent: Tuesday, October 29, 2024 2:23 PM
> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; Kandpal,
> Suraj <suraj.kandpal@intel.com>
> Subject: [PATCH] drm/i915/hdcp: Add the loop only for DP encoders
> 
> Add the loop on the first read and write only for DP/DPMST encoders as this
> issue is only observed here no need to burden other encoders with extra
> retries.

It would make the motivation of the patch clearer if we describe "this issue" here.

> 
> Fixes: 9d5a05f86d2f ("drm/i915/hdcp: Retry first read and writes to
> downstream")
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdcp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> b/drivers/gpu/drm/i915/display/intel_hdcp.c
> index ed6aa87403e2..fcfb5a9f303a 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,12 +1515,18 @@ 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;
> +	int ret, i, max_retries;
> 
>  	/* Init for seq_num */
>  	hdcp->seq_num_v = 0;
>  	hdcp->seq_num_m = 0;
> 
> +	if (intel_encoder_is_dp(&dig_port->base) ||
> +	    intel_encoder_is_mst(&dig_port->base))
> +		max_retries = 10;
> +	else
> +		max_retries = 1;
> +
>  	ret = hdcp2_prepare_ake_init(connector, &msgs.ake_init);
>  	if (ret < 0)
>  		return ret;
> @@ -1536,7 +1544,7 @@ static int
> hdcp2_authentication_key_exchange(struct intel_connector *connector)
>  	 * authentication. The values of 10 and delay of 50ms was decided
> based
>  	 * on multiple trial and errors.
>  	 */

Perhaps this comment can use an update too.

Functionally LGTM.

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>

> -	for (i = 0; i < 10; i++) {
> +	for (i = 0; i < max_retries; i++) {
>  		if (!intel_hdcp2_get_capability(connector)) {
>  			msleep(50);
>  			continue;
> --
> 2.34.1

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

[Jani Nikula]

On Tue, 29 Oct 2024, "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com> wrote:
>> -----Original Message-----
>> From: Kandpal, Suraj <suraj.kandpal@intel.com>
>> Sent: Tuesday, October 29, 2024 2:23 PM
>> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
>> Cc: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; Kandpal,
>> Suraj <suraj.kandpal@intel.com>
>> Subject: [PATCH] drm/i915/hdcp: Add the loop only for DP encoders
>> 
>> Add the loop on the first read and write only for DP/DPMST encoders as this
>> issue is only observed here no need to burden other encoders with extra
>> retries.
>
> It would make the motivation of the patch clearer if we describe "this issue" here.

Ditto for "the loop".

>
>> 
>> Fixes: 9d5a05f86d2f ("drm/i915/hdcp: Retry first read and writes to
>> downstream")
>> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_hdcp.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
>> b/drivers/gpu/drm/i915/display/intel_hdcp.c
>> index ed6aa87403e2..fcfb5a9f303a 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,12 +1515,18 @@ 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;
>> +	int ret, i, max_retries;
>> 
>>  	/* Init for seq_num */
>>  	hdcp->seq_num_v = 0;
>>  	hdcp->seq_num_m = 0;
>> 
>> +	if (intel_encoder_is_dp(&dig_port->base) ||
>> +	    intel_encoder_is_mst(&dig_port->base))
>> +		max_retries = 10;
>> +	else
>> +		max_retries = 1;
>> +
>>  	ret = hdcp2_prepare_ake_init(connector, &msgs.ake_init);
>>  	if (ret < 0)
>>  		return ret;
>> @@ -1536,7 +1544,7 @@ static int
>> hdcp2_authentication_key_exchange(struct intel_connector *connector)
>>  	 * authentication. The values of 10 and delay of 50ms was decided
>> based
>>  	 * on multiple trial and errors.
>>  	 */
>
> Perhaps this comment can use an update too.
>
> Functionally LGTM.
>
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>
>> -	for (i = 0; i < 10; i++) {
>> +	for (i = 0; i < max_retries; i++) {
>>  		if (!intel_hdcp2_get_capability(connector)) {
>>  			msleep(50);
>>  			continue;
>> --
>> 2.34.1
>

-- 
Jani Nikula, Intel

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

[Suraj Kandpal]

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@intel.com>
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@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) {
 			msleep(50);
 			continue;
 		}
-- 
2.34.1

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

[Borah, Chaitanya Kumar]

> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> Sent: Wednesday, October 30, 2024 8:23 AM
> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Kandpal, Suraj <suraj.kandpal@intel.com>; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@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@intel.com>
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@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?

Regards

Chaitanya

>  			msleep(50);
>  			continue;
>  		}
> --
> 2.34.1

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

[Jani Nikula]

On Wed, 30 Oct 2024, "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com> wrote:
>> -----Original Message-----
>> From: Kandpal, Suraj <suraj.kandpal@intel.com>
>> Sent: Wednesday, October 30, 2024 8:23 AM
>> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
>> Cc: Kandpal, Suraj <suraj.kandpal@intel.com>; Borah, Chaitanya Kumar
>> <chaitanya.kumar.borah@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@intel.com>
>> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@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?

Even more confusing, if you get false for hdcp2 cap with HDMI, you'll
continue with this stuff anyway?! What's the point in that?

BR,
Jani.

>
> Regards
>
> Chaitanya
>
>>  			msleep(50);
>>  			continue;
>>  		}
>> --
>> 2.34.1
>

-- 
Jani Nikula, Intel

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

[Kandpal, Suraj]

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

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

[Kandpal, Suraj]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Wednesday, October 30, 2024 3:52 PM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; Kandpal,
> Suraj <suraj.kandpal@intel.com>; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org
> Subject: RE: [PATCH] drm/i915/hdcp: Add the loop only for DP encoders
> 
> On Wed, 30 Oct 2024, "Borah, Chaitanya Kumar"
> <chaitanya.kumar.borah@intel.com> wrote:
> >> -----Original Message-----
> >> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> >> Sent: Wednesday, October 30, 2024 8:23 AM
> >> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> >> Cc: Kandpal, Suraj <suraj.kandpal@intel.com>; Borah, Chaitanya Kumar
> >> <chaitanya.kumar.borah@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@intel.com>
> >> Reviewed-by: Chaitanya Kumar Borah
> <chaitanya.kumar.borah@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?
> 
> Even more confusing, if you get false for hdcp2 cap with HDMI, you'll continue
> with this stuff anyway?! What's the point in that?
> 
> BR,
> Jani.

You are right this check shouldn't have been added does not make any sense here


Regards,
Suraj Kandpal

> 
> >
> > Regards
> >
> > Chaitanya
> >
> >>  			msleep(50);
> >>  			continue;
> >>  		}
> >> --
> >> 2.34.1
> >
> 
> --
> Jani Nikula, Intel

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

[Jani Nikula]

On Wed, 30 Oct 2024, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> 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]

What I really meant was, the subject and the commit message talk about
"the loop" as if it's the one and only loop in all of HDCP.

Imagine having to write a changelog for all the changes in a pull
request [1]. Imagine you don't know anything about the details in the
commits.

First, you look at the commit subject. Huh, what loop? Then you look at
the commit message. Huh, what? The commit message is a single five-line
sentence. It's really really hard to understand what it's trying to
say. What's being done? Especially why? What's the user impact? I'd have
to look at the code to understand, and that's not a great sign for the
commit message. If it's a fix, is there a bug report that I could look
at to help me understand, etc.


BR,
Jani.


[1] https://lore.kernel.org/r/878quu6go9.fsf@intel.com


> -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@intel.com>
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@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) {
>  			msleep(50);
>  			continue;
>  		}

-- 
Jani Nikula, Intel