[PATCH 01/13] drm/amd/display: HDCP SEND AKI INIT error

[Paul Menzel]

Dear Alex, dear Ahmad,


Thank you for the patch.

Am 18.03.22 um 22:47 schrieb Alex Hung:
> From: Ahmad Othman <ahmad.othman at amd.com>

Could you please make the commit message summary/title a statement by 
adding a verb (imperative mood) [1]. Maybe:

drm/amd/display: Fix HDCP SEND AKI INIT error

> [why]
> HDCP sends AKI INIT error in case of multiple display on dock

What is the test setup exactly, and how can the error be reproduced? 
Does Linux log something?

> [how]
> Added new checks and method to handfle display adjustment

s/Added/Add/
s/handfle/handle/

> for multiple display cases

Why are these checks and methods correct, and what do they try to 
achieve? Is it the HDCP(?) specification?

> Reviewed-by: Wenjing Liu <Wenjing.Liu at amd.com>
> Acked-by: Alex Hung <alex.hung at amd.com>
> Signed-off-by: Ahmad Othman <ahmad.othman at amd.com>

Could the order be reversed, so it’s clear that the Signed-off-by line 
came first and not after the review? Or is it actually signed off after 
the review again?

> ---
>   .../gpu/drm/amd/display/modules/hdcp/hdcp.c   | 38 ++++++++++++++++++-
>   .../gpu/drm/amd/display/modules/hdcp/hdcp.h   |  8 ++++
>   .../drm/amd/display/modules/inc/mod_hdcp.h    |  2 +-
>   3 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
> index 3e81850a7ffe..5e01c6e24cbc 100644
> --- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
> +++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.c
> @@ -251,6 +251,33 @@ static enum mod_hdcp_status reset_connection(struct mod_hdcp *hdcp,
>   	return status;
>   }
>   
> +static enum mod_hdcp_status update_display_adjustments(struct mod_hdcp *hdcp,
> +		struct mod_hdcp_display *display,
> +		struct mod_hdcp_display_adjustment *adj)
> +{
> +	enum mod_hdcp_status status = MOD_HDCP_STATUS_NOT_IMPLEMENTED;
> +
> +	if (is_in_authenticated_states(hdcp) &&
> +			is_dp_mst_hdcp(hdcp) &&
> +			display->adjust.disable == true &&
> +			adj->disable == false) {
> +		display->adjust.disable = false;
> +		if (is_hdcp1(hdcp))
> +			status = mod_hdcp_hdcp1_enable_dp_stream_encryption(hdcp);
> +		else if (is_hdcp2(hdcp))
> +			status = mod_hdcp_hdcp2_enable_dp_stream_encryption(hdcp);
> +
> +		if (status != MOD_HDCP_STATUS_SUCCESS)
> +			display->adjust.disable = true;
> +	}
> +
> +	if (status == MOD_HDCP_STATUS_SUCCESS &&
> +		memcmp(adj, &display->adjust,
> +		sizeof(struct mod_hdcp_display_adjustment)) != 0)
> +		status = MOD_HDCP_STATUS_NOT_IMPLEMENTED;
> +
> +	return status;
> +}
>   /*
>    * Implementation of functions in mod_hdcp.h
>    */
> @@ -391,7 +418,7 @@ enum mod_hdcp_status mod_hdcp_remove_display(struct mod_hdcp *hdcp,
>   	return status;
>   }
>   
> -enum mod_hdcp_status mod_hdcp_update_authentication(struct mod_hdcp *hdcp,
> +enum mod_hdcp_status mod_hdcp_update_display(struct mod_hdcp *hdcp,
>   		uint8_t index,
>   		struct mod_hdcp_link_adjustment *link_adjust,
>   		struct mod_hdcp_display_adjustment *display_adjust,
> @@ -419,6 +446,15 @@ enum mod_hdcp_status mod_hdcp_update_authentication(struct mod_hdcp *hdcp,
>   		goto out;
>   	}
>   
> +	if (memcmp(link_adjust, &hdcp->connection.link.adjust,
> +			sizeof(struct mod_hdcp_link_adjustment)) == 0 &&
> +			memcmp(display_adjust, &display->adjust,
> +					sizeof(struct mod_hdcp_display_adjustment)) != 0) {
> +		status = update_display_adjustments(hdcp, display, display_adjust);
> +		if (status != MOD_HDCP_STATUS_NOT_IMPLEMENTED)
> +			goto out;
> +	}
> +
>   	/* stop current authentication */
>   	status = reset_authentication(hdcp, output);
>   	if (status != MOD_HDCP_STATUS_SUCCESS)
> diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h
> index 399fbca8947b..6b195207de90 100644
> --- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h
> +++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h
> @@ -445,6 +445,14 @@ static inline uint8_t is_in_hdcp2_dp_states(struct mod_hdcp *hdcp)
>   			current_state(hdcp) <= HDCP2_DP_STATE_END);
>   }
>   
> +static inline uint8_t is_in_authenticated_states(struct mod_hdcp *hdcp)
> +{
> +	return (current_state(hdcp) == D1_A4_AUTHENTICATED ||
> +	current_state(hdcp) == H1_A45_AUTHENTICATED ||
> +	current_state(hdcp) == D2_A5_AUTHENTICATED ||
> +	current_state(hdcp) == H2_A5_AUTHENTICATED);
> +}
> +

The compiler is probably going to optimize the four `current_state` 
calls away, but maybe use a helper variable, so it’s clear for the 
reader the same function is each time. Also, why not put the helper into
`drivers/gpu/drm/amd/display/modules/hdcp/hdcp.h`?

>   static inline uint8_t is_hdcp1(struct mod_hdcp *hdcp)
>   {
>   	return (is_in_hdcp1_states(hdcp) || is_in_hdcp1_dp_states(hdcp));
> diff --git a/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h b/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h
> index f7420c3f5672..3348bb97ef81 100644
> --- a/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h
> +++ b/drivers/gpu/drm/amd/display/modules/inc/mod_hdcp.h
> @@ -294,7 +294,7 @@ enum mod_hdcp_status mod_hdcp_remove_display(struct mod_hdcp *hdcp,
>   		uint8_t index, struct mod_hdcp_output *output);
>   
>   /* called per display to apply new authentication adjustment */
> -enum mod_hdcp_status mod_hdcp_update_authentication(struct mod_hdcp *hdcp,
> +enum mod_hdcp_status mod_hdcp_update_display(struct mod_hdcp *hdcp,
>   		uint8_t index,
>   		struct mod_hdcp_link_adjustment *link_adjust,
>   		struct mod_hdcp_display_adjustment *display_adjust,


Kind regards,

Paul


[1]: https://chris.beams.io/posts/git-commit/