<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=Windows-1252"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only] <div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Hi Paul,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> 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.</div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Paul Menzel <pmenzel@molgen.mpg.de> Sent: 19 March 2022 01:16 To: Hung, Alex <Alex.Hung@amd.com>; Othman, Ahmad <Ahmad.Othman@amd.com> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Wang, Chao-kai (Stylon) <Stylon.Wang@amd.com>; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>; Zhuo, Qingqing (Lillian) <Qingqing.Zhuo@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Li, Roman <Roman.Li@amd.com>; Chiu, Solomon <Solomon.Chiu@amd.com>; Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>; Liu, Wenjing <Wenjing.Liu@amd.com>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@amd.com>; Gutierrez, Agustin <Agustin.Gutierrez@amd.com>; Kotarac, Pavle <Pavle.Kotarac@amd.com> Subject: Re: [PATCH 01/13] drm/amd/display: HDCP SEND AKI INIT error <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">Dear Alex, dear Ahmad, Thank you for the patch. Am 18.03.22 um 22:47 schrieb Alex Hung: > From: Ahmad Othman <ahmad.othman@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@amd.com> > Acked-by: Alex Hung <alex.hung@amd.com> > Signed-off-by: Ahmad Othman <ahmad.othman@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]: <a href="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"> 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</a> </div> </div> </div> </body> </html>