[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 <[email protected]> Sent: 19 March 2022 01:16 To: Hung, Alex <[email protected]>; Othman, Ahmad <[email protected]> Cc: [email protected] <[email protected]>; Wang, Chao-kai (Stylon) <[email protected]>; Li, Sun peng (Leo) <[email protected]>; Wentland, Harry <[email protected]>; Zhuo, Qingqing (Lillian) <[email protected]>; Siqueira, Rodrigo <[email protected]>; Li, Roman <[email protected]>; Chiu, Solomon <[email protected]>; Pillai, Aurabindo <[email protected]>; Lin, Wayne <[email protected]>; Liu, Wenjing <[email protected]>; Lakha, Bhawanpreet <[email protected]>; Gutierrez, Agustin <[email protected]>; Kotarac, Pavle <[email protected]> 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 <[email protected]> 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 <[email protected]> > Acked-by: Alex Hung <[email protected]> > Signed-off-by: Ahmad Othman <[email protected]> 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
