| Message ID | 20230417100021.3205172-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display/dp: 128/132b LT requirement | expand |
On Mon, 17 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. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > .../drm/i915/display/intel_dp_link_training.c | 52 +++++++++++++------ > 1 file changed, 35 insertions(+), 17 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..83ea9ece0157 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -686,23 +686,41 @@ 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 128/132 DP Channel coding and then set link rate > + */ > + link_config[0] = crtc_state->vrr.enable ? 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); > + /* 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); > + } else { > + /* 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.enable ? 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); > + } I'd rather we change the order for 8b10b too. If we can't do that, you need to add two functions that do each step, and then call them in different order for different channel coding. We don't want all of the above duplicated. Also, in what looks like a rebase fail, you change vrr.flipline to vrr.enable. BR, Jani. > > return true; > }
On Mon, 17 Apr 2023, Jani Nikula <jani.nikula@intel.com> wrote: > On Mon, 17 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. >> >> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> >> --- >> .../drm/i915/display/intel_dp_link_training.c | 52 +++++++++++++------ >> 1 file changed, 35 insertions(+), 17 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..83ea9ece0157 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c >> @@ -686,23 +686,41 @@ 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 128/132 DP Channel coding and then set link rate PS. I've taken great care to use "128b/132b" in comments everywhere. There isn't a single instance of "128/132". It'll be helpful when you git grep 128b/132b. BR, Jani. >> + */ >> + link_config[0] = crtc_state->vrr.enable ? 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); >> + /* 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); >> + } else { >> + /* 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.enable ? 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); >> + } > > I'd rather we change the order for 8b10b too. > > If we can't do that, you need to add two functions that do each step, > and then call them in different order for different channel coding. We > don't want all of the above duplicated. > > Also, in what looks like a rebase fail, you change vrr.flipline to > vrr.enable. > > > BR, > Jani. > > >> >> return true; >> }
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..83ea9ece0157 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -686,23 +686,41 @@ 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 128/132 DP Channel coding and then set link rate + */ + link_config[0] = crtc_state->vrr.enable ? 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); + /* 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); + } else { + /* 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.enable ? 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); + } return true; }
For 128b/132b LT prior to LT DPTX should set power state, DP channel coding and then link rate. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- .../drm/i915/display/intel_dp_link_training.c | 52 +++++++++++++------ 1 file changed, 35 insertions(+), 17 deletions(-)