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

[Paul Menzel]

Dear Nicholas, dear Harry,


On 25.10.21 16:42, Kazlauskas, Nicholas wrote:
> On 2021-10-25 9:58 a.m., Harry Wentland wrote:

>> On 2021-10-25 07:25, Paul Menzel wrote:

>>> On 24.10.21 15:31, Rodrigo Siqueira wrote:
>>>> From: Wenjing Liu <wenjing.liu at 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 at amd.com>
>>>> Acked-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>
>>>> Signed-off-by: Wenjing Liu <wenjing.liu at 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.

 From the Linux kernel coding style [1][2]:

> Within code, where possible, use the IS_ENABLED macro to convert a
> Kconfig symbol into a C boolean expression, and use it in a normal C
> conditional:
> 
> if (IS_ENABLED(CONFIG_SOMETHING)) {
>         ...
> }


Kind regards,

Paul


>>>> +    }
>>>> +}
>>>> +
>>>>    enum dc_status dp_get_lane_status_and_lane_adjust(
>>>>        struct dc_link *link,
>>>>        const struct link_training_settings *link_training_setting,

[1]: 
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
[2]: Documentation/process/coding-style.rst