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
>> + }
>> +}
>> +
>> 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
