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

[Paul Menzel <pmenzel@xxxxxxxxxxxxx>]

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,

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