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