diff mbox series

[5/7] drm/i915: Reverse preemph vs. voltage swing preference

Message ID 20200512174145.3186-6-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: DP vswing/preemph fixes | expand

Commit Message

Ville Syrjälä May 12, 2020, 5:41 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The DP spec says:
"When the combination of the requested pre-emphasis level and
 voltage swing exceeds the capability of a DPTX, the DPTX shall
 set the pre-emphasis level according to the request and use the
 highest voltage swing it can output with the given pre-emphasis level."
and
"When a DPTX reads a request beyond the limits of this Standard,
 the DPTX shall set the pre-emphasis level according to the request
 and set the highest voltage swing level it can output with the
 given pre-emphasis level. If a DPTX is requested for 9.5dB of
 pre-emphasis level (may be supported for a DPTX) and cannot support
 that level, it shall set the pre-emphasis level to the next
 highest level, 6dB."

Ie. we should first validate the pre-emphasis, and then select
the appropriate vswing for it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/intel_dp_link_training.c | 32 +++++++++----------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Navare, Manasi May 15, 2020, 7:18 p.m. UTC | #1 
On Tue, May 12, 2020 at 08:41:43PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The DP spec says:
> "When the combination of the requested pre-emphasis level and
>  voltage swing exceeds the capability of a DPTX, the DPTX shall
>  set the pre-emphasis level according to the request and use the
>  highest voltage swing it can output with the given pre-emphasis level."
> and
> "When a DPTX reads a request beyond the limits of this Standard,
>  the DPTX shall set the pre-emphasis level according to the request
>  and set the highest voltage swing level it can output with the
>  given pre-emphasis level. If a DPTX is requested for 9.5dB of
>  pre-emphasis level (may be supported for a DPTX) and cannot support
>  that level, it shall set the pre-emphasis level to the next
>  highest level, 6dB."
> 
> Ie. we should first validate the pre-emphasis, and then select
> the appropriate vswing for it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

So basically reverse the logic for selecting the vswing and pre emphasis

> ---
>  .../drm/i915/display/intel_dp_link_training.c | 32 +++++++++----------
>  1 file changed, 16 insertions(+), 16 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 171d9e842fc0..573f93779449 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -34,18 +34,18 @@ intel_dp_dump_link_status(const u8 link_status[DP_LINK_STATUS_SIZE])
>  		      link_status[3], link_status[4], link_status[5]);
>  }
>  
> -static u8 dp_pre_emphasis_max(u8 voltage_swing)
> +static u8 dp_voltage_max(u8 preemph)
>  {
> -	switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
> -		return DP_TRAIN_PRE_EMPH_LEVEL_3;
> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
> -		return DP_TRAIN_PRE_EMPH_LEVEL_2;
> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
> -		return DP_TRAIN_PRE_EMPH_LEVEL_1;
> -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
> +	switch (preemph & DP_TRAIN_PRE_EMPHASIS_MASK) {
> +	case DP_TRAIN_PRE_EMPH_LEVEL_0:
> +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
> +	case DP_TRAIN_PRE_EMPH_LEVEL_1:
> +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
> +	case DP_TRAIN_PRE_EMPH_LEVEL_2:
> +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_1;
> +	case DP_TRAIN_PRE_EMPH_LEVEL_3:
>  	default:
> -		return DP_TRAIN_PRE_EMPH_LEVEL_0;
> +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_0;

These vswing levels for that specific pre emph level comes from the Bspec
or from the DP spec? It wasnt clear to me how level3 of vswing was the max for pre emphasis level 0 and all others?

Manasi

>  	}
>  }
>  
> @@ -68,15 +68,15 @@ void intel_dp_get_adjust_train(struct intel_dp *intel_dp,
>  			p = this_p;
>  	}
>  
> -	voltage_max = intel_dp->voltage_max(intel_dp);
> -	if (v >= voltage_max)
> -		v = voltage_max | DP_TRAIN_MAX_SWING_REACHED;
> -
> -	preemph_max = min(intel_dp->preemph_max(intel_dp),
> -			  dp_pre_emphasis_max(v));
> +	preemph_max = intel_dp->preemph_max(intel_dp);
>  	if (p >= preemph_max)
>  		p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>  
> +	voltage_max = min(intel_dp->voltage_max(intel_dp),
> +			  dp_voltage_max(p));
> +	if (v >= voltage_max)
> +		v = voltage_max | DP_TRAIN_MAX_SWING_REACHED;
> +
>  	for (lane = 0; lane < 4; lane++)
>  		intel_dp->train_set[lane] = v | p;
>  }
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä May 15, 2020, 7:59 p.m. UTC | #2 
On Fri, May 15, 2020 at 12:18:22PM -0700, Manasi Navare wrote:
> On Tue, May 12, 2020 at 08:41:43PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The DP spec says:
> > "When the combination of the requested pre-emphasis level and
> >  voltage swing exceeds the capability of a DPTX, the DPTX shall
> >  set the pre-emphasis level according to the request and use the
> >  highest voltage swing it can output with the given pre-emphasis level."
> > and
> > "When a DPTX reads a request beyond the limits of this Standard,
> >  the DPTX shall set the pre-emphasis level according to the request
> >  and set the highest voltage swing level it can output with the
> >  given pre-emphasis level. If a DPTX is requested for 9.5dB of
> >  pre-emphasis level (may be supported for a DPTX) and cannot support
> >  that level, it shall set the pre-emphasis level to the next
> >  highest level, 6dB."
> > 
> > Ie. we should first validate the pre-emphasis, and then select
> > the appropriate vswing for it.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> So basically reverse the logic for selecting the vswing and pre emphasis
> 
> > ---
> >  .../drm/i915/display/intel_dp_link_training.c | 32 +++++++++----------
> >  1 file changed, 16 insertions(+), 16 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 171d9e842fc0..573f93779449 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -34,18 +34,18 @@ intel_dp_dump_link_status(const u8 link_status[DP_LINK_STATUS_SIZE])
> >  		      link_status[3], link_status[4], link_status[5]);
> >  }
> >  
> > -static u8 dp_pre_emphasis_max(u8 voltage_swing)
> > +static u8 dp_voltage_max(u8 preemph)
> >  {
> > -	switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
> > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
> > -		return DP_TRAIN_PRE_EMPH_LEVEL_3;
> > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
> > -		return DP_TRAIN_PRE_EMPH_LEVEL_2;
> > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
> > -		return DP_TRAIN_PRE_EMPH_LEVEL_1;
> > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
> > +	switch (preemph & DP_TRAIN_PRE_EMPHASIS_MASK) {
> > +	case DP_TRAIN_PRE_EMPH_LEVEL_0:
> > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
> > +	case DP_TRAIN_PRE_EMPH_LEVEL_1:
> > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
> > +	case DP_TRAIN_PRE_EMPH_LEVEL_2:
> > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_1;
> > +	case DP_TRAIN_PRE_EMPH_LEVEL_3:
> >  	default:
> > -		return DP_TRAIN_PRE_EMPH_LEVEL_0;
> > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
> 
> These vswing levels for that specific pre emph level comes from the Bspec
> or from the DP spec? It wasnt clear to me how level3 of vswing was the max for pre emphasis level 0 and all others?

From DP 1.4 spec "Table 3-1: Allowed Vdiff_pre_pp and Pre-emphasis
Combinations"

Previosuly this was present in some semi-mangled way in each
platform's max preeph calculation. Now we just have one canonical
copy of it. Later on we could probably lift this into drm_dp_helper.
Navare, Manasi May 18, 2020, 6:20 p.m. UTC | #3 
On Fri, May 15, 2020 at 10:59:57PM +0300, Ville Syrjälä wrote:
> On Fri, May 15, 2020 at 12:18:22PM -0700, Manasi Navare wrote:
> > On Tue, May 12, 2020 at 08:41:43PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The DP spec says:
> > > "When the combination of the requested pre-emphasis level and
> > >  voltage swing exceeds the capability of a DPTX, the DPTX shall
> > >  set the pre-emphasis level according to the request and use the
> > >  highest voltage swing it can output with the given pre-emphasis level."
> > > and
> > > "When a DPTX reads a request beyond the limits of this Standard,
> > >  the DPTX shall set the pre-emphasis level according to the request
> > >  and set the highest voltage swing level it can output with the
> > >  given pre-emphasis level. If a DPTX is requested for 9.5dB of
> > >  pre-emphasis level (may be supported for a DPTX) and cannot support
> > >  that level, it shall set the pre-emphasis level to the next
> > >  highest level, 6dB."
> > > 
> > > Ie. we should first validate the pre-emphasis, and then select
> > > the appropriate vswing for it.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > So basically reverse the logic for selecting the vswing and pre emphasis
> > 
> > > ---
> > >  .../drm/i915/display/intel_dp_link_training.c | 32 +++++++++----------
> > >  1 file changed, 16 insertions(+), 16 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 171d9e842fc0..573f93779449 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > @@ -34,18 +34,18 @@ intel_dp_dump_link_status(const u8 link_status[DP_LINK_STATUS_SIZE])
> > >  		      link_status[3], link_status[4], link_status[5]);
> > >  }
> > >  
> > > -static u8 dp_pre_emphasis_max(u8 voltage_swing)
> > > +static u8 dp_voltage_max(u8 preemph)
> > >  {
> > > -	switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
> > > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
> > > -		return DP_TRAIN_PRE_EMPH_LEVEL_3;
> > > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
> > > -		return DP_TRAIN_PRE_EMPH_LEVEL_2;
> > > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
> > > -		return DP_TRAIN_PRE_EMPH_LEVEL_1;
> > > -	case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
> > > +	switch (preemph & DP_TRAIN_PRE_EMPHASIS_MASK) {
> > > +	case DP_TRAIN_PRE_EMPH_LEVEL_0:
> > > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
> > > +	case DP_TRAIN_PRE_EMPH_LEVEL_1:
> > > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
> > > +	case DP_TRAIN_PRE_EMPH_LEVEL_2:
> > > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_1;
> > > +	case DP_TRAIN_PRE_EMPH_LEVEL_3:
> > >  	default:
> > > -		return DP_TRAIN_PRE_EMPH_LEVEL_0;
> > > +		return DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
> > 
> > These vswing levels for that specific pre emph level comes from the Bspec
> > or from the DP spec? It wasnt clear to me how level3 of vswing was the max for pre emphasis level 0 and all others?
> 
> From DP 1.4 spec "Table 3-1: Allowed Vdiff_pre_pp and Pre-emphasis
> Combinations"
> 
> Previosuly this was present in some semi-mangled way in each
> platform's max preeph calculation. Now we just have one canonical
> copy of it. Later on we could probably lift this into drm_dp_helper.

Okay great yes confirmed from that table and looks good to me

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> 
> -- 
> Ville Syrjälä
> Intel
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 171d9e842fc0..573f93779449 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -34,18 +34,18 @@  intel_dp_dump_link_status(const u8 link_status[DP_LINK_STATUS_SIZE])
 		      link_status[3], link_status[4], link_status[5]);
 }
 
-static u8 dp_pre_emphasis_max(u8 voltage_swing)
+static u8 dp_voltage_max(u8 preemph)
 {
-	switch (voltage_swing & DP_TRAIN_VOLTAGE_SWING_MASK) {
-	case DP_TRAIN_VOLTAGE_SWING_LEVEL_0:
-		return DP_TRAIN_PRE_EMPH_LEVEL_3;
-	case DP_TRAIN_VOLTAGE_SWING_LEVEL_1:
-		return DP_TRAIN_PRE_EMPH_LEVEL_2;
-	case DP_TRAIN_VOLTAGE_SWING_LEVEL_2:
-		return DP_TRAIN_PRE_EMPH_LEVEL_1;
-	case DP_TRAIN_VOLTAGE_SWING_LEVEL_3:
+	switch (preemph & DP_TRAIN_PRE_EMPHASIS_MASK) {
+	case DP_TRAIN_PRE_EMPH_LEVEL_0:
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_3;
+	case DP_TRAIN_PRE_EMPH_LEVEL_1:
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
+	case DP_TRAIN_PRE_EMPH_LEVEL_2:
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_1;
+	case DP_TRAIN_PRE_EMPH_LEVEL_3:
 	default:
-		return DP_TRAIN_PRE_EMPH_LEVEL_0;
+		return DP_TRAIN_VOLTAGE_SWING_LEVEL_0;
 	}
 }
 
@@ -68,15 +68,15 @@  void intel_dp_get_adjust_train(struct intel_dp *intel_dp,
 			p = this_p;
 	}
 
-	voltage_max = intel_dp->voltage_max(intel_dp);
-	if (v >= voltage_max)
-		v = voltage_max | DP_TRAIN_MAX_SWING_REACHED;
-
-	preemph_max = min(intel_dp->preemph_max(intel_dp),
-			  dp_pre_emphasis_max(v));
+	preemph_max = intel_dp->preemph_max(intel_dp);
 	if (p >= preemph_max)
 		p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
 
+	voltage_max = min(intel_dp->voltage_max(intel_dp),
+			  dp_voltage_max(p));
+	if (v >= voltage_max)
+		v = voltage_max | DP_TRAIN_MAX_SWING_REACHED;
+
 	for (lane = 0; lane < 4; lane++)
 		intel_dp->train_set[lane] = v | p;
 }