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

[Rodrigo Siqueira Jordao <rjordrigo@amd.com>]

Hi Paul,

First of all, thanks for your feedback.

Before I address your comments, I just want to give you some background 
about this type of patchset.

All patches in this series are from a week ago and came from all display 
teams in AMD. Keep in mind that we share our display core with other OS; 
as a result, we have multiple updates every week. To keep the upstream 
aligned with the Display core, we run a weekly process where we extract 
patches made for other people who might be a good candidates to be sent 
to the upstream. The process looks like this (oversimplified):

1. Run a script to check which commit from last week is a good candidate 
to go upstream - finally, port all of those patches to the upstream 
(this is a manual task).
2. Prepare a branch based on amd-staging-drm-next with all the new 
patches on top of it.
3. Send it to our validation team to run an extensive set of tests in 
the new series.
4. If everything is ok, send the patchset to amdgfx.
5. If the test results are positive, we merge this series in the upstream.

Again, that's just a summary; we have way more steps here...

On 2021-10-25 7:25 a.m., 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 is one of the situations where the week interval creates some 
descriptions that might sound a little bit different. Basically, this 
patch refers to the patch "implement decide lane settings", in this 
series; however, we noticed that this is not reverted, and that's why we 
decided to keep both patches in this series.

>> 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?

Our DQE team validates this series using multiple ASICs. See the final 
report from Daniel in reply to this series cover letter for more details.

> 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.

Nice catch! I'm going to drop this CONFIG_DRM_AMD_DC_DP2_0 line.

Thanks
Siqueira

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