|
[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@xxxxxxxxxxxxx>
Sent: 19 March 2022 01:16 To: Hung, Alex <Alex.Hung@xxxxxxx>; Othman, Ahmad <Ahmad.Othman@xxxxxxx> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Wang, Chao-kai (Stylon) <Stylon.Wang@xxxxxxx>; Li, Sun peng (Leo) <Sunpeng.Li@xxxxxxx>; Wentland, Harry <Harry.Wentland@xxxxxxx>; Zhuo, Qingqing (Lillian) <Qingqing.Zhuo@xxxxxxx>; Siqueira, Rodrigo <Rodrigo.Siqueira@xxxxxxx>; Li, Roman <Roman.Li@xxxxxxx>; Chiu, Solomon <Solomon.Chiu@xxxxxxx>; Pillai, Aurabindo <Aurabindo.Pillai@xxxxxxx>; Lin, Wayne <Wayne.Lin@xxxxxxx>; Liu, Wenjing <Wenjing.Liu@xxxxxxx>; Lakha, Bhawanpreet <Bhawanpreet.Lakha@xxxxxxx>; Gutierrez, Agustin <Agustin.Gutierrez@xxxxxxx>; Kotarac, Pavle <Pavle.Kotarac@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> > Acked-by: Alex Hung <alex.hung@xxxxxxx> > Signed-off-by: Ahmad Othman <ahmad.othman@xxxxxxx> 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=""> |
