On Wed, 25 Sep 2024, Arun R Murthy <[email protected]> wrote: > Figure 3-52: 128b132b DP DPTC LANEx_CHANNEL_EQ_DONE Sequence of > DP2.1a spec. > After reading LANEx_CHANNEL_EQ_DONE, read the FFE presets. > AUX_RD_INTERVAL and then write the new FFE presets.
Nope. That's just not what the figure has. > Co-developed-by: Srikanth V NagaVenkata <[email protected]> > Signed-off-by: Arun R Murthy <[email protected]> > Reviewed-by: Suraj Kandpal <[email protected]> > --- > .../gpu/drm/i915/display/intel_dp_link_training.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 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 f41b69840ad9..1bac00e46533 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -1419,12 +1419,6 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp, > for (try = 0; try < max_tries; try++) { > fsleep(delay_us); > > - /* > - * The delay may get updated. The transmitter shall read the > - * delay before link status during link training. > - */ > - delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux); > - > if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < > 0) { > lt_err(intel_dp, DP_PHY_DPRX, "Failed to read link > status\n"); > return false; > @@ -1451,8 +1445,14 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp, > if (time_after(jiffies, deadline)) > timeout = true; /* try one last time after deadline */ > > - /* Update signal levels and training set as requested. */ > intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, > link_status); > + /* > + * During LT, Tx shall read AUX_RD_INTERVAL just before writing > the new FFE > + * presets. > + */ > + delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux); I said this should be put *above* intel_dp_get_adjust_train(), and you agreed. There was even a patch to that effect, and that's what I thought we were going by. It's both logical and per spec to keep the TX FFE PRESET read and write together, and AUX RD INTERVAL read *before* them. Where does the spec say, "read AUX_RD_INTERVAL just before writing the new FFE presets"? I don't think it does. The box in figure 3-52 has: - Read AUX_RD_INTERVAL value - Adjust the TX_FFE_PRESET_VALUE setting as requested by a DPRX/LTTPR_UFP The "as requested by" part involves reading TX FFE PRESET to know what the DPRX requested. I don't see anything wrong in the original comment, just the placement of the read. There have been a multitude of different patches with random version numbers with no changelog and I don't even know what I'm supposed to be reviewing anymore. BR, Jani. > + > + /* Update signal levels and training set as requested. */ > if (!intel_dp_update_link_train(intel_dp, crtc_state, > DP_PHY_DPRX)) { > lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX FFE > settings\n"); > return false; -- Jani Nikula, Intel
