Re: [Intel-gfx] [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT requirement

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 19, 2023 at 10:07:46AM +0000, Murthy, Arun R wrote:
> > -----Original Message-----
> > From: Nikula, Jani <jani.nikula@intel.com>
> > Sent: Wednesday, April 19, 2023 3:26 PM
> > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: ville.syrjala@linux.intel.com
> > Subject: RE: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
> > requirement
> > 
> > On Wed, 19 Apr 2023, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
> > >> -----Original Message-----
> > >> From: Nikula, Jani <jani.nikula@intel.com>
> > >> Sent: Wednesday, April 19, 2023 12:48 PM
> > >> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> > >> gfx@lists.freedesktop.org
> > >> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > >> Subject: Re: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
> > >> requirement
> > >>
> > >> On Wed, 19 Apr 2023, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > >> > For 128b/132b LT prior to LT DPTX should set power state, DP
> > >> > channel coding and then link rate.
> > >> >
> > >> > v2: added separate function to avoid code duplication(Jani N)
> > >> >
> > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > >>
> > >> RESEND for what reason?
> > > Typo is sending V2 patch hence corrected and sent it again.
> > >
> > >>
> > >> Two v2 and neither fixes
> > >> https://lore.kernel.org/r/87o7nmergw.fsf@intel.com
> > > This is pointing to the v1 patch.
> > > V2 patch addressing review comments can be located @
> > > https://lore.kernel.org/all/20230419022522.3457924-1-arun.r.murthy@int
> > > el.com/
> > 
> > Argh.
> > 
> > RESEND means you're sending the exact same patch again. Hence *re-send*.
> > That's what I thought. That's what everyone would think.
> > 
> > It's even documented in submitting-patches.rst [1].
> > 
> > ---
> > 
> > There's still the question of whether we could just change the order for
> > 8b/10b too [2]. On IRC, Ville thinks we could, "i don't think there is any order
> > specified. just use the same alwas imo".
> > 
> Spec DP2.1 section 3.5.1.2 (for 8b/10b LT)
> write the following Link Configuration parameters:
> * LINK_BW_SET register (DPCD 00100h)
> * LANE_COUNT_SET field in the LANE_COUNT_SET register (DPCD 00101h[4:0])
> * DOWNSPREAD_CTRL register (DPCD 00107h)
> * MAIN_LINK_CHANNEL_CODING_SET register (DPCD 00108h)

Looks like an unordered list to me

> 
> Whereas for 128b/132b section 3.5.2.16 says
> Prior to link training, a DPTX should perform the following:
> 1 Verify that the SET_POWER_STATE field in the
> SET_POWER_AND_SET_DP_PWR_VOLTAGE register is programmed to D0 normal
> operation (DPCD 00600h[2:0] = 001b).
> 2 Write DPCD 00108h = 02h to select 128b/132b DP channel coding.
> 3 Program the target link rate and lane count by way of an AUX write transaction to
> DPCD 00100h and 00101h, respectively

whereas this is an ordered list.

> 
> 
> Thanks and Regards,
> Arun R Murthy
> -------------------
> > 
> > BR,
> > Jani.
> > 
> > 
> > [1] https://docs.kernel.org/process/submitting-patches.html#don-t-get-
> > discouraged-or-impatient
> > [2] https://lore.kernel.org/r/87r0siernf.fsf@intel.com
> > 
> > 
> > 
> > 
> > 
> > 
> > >
> > > Thanks and Regards,
> > > Arun R Murthy
> > > --------------------
> > >>
> > >> BR,
> > >> Jani.
> > >>
> > >>
> > >> > ---
> > >> >  .../drm/i915/display/intel_dp_link_training.c | 62
> > >> > +++++++++++++------
> > >> >  1 file changed, 44 insertions(+), 18 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> > index 6aa4ae5e7ebe..e5809cf7d0c4 100644
> > >> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> > @@ -637,6 +637,37 @@ static bool
> > >> intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
> > >> >     return true;
> > >> >  }
> > >> >
> > >> > +static void
> > >> > +intel_dp_update_downspread_ctrl(struct intel_dp *intel_dp,
> > >> > +                           const struct intel_crtc_state *crtc_state) {
> > >> > +   u8 link_config[2];
> > >> > +
> > >> > +   link_config[0] = crtc_state->vrr.flipline ?
> > >> DP_MSA_TIMING_PAR_IGNORE_EN : 0;
> > >> > +   link_config[1] = intel_dp_is_uhbr(crtc_state) ?
> > >> > +                    DP_SET_ANSI_128B132B : DP_SET_ANSI_8B10B;
> > >> > +   drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL,
> > >> link_config,
> > >> > +2); }
> > >> > +
> > >> > +static void
> > >> > +intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
> > >> > +                       const struct intel_crtc_state *crtc_state,
> > >> > +                       u8 link_bw, u8 rate_select) {
> > >> > +   u8 link_config[2];
> > >> > +
> > >> > +   /* Write the link configuration data */
> > >> > +   link_config[0] = link_bw;
> > >> > +   link_config[1] = crtc_state->lane_count;
> > >> > +   if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> > >> > +           link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> > >> > +   drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config,
> > >> 2);
> > >> > +   /* eDP 1.4 rate select method. */
> > >> > +   if (!link_bw)
> > >> > +           drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
> > >> > +                             &rate_select, 1); }
> > >> > +
> > >> >  /*
> > >> >   * Prepare link training by configuring the link parameters. On
> > >> > DDI
> > >> platforms
> > >> >   * also enable the port here.
> > >> > @@ -647,7 +678,6 @@ intel_dp_prepare_link_train(struct intel_dp
> > >> > *intel_dp,  {
> > >> >     struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > >> >     struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > >> > -   u8 link_config[2];
> > >> >     u8 link_bw, rate_select;
> > >> >
> > >> >     if (intel_dp->prepare_link_retrain) @@ -686,23 +716,19 @@
> > >> > intel_dp_prepare_link_train(struct intel_dp
> > >> *intel_dp,
> > >> >             drm_dbg_kms(&i915->drm,
> > >> >                         "[ENCODER:%d:%s] Using LINK_RATE_SET value
> > >> %02x\n",
> > >> >                         encoder->base.base.id, encoder->base.name,
> > >> rate_select);
> > >> > -
> > >> > -   /* Write the link configuration data */
> > >> > -   link_config[0] = link_bw;
> > >> > -   link_config[1] = crtc_state->lane_count;
> > >> > -   if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
> > >> > -           link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> > >> > -   drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config,
> > >> 2);
> > >> > -
> > >> > -   /* eDP 1.4 rate select method. */
> > >> > -   if (!link_bw)
> > >> > -           drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
> > >> > -                             &rate_select, 1);
> > >> > -
> > >> > -   link_config[0] = crtc_state->vrr.flipline ?
> > >> DP_MSA_TIMING_PAR_IGNORE_EN : 0;
> > >> > -   link_config[1] = intel_dp_is_uhbr(crtc_state) ?
> > >> > -           DP_SET_ANSI_128B132B : DP_SET_ANSI_8B10B;
> > >> > -   drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL,
> > >> link_config, 2);
> > >> > +   if (intel_dp_is_uhbr(crtc_state)) {
> > >> > +           /*
> > >> > +            * Spec DP2.1 Section 3.5.2.16
> > >> > +            * Prior to LT DPTX should set 128b/132b DP Channel
> > >> > + coding
> > >> and then set link rate
> > >> > +            */
> > >> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> > >> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> > >> > +                                       rate_select);
> > >> > +   } else {
> > >> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> > >> > +                                       rate_select);
> > >> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> > >> > +   }
> > >> >
> > >> >     return true;
> > >> >  }
> > >>
> > >> --
> > >> Jani Nikula, Intel Open Source Graphics Center
> > 
> > --
> > Jani Nikula, Intel Open Source Graphics Center

-- 
Ville Syrjälä
Intel