[PATCH 01/13] drm/amd/display: HDCP SEND AKI INIT error
Hung, Alex
Alex.Hung at amd.com
Fri Mar 25 20:43:00 UTC 2022
[AMD Official Use Only]
Hi Paul,
Thanks for your feedbacks. I fixed many errors and typos you highlighted in this series. In cases where modification requires re-testing we or anyone can have follow-up patches in the future.
________________________________
From: Paul Menzel <pmenzel at molgen.mpg.de>
Sent: 19 March 2022 01:16
To: Hung, Alex <Alex.Hung at amd.com>; Othman, Ahmad <Ahmad.Othman at amd.com>
Cc: amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>; Wang, Chao-kai (Stylon) <Stylon.Wang at amd.com>; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>; Wentland, Harry <Harry.Wentland at amd.com>; Zhuo, Qingqing (Lillian) <Qingqing.Zhuo at amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira at amd.com>; Li, Roman <Roman.Li at amd.com>; Chiu, Solomon <Solomon.Chiu at amd.com>; Pillai, Aurabindo <Aurabindo.Pillai at amd.com>; Lin, Wayne <Wayne.Lin at amd.com>; Liu, Wenjing <Wenjing.Liu at amd.com>; Lakha, Bhawanpreet <Bhawanpreet.Lakha at amd.com>; Gutierrez, Agustin <Agustin.Gutierrez at amd.com>; Kotarac, Pavle <Pavle.Kotarac at amd.com>
Subject: Re: [PATCH 01/13] drm/amd/display: HDCP SEND AKI INIT error
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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchris.beams.io%2Fposts%2Fgit-commit%2F&data=04%7C01%7Calex.hung%40amd.com%7Ce0754902cc3545234c1508da097874f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637832710221132603%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=nixPwZB4AwGHzYIF3euukbDQcgJ0zE0GwzBQVIlUe2U%3D&reserved=0
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220325/49035c49/attachment-0001.htm>
More information about the amd-gfx
mailing list