On Wed, 25 Sep 2024, Arun R Murthy <arun.r.murthy@xxxxxxxxx> 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 <nagavenkata.srikanth.v@xxxxxxxxx>
> Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> Reviewed-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> ---
> .../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
