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

["Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>]

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

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
>