diff mbox series

[2/3] drm/i915/dp: read Aux RD interval after reading the FFE preset

Message ID 20240912050552.779356-3-arun.r.murthy@intel.com (mailing list archive)
State New, archived
Headers show
Series Some correction in the DP Link Training dequence | expand

Commit Message

Murthy, Arun R Sept. 12, 2024, 5:05 a.m. UTC 
DP Source should be reading AUX_RD interval after we get adjusted
TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting
in DP Source)

Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Jani Nikula Sept. 12, 2024, 9:04 a.m. UTC | #1 
On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> DP Source should be reading AUX_RD interval after we get adjusted
> TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting
> in DP Source)

Please explain why.

BR,
Jani.

>
> Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 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..ca179bed46ad 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;
> @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
>  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX FFE settings\n");
>  			return false;
>  		}
> +
> +		/*
> +		 * 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 (try == max_tries) {
Murthy, Arun R Sept. 12, 2024, 10:54 a.m. UTC | #2 
> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > DP Source should be reading AUX_RD interval after we get adjusted
> > TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting in DP
> > Source)
> 
> Please explain why.
 As per the DP 2.1 spec section 3.5.2.16.1
"The transmitter shall finish reading
from DPCD 00202h through 00207h, DPCD 0200Ch through 0200Fh, and DPCD 02216h, and
writing to DPCD 00103h through 00106h (listed as "AUX TX response" in Figure 3-51) within
2.5 ms or less, such that the total duration for AUX TX responses with a 20-loop count does not
exceed 50 ms. "

Thanks and Regards,
Arun R Murthy
--------------------
Srikanth V, NagaVenkata Sept. 12, 2024, 11:58 a.m. UTC | #3 
Hi Jani,



It's as per DP2.1 spec, where we should be reading AUX_RD interval at the loop before we wait.

[cid:image001.png@01DB0538.65998930]

Regards

Srikanth



-----Original Message-----
From: Murthy, Arun R <arun.r.murthy@intel.com>
Sent: Thursday, September 12, 2024 4:25 PM
To: Jani Nikula <jani.nikula@linux.intel.com>; intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
Cc: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com>
Subject: RE: [PATCH 2/3] drm/i915/dp: read Aux RD interval after reading the FFE preset



> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com<mailto:arun.r.murthy@intel.com>> wrote:

> > DP Source should be reading AUX_RD interval after we get adjusted

> > TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting in

> > DP

> > Source)

>

> Please explain why.

As per the DP 2.1 spec section 3.5.2.16.1 "The transmitter shall finish reading from DPCD 00202h through 00207h, DPCD 0200Ch through 0200Fh, and DPCD 02216h, and writing to DPCD 00103h through 00106h (listed as "AUX TX response" in Figure 3-51) within

2.5 ms or less, such that the total duration for AUX TX responses with a 20-loop count does not exceed 50 ms. "



Thanks and Regards,

Arun R Murthy

--------------------
Suraj Kandpal Sept. 23, 2024, 6:21 a.m. UTC | #4 
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Arun R
> Murthy
> Sent: Thursday, September 12, 2024 10:36 AM
> To: 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>
> Subject: [PATCH 2/3] drm/i915/dp: read Aux RD interval after reading the FFE
> preset
> 
> DP Source should be reading AUX_RD interval after we get adjusted
> TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting in DP
> Source)

I think mentioning the dp spec reference here would be helpful

> 
> Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 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..ca179bed46ad 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;
> @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> *intel_dp,
>  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX
> FFE settings\n");
>  			return false;
>  		}
> +
> +		/*
> +		 * The delay may get updated. The transmitter shall read the
> +		 * delay before link status during link training.
> +		 */

The comment needs to be updated as this is not being done before link status 
Also a question does this not conflict with the requirement we previously had (reading it before link status) ?

Regards,
Suraj Kandpal

Regards,
Suraj Kandpal

> +		delay_us =
> drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
>  	}
> 
>  	if (try == max_tries) {
> --
> 2.25.1
Srikanth V, NagaVenkata Sept. 23, 2024, 6:28 a.m. UTC | #5 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > Arun R Murthy
> > Sent: Thursday, September 12, 2024 10:36 AM
> > To: 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>
> > Subject: [PATCH 2/3] drm/i915/dp: read Aux RD interval after reading
> > the FFE preset
> >
> > DP Source should be reading AUX_RD interval after we get adjusted
> > TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting in DP
> > Source)
> 
> I think mentioning the dp spec reference here would be helpful
> 
Please refer to Figure 3-52: 128b132b DP DPTC LANEx_CHANNEL_EQ_DONE Sequence of DP2.1a spec.

> >
> > Signed-off-by: Srikanth V NagaVenkata
> > <nagavenkata.srikanth.v@intel.com>
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 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..ca179bed46ad 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;
> > @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> > *intel_dp,
> >  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX
> FFE
> > settings\n");
> >  			return false;
> >  		}
> > +
> > +		/*
> > +		 * The delay may get updated. The transmitter shall read the
> > +		 * delay before link status during link training.
> > +		 */
> 
> The comment needs to be updated as this is not being done before link
> status Also a question does this not conflict with the requirement we
> previously had (reading it before link status) ?
> 
> Regards,
> Suraj Kandpal
> 
> Regards,
> Suraj Kandpal
> 
> > +		delay_us =
> > drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
> >  	}
> >
> >  	if (try == max_tries) {
> > --
> > 2.25.1
Suraj Kandpal Sept. 23, 2024, 6:32 a.m. UTC | #6 
> -----Original Message-----
> From: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Sent: Monday, September 23, 2024 11:59 AM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; 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>
> Subject: RE: [PATCH 2/3] drm/i915/dp: read Aux RD interval after reading the
> FFE preset
> 
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > Arun R Murthy
> > > Sent: Thursday, September 12, 2024 10:36 AM
> > > To: 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>
> > > Subject: [PATCH 2/3] drm/i915/dp: read Aux RD interval after reading
> > > the FFE preset
> > >
> > > DP Source should be reading AUX_RD interval after we get adjusted
> > > TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting in
> > > DP
> > > Source)
> >
> > I think mentioning the dp spec reference here would be helpful
> >
> Please refer to Figure 3-52: 128b132b DP DPTC LANEx_CHANNEL_EQ_DONE
> Sequence of DP2.1a spec.

I think you can update your commit message to say DP2.1a spec Fig. 3-52

Regards,
Suraj Kandpal
> 
> > >
> > > Signed-off-by: Srikanth V NagaVenkata
> > > <nagavenkata.srikanth.v@intel.com>
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 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..ca179bed46ad 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;
> > > @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> > > *intel_dp,
> > >  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX
> > FFE
> > > settings\n");
> > >  			return false;
> > >  		}
> > > +
> > > +		/*
> > > +		 * The delay may get updated. The transmitter shall read the
> > > +		 * delay before link status during link training.
> > > +		 */
> >
> > The comment needs to be updated as this is not being done before link
> > status Also a question does this not conflict with the requirement we
> > previously had (reading it before link status) ?
> >
> > Regards,
> > Suraj Kandpal
> >
> > Regards,
> > Suraj Kandpal
> >
> > > +		delay_us =
> > > drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
> > >  	}
> > >
> > >  	if (try == max_tries) {
> > > --
> > > 2.25.1
Suraj Kandpal Sept. 23, 2024, 6:56 a.m. UTC | #7 
> -----Original Message-----
> From: Srikanth V, NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Sent: Monday, September 23, 2024 11:59 AM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; 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>
> Subject: RE: [PATCH 2/3] drm/i915/dp: read Aux RD interval after reading the
> FFE preset
> 
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > Arun R Murthy
> > > Sent: Thursday, September 12, 2024 10:36 AM
> > > To: 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>
> > > Subject: [PATCH 2/3] drm/i915/dp: read Aux RD interval after reading
> > > the FFE preset
> > >
> > > DP Source should be reading AUX_RD interval after we get adjusted
> > > TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting in
> > > DP
> > > Source)
> >
> > I think mentioning the dp spec reference here would be helpful
> >
> Please refer to Figure 3-52: 128b132b DP DPTC LANEx_CHANNEL_EQ_DONE
> Sequence of DP2.1a spec.
> 
> > >
> > > Signed-off-by: Srikanth V NagaVenkata
> > > <nagavenkata.srikanth.v@intel.com>
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 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..ca179bed46ad 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;
> > > @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> > > *intel_dp,
> > >  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX
> > FFE
> > > settings\n");
> > >  			return false;
> > >  		}
> > > +
> > > +		/*
> > > +		 * The delay may get updated. The transmitter shall read the
> > > +		 * delay before link status during link training.
> > > +		 */
> >
> > The comment needs to be updated as this is not being done before link
> > status Also a question does this not conflict with the requirement we
> > previously had (reading it before link status) ?
> >

Also  this whole delay us read should be called much below in the sequence from what I can see
In the dp spec just before we adjust the ffe settings at this point

/* Update signal levels and training set as requested. */
                intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, link_status);
                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;
                }

Regards,
Suraj Kandpal

> > Regards,
> > Suraj Kandpal
> >
> > Regards,
> > Suraj Kandpal
> >
> > > +		delay_us =
> > > drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
> > >  	}
> > >
> > >  	if (try == max_tries) {
> > > --
> > > 2.25.1
Jani Nikula Sept. 23, 2024, 11:09 a.m. UTC | #8 
On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> DP Source should be reading AUX_RD interval after we get adjusted
> TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting
> in DP Source)

I don't think that's correct. See below.

> Signed-off-by: Srikanth V NagaVenkata <nagavenkata.srikanth.v@intel.com>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 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..ca179bed46ad 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;
> @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
>  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX FFE settings\n");
>  			return false;
>  		}
> +
> +		/*
> +		 * 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);

This is clearly an improvement, but Figure 3-52 of DP 2.1a has "Read
AUX_RD_INTERVAL value" before "Adjust requested the TX_FFE_PRESET_VALUE
by a DPRX/LTTPR_UFP setting". Yes, in the same box in the flow chart,
but before.

Sticking with the spec, the read should be placed above this comment:

	/* Update signal levels and training set as requested. */

Be sure to reference the spec in the commit message.


BR,
Jani.


>  	}
>  
>  	if (try == max_tries) {
Murthy, Arun R Sept. 24, 2024, 5:58 a.m. UTC | #9 
> > > > -		/*
> > > > -		 * 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;
> > > > @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> > > > *intel_dp,
> > > >  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX
> > > FFE
> > > > settings\n");
> > > >  			return false;
> > > >  		}
> > > > +
> > > > +		/*
> > > > +		 * The delay may get updated. The transmitter shall read the
> > > > +		 * delay before link status during link training.
> > > > +		 */
> > >
> > > The comment needs to be updated as this is not being done before
> > > link status Also a question does this not conflict with the
> > > requirement we previously had (reading it before link status) ?
> > >
> 
> Also  this whole delay us read should be called much below in the sequence
> from what I can see In the dp spec just before we adjust the ffe settings at this
> point
> 
> /* Update signal levels and training set as requested. */
>                 intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX,
> link_status);
>                 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;
>                 }
> 
Yes as per the spec,
" During LT, the transmitter shall read DPCD 02216h before DPCD 00202h through 00207h, and 0200Ch through 0200Fh."

Will change it accordingly.

Thanks and Regards,
Arun R Murthy
--------------------
Murthy, Arun R Sept. 24, 2024, 6 a.m. UTC | #10 
> On Thu, 12 Sep 2024, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> > DP Source should be reading AUX_RD interval after we get adjusted
> > TX_FFE_PRESET_VALUE from the DP Sink. (before actually adjusting in DP
> > Source)
> 
> I don't think that's correct. See below.
> 
Will correct the statement.

> > Signed-off-by: Srikanth V NagaVenkata
> > <nagavenkata.srikanth.v@intel.com>
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 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..ca179bed46ad 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;
> > @@ -1457,6 +1451,12 @@ intel_dp_128b132b_lane_eq(struct intel_dp
> *intel_dp,
> >  			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX
> FFE settings\n");
> >  			return false;
> >  		}
> > +
> > +		/*
> > +		 * 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);
> 
> This is clearly an improvement, but Figure 3-52 of DP 2.1a has "Read
> AUX_RD_INTERVAL value" before "Adjust requested the TX_FFE_PRESET_VALUE
> by a DPRX/LTTPR_UFP setting". Yes, in the same box in the flow chart, but
> before.
> 
> Sticking with the spec, the read should be placed above this comment:
> 
> 	/* Update signal levels and training set as requested. */
> 
> Be sure to reference the spec in the commit message.
> 
Yes, agree Will update this in the next version.

Thanks and Regards,
Arun R Murthy
--------------------
diff mbox series

Patch

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..ca179bed46ad 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;
@@ -1457,6 +1451,12 @@  intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
 			lt_err(intel_dp, DP_PHY_DPRX, "Failed to update TX FFE settings\n");
 			return false;
 		}
+
+		/*
+		 * 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 (try == max_tries) {