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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[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="">

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux