[PATCHv2 0/3] Some correction in the DP Link Training sequence

[PATCHv2 0/3] Some correction in the DP Link Training sequence

[Arun R Murthy]

Arun R Murthy (3):
  drm/i915/dp: use fsleep instead of usleep_range for LT
  drm/i915/dp: read Aux RD interval just before setting the FFE preset
  drm/i915/dp: Include the time taken by AUX Tx for timeout

 .../drm/i915/display/intel_dp_link_training.c | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCHv2 1/3] drm/i915/dp: use fsleep instead of usleep_range for LT

[Arun R Murthy]

Sleeping for < 10us use udelay, for 10us to 20ms use usleep_range() and
for > 10ms use msleep. flseep() will call the particular API based on
the above condition. (Documentation/timers/timers-howto.rst)
Aux RD Interval value depends on the value read from the dpcd register
which is updated from the sink device, hence use flseep.

Co-developed-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_link_training.c | 6 +++---
 1 file changed, 3 insertions(+), 3 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 397cc4ebae52..f41b69840ad9 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -898,7 +898,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
 
 	voltage_tries = 1;
 	for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
-		usleep_range(delay_us, 2 * delay_us);
+		fsleep(delay_us);
 
 		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
 						     link_status) < 0) {
@@ -1040,7 +1040,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp,
 	}
 
 	for (tries = 0; tries < 5; tries++) {
-		usleep_range(delay_us, 2 * delay_us);
+		fsleep(delay_us);
 
 		if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy,
 						     link_status) < 0) {
@@ -1417,7 +1417,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
 	deadline = jiffies + msecs_to_jiffies_timeout(400);
 
 	for (try = 0; try < max_tries; try++) {
-		usleep_range(delay_us, 2 * delay_us);
+		fsleep(delay_us);
 
 		/*
 		 * The delay may get updated. The transmitter shall read the
-- 
2.25.1

[PATCHv3 2/3] drm/i915/dp: read Aux RD interval just before setting the FFE preset

[Arun R Murthy]

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.

Co-developed-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>

squash 2

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 .../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..ec6c323a49c0 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 DPCD 02216h before DPCD 00202h to 00207h and
+		 * 0200Ch through 0200Fh.
+		 */
+		delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
+
+		/* 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;
-- 
2.25.1

[PATCHv2 3/3] drm/i915/dp: Include the time taken by AUX Tx for timeout

[Arun R Murthy]

As per DP spec the timeout for LANE_CHANNEL_EQ_DONE is 400ms. But this
timeout value is exclusively for the Aux RD Interval and excludes the
time consumed for the AUX Tx (i.e reading/writing FFE presets). Add
another 50ms for these AUX Tx to the 400ms timeout.
Ref: "Figure 3-52: 128b132b DP DPTC LANEx_CHANNEL_EQ_DONE Sequence" of
DP2.1a spec.

Co-developed-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_link_training.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 ec6c323a49c0..ddf91b3533a4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -1414,7 +1414,7 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
 	}
 
 	/* Time budget for the LANEx_EQ_DONE Sequence */
-	deadline = jiffies + msecs_to_jiffies_timeout(400);
+	deadline = jiffies + msecs_to_jiffies_timeout(450);
 
 	for (try = 0; try < max_tries; try++) {
 		fsleep(delay_us);
-- 
2.25.1

[PATCHv3 2/3] drm/i915/dp: read Aux RD interval just before setting the FFE preset

[Arun R Murthy]

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.

Co-developed-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
---
 .../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);
+
+		/* 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;
-- 
2.25.1

Re: [PATCHv3 2/3] drm/i915/dp: read Aux RD interval just before setting the FFE preset

[Jani Nikula]

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

RE: [PATCHv3 2/3] drm/i915/dp: read Aux RD interval just before setting the FFE preset

[Murthy, Arun R]

> -----Original Message-----
> From: Jani Nikula <jani.nikula@linux.intel.com>
> Sent: Wednesday, September 25, 2024 2:18 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-xe@lists.freedesktop.org;
> intel-gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>; Srikanth V, NagaVenkata
> <nagavenkata.srikanth.v@intel.com>; Kandpal, Suraj
> <suraj.kandpal@intel.com>
> Subject: Re: [PATCHv3 2/3] drm/i915/dp: read Aux RD interval just before
> setting the FFE preset
> 
> On Wed, 25 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> 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@intel.com>
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > Reviewed-by: Suraj Kandpal <suraj.kandpal@intel.com>
> > ---
> >  .../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.
> 
Yes I had pushed a patch also, but later based on Suraj comment pointing to the spec changed this. I am not able to find that again in spec.

> It's both logical and per spec to keep the TX FFE PRESET read and write
> together, and AUX RD INTERVAL read *before* them.
> 
Yes agree!

> 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.
> 
Yes, but I now see a different thing now
" During LT, the transmitter shall read DPCD 02216h before DPCD 00202h through 00207h,
and 0200Ch through 0200Fh. If DPCD 00204h[0] = 0 and DPCD 0200Eh[0] = 0, the transmitter
shall adjust the drive setting according to the receiver's request, and then write the new setting
to DPCD 00103h through 00106h and re-initiate the receiver counter"

Ref: 3.5.2.16.1.1 of spec DP2.1

Here it says read AUX_RD_INTERVAL before reading the lane states(0x0202, 0x0203) reading the
FFE presets(0x0206, 0x0207), which is what our original code was doing.

> 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.
> 
Sorry for that!

Thanks and Regards,
Arun R Murthy
--------------------