diff mbox series

[RESEND,PATCHv2] drm/i915/display/dp: 128/132b LT requirement

Message ID 20230419022522.3457924-1-arun.r.murthy@intel.com (mailing list archive)
State New, archived
Headers show
Series [RESEND,PATCHv2] drm/i915/display/dp: 128/132b LT requirement | expand

Commit Message

Murthy, Arun R April 19, 2023, 2:25 a.m. UTC
For 128b/132b LT prior to LT DPTX should set power state, DP channel
coding and then link rate.

v2: added separate function to avoid code duplication(Jani N)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 .../drm/i915/display/intel_dp_link_training.c | 62 +++++++++++++------
 1 file changed, 44 insertions(+), 18 deletions(-)

Comments

Jani Nikula April 19, 2023, 7:18 a.m. UTC | #1
On Wed, 19 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.
>
> v2: added separate function to avoid code duplication(Jani N)
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>

RESEND for what reason?

Two v2 and neither fixes
https://lore.kernel.org/r/87o7nmergw.fsf@intel.com

BR,
Jani.


> ---
>  .../drm/i915/display/intel_dp_link_training.c | 62 +++++++++++++------
>  1 file changed, 44 insertions(+), 18 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..e5809cf7d0c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -637,6 +637,37 @@ static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
>  	return true;
>  }
>  
> +static void
> +intel_dp_update_downspread_ctrl(struct intel_dp *intel_dp,
> +				const struct intel_crtc_state *crtc_state)
> +{
> +	u8 link_config[2];
> +
> +	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);
> +}
> +
> +static void
> +intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
> +			    const struct intel_crtc_state *crtc_state,
> +			    u8 link_bw, u8 rate_select)
> +{
> +	u8 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);
> +}
> +
>  /*
>   * Prepare link training by configuring the link parameters. On DDI platforms
>   * also enable the port here.
> @@ -647,7 +678,6 @@ intel_dp_prepare_link_train(struct intel_dp *intel_dp,
>  {
>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> -	u8 link_config[2];
>  	u8 link_bw, rate_select;
>  
>  	if (intel_dp->prepare_link_retrain)
> @@ -686,23 +716,19 @@ 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 128b/132b DP Channel coding and then set link rate
> +		 */
> +		intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> +		intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> +					    rate_select);
> +	} else {
> +		intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> +					    rate_select);
> +		intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> +	}
>  
>  	return true;
>  }
Murthy, Arun R April 19, 2023, 8:03 a.m. UTC | #2
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Wednesday, April 19, 2023 12:48 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: Re: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
> requirement
> 
> On Wed, 19 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.
> >
> > v2: added separate function to avoid code duplication(Jani N)
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> 
> RESEND for what reason?
Typo is sending V2 patch hence corrected and sent it again.

> 
> Two v2 and neither fixes
> https://lore.kernel.org/r/87o7nmergw.fsf@intel.com
This is pointing to the v1 patch.
V2 patch addressing review comments can be located @ https://lore.kernel.org/all/20230419022522.3457924-1-arun.r.murthy@intel.com/

Thanks and Regards,
Arun R Murthy
--------------------
> 
> BR,
> Jani.
> 
> 
> > ---
> >  .../drm/i915/display/intel_dp_link_training.c | 62
> > +++++++++++++------
> >  1 file changed, 44 insertions(+), 18 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..e5809cf7d0c4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -637,6 +637,37 @@ static bool
> intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
> >  	return true;
> >  }
> >
> > +static void
> > +intel_dp_update_downspread_ctrl(struct intel_dp *intel_dp,
> > +				const struct intel_crtc_state *crtc_state) {
> > +	u8 link_config[2];
> > +
> > +	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); }
> > +
> > +static void
> > +intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
> > +			    const struct intel_crtc_state *crtc_state,
> > +			    u8 link_bw, u8 rate_select)
> > +{
> > +	u8 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);
> > +}
> > +
> >  /*
> >   * Prepare link training by configuring the link parameters. On DDI
> platforms
> >   * also enable the port here.
> > @@ -647,7 +678,6 @@ intel_dp_prepare_link_train(struct intel_dp
> > *intel_dp,  {
> >  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> >  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > -	u8 link_config[2];
> >  	u8 link_bw, rate_select;
> >
> >  	if (intel_dp->prepare_link_retrain)
> > @@ -686,23 +716,19 @@ 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 128b/132b DP Channel coding
> and then set link rate
> > +		 */
> > +		intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> > +		intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> > +					    rate_select);
> > +	} else {
> > +		intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> > +					    rate_select);
> > +		intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> > +	}
> >
> >  	return true;
> >  }
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
Jani Nikula April 19, 2023, 9:55 a.m. UTC | #3
On Wed, 19 Apr 2023, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani <jani.nikula@intel.com>
>> Sent: Wednesday, April 19, 2023 12:48 PM
>> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
>> gfx@lists.freedesktop.org
>> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
>> Subject: Re: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
>> requirement
>>
>> On Wed, 19 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.
>> >
>> > v2: added separate function to avoid code duplication(Jani N)
>> >
>> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>>
>> RESEND for what reason?
> Typo is sending V2 patch hence corrected and sent it again.
>
>>
>> Two v2 and neither fixes
>> https://lore.kernel.org/r/87o7nmergw.fsf@intel.com
> This is pointing to the v1 patch.
> V2 patch addressing review comments can be located @ https://lore.kernel.org/all/20230419022522.3457924-1-arun.r.murthy@intel.com/

Argh.

RESEND means you're sending the exact same patch again. Hence
*re-send*. That's what I thought. That's what everyone would think.

It's even documented in submitting-patches.rst [1].

---

There's still the question of whether we could just change the order for
8b/10b too [2]. On IRC, Ville thinks we could, "i don't think there is
any order specified. just use the same alwas imo".


BR,
Jani.


[1] https://docs.kernel.org/process/submitting-patches.html#don-t-get-discouraged-or-impatient
[2] https://lore.kernel.org/r/87r0siernf.fsf@intel.com






>
> Thanks and Regards,
> Arun R Murthy
> --------------------
>>
>> BR,
>> Jani.
>>
>>
>> > ---
>> >  .../drm/i915/display/intel_dp_link_training.c | 62
>> > +++++++++++++------
>> >  1 file changed, 44 insertions(+), 18 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..e5809cf7d0c4 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> > @@ -637,6 +637,37 @@ static bool
>> intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
>> >     return true;
>> >  }
>> >
>> > +static void
>> > +intel_dp_update_downspread_ctrl(struct intel_dp *intel_dp,
>> > +                           const struct intel_crtc_state *crtc_state) {
>> > +   u8 link_config[2];
>> > +
>> > +   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); }
>> > +
>> > +static void
>> > +intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
>> > +                       const struct intel_crtc_state *crtc_state,
>> > +                       u8 link_bw, u8 rate_select)
>> > +{
>> > +   u8 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);
>> > +}
>> > +
>> >  /*
>> >   * Prepare link training by configuring the link parameters. On DDI
>> platforms
>> >   * also enable the port here.
>> > @@ -647,7 +678,6 @@ intel_dp_prepare_link_train(struct intel_dp
>> > *intel_dp,  {
>> >     struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
>> >     struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> > -   u8 link_config[2];
>> >     u8 link_bw, rate_select;
>> >
>> >     if (intel_dp->prepare_link_retrain)
>> > @@ -686,23 +716,19 @@ 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 128b/132b DP Channel coding
>> and then set link rate
>> > +            */
>> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
>> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
>> > +                                       rate_select);
>> > +   } else {
>> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
>> > +                                       rate_select);
>> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
>> > +   }
>> >
>> >     return true;
>> >  }
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
Murthy, Arun R April 19, 2023, 10:07 a.m. UTC | #4
> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Wednesday, April 19, 2023 3:26 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com
> Subject: RE: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
> requirement
> 
> On Wed, 19 Apr 2023, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula@intel.com>
> >> Sent: Wednesday, April 19, 2023 12:48 PM
> >> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> >> gfx@lists.freedesktop.org
> >> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> >> Subject: Re: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
> >> requirement
> >>
> >> On Wed, 19 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.
> >> >
> >> > v2: added separate function to avoid code duplication(Jani N)
> >> >
> >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> >>
> >> RESEND for what reason?
> > Typo is sending V2 patch hence corrected and sent it again.
> >
> >>
> >> Two v2 and neither fixes
> >> https://lore.kernel.org/r/87o7nmergw.fsf@intel.com
> > This is pointing to the v1 patch.
> > V2 patch addressing review comments can be located @
> > https://lore.kernel.org/all/20230419022522.3457924-1-arun.r.murthy@int
> > el.com/
> 
> Argh.
> 
> RESEND means you're sending the exact same patch again. Hence *re-send*.
> That's what I thought. That's what everyone would think.
> 
> It's even documented in submitting-patches.rst [1].
> 
> ---
> 
> There's still the question of whether we could just change the order for
> 8b/10b too [2]. On IRC, Ville thinks we could, "i don't think there is any order
> specified. just use the same alwas imo".
> 
Spec DP2.1 section 3.5.1.2 (for 8b/10b LT)
write the following Link Configuration parameters:
* LINK_BW_SET register (DPCD 00100h)
* LANE_COUNT_SET field in the LANE_COUNT_SET register (DPCD 00101h[4:0])
* DOWNSPREAD_CTRL register (DPCD 00107h)
* MAIN_LINK_CHANNEL_CODING_SET register (DPCD 00108h)

Whereas for 128b/132b section 3.5.2.16 says
Prior to link training, a DPTX should perform the following:
1 Verify that the SET_POWER_STATE field in the
SET_POWER_AND_SET_DP_PWR_VOLTAGE register is programmed to D0 normal
operation (DPCD 00600h[2:0] = 001b).
2 Write DPCD 00108h = 02h to select 128b/132b DP channel coding.
3 Program the target link rate and lane count by way of an AUX write transaction to
DPCD 00100h and 00101h, respectively


Thanks and Regards,
Arun R Murthy
-------------------
> 
> BR,
> Jani.
> 
> 
> [1] https://docs.kernel.org/process/submitting-patches.html#don-t-get-
> discouraged-or-impatient
> [2] https://lore.kernel.org/r/87r0siernf.fsf@intel.com
> 
> 
> 
> 
> 
> 
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> > ---
> >> >  .../drm/i915/display/intel_dp_link_training.c | 62
> >> > +++++++++++++------
> >> >  1 file changed, 44 insertions(+), 18 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..e5809cf7d0c4 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> >> > @@ -637,6 +637,37 @@ static bool
> >> intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
> >> >     return true;
> >> >  }
> >> >
> >> > +static void
> >> > +intel_dp_update_downspread_ctrl(struct intel_dp *intel_dp,
> >> > +                           const struct intel_crtc_state *crtc_state) {
> >> > +   u8 link_config[2];
> >> > +
> >> > +   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); }
> >> > +
> >> > +static void
> >> > +intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
> >> > +                       const struct intel_crtc_state *crtc_state,
> >> > +                       u8 link_bw, u8 rate_select) {
> >> > +   u8 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); }
> >> > +
> >> >  /*
> >> >   * Prepare link training by configuring the link parameters. On
> >> > DDI
> >> platforms
> >> >   * also enable the port here.
> >> > @@ -647,7 +678,6 @@ intel_dp_prepare_link_train(struct intel_dp
> >> > *intel_dp,  {
> >> >     struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> >> >     struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> >> > -   u8 link_config[2];
> >> >     u8 link_bw, rate_select;
> >> >
> >> >     if (intel_dp->prepare_link_retrain) @@ -686,23 +716,19 @@
> >> > 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 128b/132b DP Channel
> >> > + coding
> >> and then set link rate
> >> > +            */
> >> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> >> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> >> > +                                       rate_select);
> >> > +   } else {
> >> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> >> > +                                       rate_select);
> >> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> >> > +   }
> >> >
> >> >     return true;
> >> >  }
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> --
> Jani Nikula, Intel Open Source Graphics Center
Ville Syrjälä April 24, 2023, 3:27 p.m. UTC | #5
On Wed, Apr 19, 2023 at 10:07:46AM +0000, Murthy, Arun R wrote:
> > -----Original Message-----
> > From: Nikula, Jani <jani.nikula@intel.com>
> > Sent: Wednesday, April 19, 2023 3:26 PM
> > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: ville.syrjala@linux.intel.com
> > Subject: RE: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
> > requirement
> > 
> > On Wed, 19 Apr 2023, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
> > >> -----Original Message-----
> > >> From: Nikula, Jani <jani.nikula@intel.com>
> > >> Sent: Wednesday, April 19, 2023 12:48 PM
> > >> To: Murthy, Arun R <arun.r.murthy@intel.com>; intel-
> > >> gfx@lists.freedesktop.org
> > >> Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > >> Subject: Re: [RESEND PATCHv2] drm/i915/display/dp: 128/132b LT
> > >> requirement
> > >>
> > >> On Wed, 19 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.
> > >> >
> > >> > v2: added separate function to avoid code duplication(Jani N)
> > >> >
> > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > >>
> > >> RESEND for what reason?
> > > Typo is sending V2 patch hence corrected and sent it again.
> > >
> > >>
> > >> Two v2 and neither fixes
> > >> https://lore.kernel.org/r/87o7nmergw.fsf@intel.com
> > > This is pointing to the v1 patch.
> > > V2 patch addressing review comments can be located @
> > > https://lore.kernel.org/all/20230419022522.3457924-1-arun.r.murthy@int
> > > el.com/
> > 
> > Argh.
> > 
> > RESEND means you're sending the exact same patch again. Hence *re-send*.
> > That's what I thought. That's what everyone would think.
> > 
> > It's even documented in submitting-patches.rst [1].
> > 
> > ---
> > 
> > There's still the question of whether we could just change the order for
> > 8b/10b too [2]. On IRC, Ville thinks we could, "i don't think there is any order
> > specified. just use the same alwas imo".
> > 
> Spec DP2.1 section 3.5.1.2 (for 8b/10b LT)
> write the following Link Configuration parameters:
> * LINK_BW_SET register (DPCD 00100h)
> * LANE_COUNT_SET field in the LANE_COUNT_SET register (DPCD 00101h[4:0])
> * DOWNSPREAD_CTRL register (DPCD 00107h)
> * MAIN_LINK_CHANNEL_CODING_SET register (DPCD 00108h)

Looks like an unordered list to me

> 
> Whereas for 128b/132b section 3.5.2.16 says
> Prior to link training, a DPTX should perform the following:
> 1 Verify that the SET_POWER_STATE field in the
> SET_POWER_AND_SET_DP_PWR_VOLTAGE register is programmed to D0 normal
> operation (DPCD 00600h[2:0] = 001b).
> 2 Write DPCD 00108h = 02h to select 128b/132b DP channel coding.
> 3 Program the target link rate and lane count by way of an AUX write transaction to
> DPCD 00100h and 00101h, respectively

whereas this is an ordered list.

> 
> 
> Thanks and Regards,
> Arun R Murthy
> -------------------
> > 
> > BR,
> > Jani.
> > 
> > 
> > [1] https://docs.kernel.org/process/submitting-patches.html#don-t-get-
> > discouraged-or-impatient
> > [2] https://lore.kernel.org/r/87r0siernf.fsf@intel.com
> > 
> > 
> > 
> > 
> > 
> > 
> > >
> > > Thanks and Regards,
> > > Arun R Murthy
> > > --------------------
> > >>
> > >> BR,
> > >> Jani.
> > >>
> > >>
> > >> > ---
> > >> >  .../drm/i915/display/intel_dp_link_training.c | 62
> > >> > +++++++++++++------
> > >> >  1 file changed, 44 insertions(+), 18 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..e5809cf7d0c4 100644
> > >> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > >> > @@ -637,6 +637,37 @@ static bool
> > >> intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
> > >> >     return true;
> > >> >  }
> > >> >
> > >> > +static void
> > >> > +intel_dp_update_downspread_ctrl(struct intel_dp *intel_dp,
> > >> > +                           const struct intel_crtc_state *crtc_state) {
> > >> > +   u8 link_config[2];
> > >> > +
> > >> > +   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); }
> > >> > +
> > >> > +static void
> > >> > +intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
> > >> > +                       const struct intel_crtc_state *crtc_state,
> > >> > +                       u8 link_bw, u8 rate_select) {
> > >> > +   u8 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); }
> > >> > +
> > >> >  /*
> > >> >   * Prepare link training by configuring the link parameters. On
> > >> > DDI
> > >> platforms
> > >> >   * also enable the port here.
> > >> > @@ -647,7 +678,6 @@ intel_dp_prepare_link_train(struct intel_dp
> > >> > *intel_dp,  {
> > >> >     struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > >> >     struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > >> > -   u8 link_config[2];
> > >> >     u8 link_bw, rate_select;
> > >> >
> > >> >     if (intel_dp->prepare_link_retrain) @@ -686,23 +716,19 @@
> > >> > 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 128b/132b DP Channel
> > >> > + coding
> > >> and then set link rate
> > >> > +            */
> > >> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> > >> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> > >> > +                                       rate_select);
> > >> > +   } else {
> > >> > +           intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
> > >> > +                                       rate_select);
> > >> > +           intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
> > >> > +   }
> > >> >
> > >> >     return true;
> > >> >  }
> > >>
> > >> --
> > >> Jani Nikula, Intel Open Source Graphics Center
> > 
> > --
> > Jani Nikula, Intel Open Source Graphics Center
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 6aa4ae5e7ebe..e5809cf7d0c4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -637,6 +637,37 @@  static bool intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp,
 	return true;
 }
 
+static void
+intel_dp_update_downspread_ctrl(struct intel_dp *intel_dp,
+				const struct intel_crtc_state *crtc_state)
+{
+	u8 link_config[2];
+
+	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);
+}
+
+static void
+intel_dp_update_link_bw_set(struct intel_dp *intel_dp,
+			    const struct intel_crtc_state *crtc_state,
+			    u8 link_bw, u8 rate_select)
+{
+	u8 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);
+}
+
 /*
  * Prepare link training by configuring the link parameters. On DDI platforms
  * also enable the port here.
@@ -647,7 +678,6 @@  intel_dp_prepare_link_train(struct intel_dp *intel_dp,
 {
 	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
-	u8 link_config[2];
 	u8 link_bw, rate_select;
 
 	if (intel_dp->prepare_link_retrain)
@@ -686,23 +716,19 @@  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 128b/132b DP Channel coding and then set link rate
+		 */
+		intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
+		intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
+					    rate_select);
+	} else {
+		intel_dp_update_link_bw_set(intel_dp, crtc_state, link_bw,
+					    rate_select);
+		intel_dp_update_downspread_ctrl(intel_dp, crtc_state);
+	}
 
 	return true;
 }