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 11:12, Paul Menzel wrote:
> 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@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.
> 
> 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,
> 

Interesting. Thanks for sharing. I wasn't aware of that.

Unfortunately a lot of our configs fall into this category
for which ifdef is still needed:

> However, this approach still allows the C compiler to
> see the code inside the block, and check it for correctness
> (syntax, types, symbol references, etc). Thus, you still
> have to use an #ifdef if the code inside the block references
> symbols that will not exist if the condition is not met.

I'll keep an eye out for where we can use the C check instead
of macros in the future.

Harry

> 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