Re: [PATCH 32/33] drm/amd/display: fix link training regression for 1 or 2 lane

[Harry Wentland <harry.wentland@amd.com>]

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@amd.com>
>>
>> [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@amd.com>
>> Acked-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>> Signed-off-by: Wenjing Liu <wenjing.liu@amd.com>
>> ---
>>   .../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