On 2021-10-25 9:58 a.m., Harry Wentland wrote:
On 2021-10-25 07:25, Paul Menzel wrote:
Dear Wenjing, dear Rodrigo,
On 24.10.21 15:31, Rodrigo Siqueira wrote:
From: Wenjing Liu <wenjing.liu@xxxxxxx>
[why]
We have a regression that cause maximize lane settings to use
uninitialized data from unused lanes.
Which commit caused the regression? Please amend the commit message.
This will cause link training to fail for 1 or 2 lanes because the lane
adjust is populated incorrectly sometimes.
On what card did you test this, and how can it be reproduced?
Please describe the fix/implemantation in the commit message.
Reviewed-by: Eric Yang <eric.yang2@xxxxxxx>
Acked-by: Rodrigo Siqueira <Rodrigo.Siqueira@xxxxxxx>
Signed-off-by: Wenjing Liu <wenjing.liu@xxxxxxx>
---
.../gpu/drm/amd/display/dc/core/dc_link_dp.c | 35 +++++++++++++++++--
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
index 653279ab96f4..f6ba7c734f54 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
@@ -108,6 +108,9 @@ static struct dc_link_settings get_common_supported_link_settings(
struct dc_link_settings link_setting_b);
static void maximize_lane_settings(const struct link_training_settings *lt_settings,
struct dc_lane_settings lane_settings[LANE_COUNT_DP_MAX]);
+static void override_lane_settings(const struct link_training_settings *lt_settings,
+ struct dc_lane_settings lane_settings[LANE_COUNT_DP_MAX]);
+
static uint32_t get_cr_training_aux_rd_interval(struct dc_link *link,
const struct dc_link_settings *link_settings)
{
@@ -734,15 +737,13 @@ void dp_decide_lane_settings(
}
#endif
}
-
- /* we find the maximum of the requested settings across all lanes*/
- /* and set this maximum for all lanes*/
dp_hw_to_dpcd_lane_settings(lt_settings, hw_lane_settings, dpcd_lane_settings);
if (lt_settings->disallow_per_lane_settings) {
/* we find the maximum of the requested settings across all lanes*/
/* and set this maximum for all lanes*/
maximize_lane_settings(lt_settings, hw_lane_settings);
+ override_lane_settings(lt_settings, hw_lane_settings);
if (lt_settings->always_match_dpcd_with_hw_lane_settings)
dp_hw_to_dpcd_lane_settings(lt_settings, hw_lane_settings, dpcd_lane_settings);
@@ -833,6 +834,34 @@ static void maximize_lane_settings(const struct link_training_settings *lt_setti
}
}
+static void override_lane_settings(const struct link_training_settings *lt_settings,
+ struct dc_lane_settings lane_settings[LANE_COUNT_DP_MAX])
+{
+ uint32_t lane;
+
+ if (lt_settings->voltage_swing == NULL &&
+ lt_settings->pre_emphasis == NULL &&
+#if defined(CONFIG_DRM_AMD_DC_DP2_0)
+ lt_settings->ffe_preset == NULL &&
+#endif
+ lt_settings->post_cursor2 == NULL)
+
+ return;
+
+ for (lane = 1; lane < LANE_COUNT_DP_MAX; lane++) {
+ if (lt_settings->voltage_swing)
+ lane_settings[lane].VOLTAGE_SWING = *lt_settings->voltage_swing;
+ if (lt_settings->pre_emphasis)
+ lane_settings[lane].PRE_EMPHASIS = *lt_settings->pre_emphasis;
+ if (lt_settings->post_cursor2)
+ lane_settings[lane].POST_CURSOR2 = *lt_settings->post_cursor2;
+#if defined(CONFIG_DRM_AMD_DC_DP2_0)
+ if (lt_settings->ffe_preset)
+ lane_settings[lane].FFE_PRESET = *lt_settings->ffe_preset;
+#endif
Normally these checks should be done in C and not the preprocessor. `if CONFIG(DRM_AMD_DC_DP2_0)` or similar should work.
Interesting. I've never seen this before. Do you have an example or link to a doc? A cursory search doesn't yield any results but I might not be searching for the right thing.
Harry
I'm curious about this too. The compiler with optimizations should
remove the constant check, but technically the C standard only permits
it - it doesn't guarantee that it happens.
However, this patch should actually be changed to drop these
CONFIG_DRM_AMD_DC_DP2_0 guards - this isn't a Kconfig option nor will
there be one specifically for DP2. This should be folded under the DCN
support.
Regards,
Nicholas Kazlauskas
+ }
+}
+
enum dc_status dp_get_lane_status_and_lane_adjust(
struct dc_link *link,
const struct link_training_settings *link_training_setting,
Kind regards,
Paul
