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://chris.beams.io/posts/git-commit/
