[v4] drm/i915: Disable fast link training if DP config changes

Submitted by Mika Kahola on Dec. 17, 2015, 12:46 p.m.

Details

Message ID 1450356363-32315-1-git-send-email-mika.kahola@intel.com
State New
Headers show
Series "drm/i915: Disable fast link training if DP config changes" ( rev: 3 ) in Intel GFX

Browsing this patch as part of:
"drm/i915: Disable fast link training if DP config changes" rev 3 in Intel GFX
<< prev patch [1/1] next patch >>

Commit Message

Mika Kahola Dec. 17, 2015, 12:46 p.m.
Disable DP fast link training if DP link configuration
changes. If one of the DP link parameters i.e. link
bandwidth, lane count, rate selection, port clock or bpp
changes the link training does no longer apply the
previously computed voltage swing and pre-emphasis values.
Instead, the link training is started with zero values.

v4: Parameter and debug message naming improvements.
    Fix for link parameter check (Ville)

v3: Remove cached old link parameters. Instead, disable
    fast link training feature when link parameters are
    set (Ville)

v2: Readout DPCD register to check if no aux handshaking is
    required in link training (Ander)

Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c               |  8 +++++++-
 drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h              |  1 +
 3 files changed, 25 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 31ba241..82504f0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1677,6 +1677,12 @@  found:
 void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      const struct intel_crtc_state *pipe_config)
 {
+	if (intel_dp->link_rate != pipe_config->port_clock ||
+	    intel_dp->lane_count != pipe_config->lane_count) {
+		intel_dp->train_set_valid = false;
+		DRM_DEBUG_KMS("setting train set valid as false\n");
+	}
+
 	intel_dp->link_rate = pipe_config->port_clock;
 	intel_dp->lane_count = pipe_config->lane_count;
 }
@@ -3849,7 +3855,7 @@  intel_dp_link_down(struct intel_dp *intel_dp)
 	intel_dp->DP = DP;
 }
 
-static bool
+bool
 intel_dp_get_dpcd(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 8888793..e46bad6 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -85,6 +85,23 @@  static bool
 intel_dp_reset_link_train(struct intel_dp *intel_dp,
 			uint8_t dp_train_pat)
 {
+	bool has_dpcd;
+	bool no_aux_handshake = false;
+
+	has_dpcd = intel_dp_get_dpcd(intel_dp);
+
+	if (has_dpcd) {
+		if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
+			no_aux_handshake = (intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
+					    DP_NO_AUX_HANDSHAKE_LINK_TRAINING);
+		}
+	}
+
+	intel_dp->train_set_valid &= no_aux_handshake;
+
+	DRM_DEBUG_KMS("link training optimization: %s\n",
+		      intel_dp->train_set_valid ? "true" : "false");
+
 	if (!intel_dp->train_set_valid)
 		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
 	intel_dp_set_signal_levels(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 798463e..94041fd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1239,6 +1239,7 @@  int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config);
 bool intel_dp_is_edp(struct drm_device *dev, enum port port);
+bool intel_dp_get_dpcd(struct intel_dp *intel_dp);
 enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
 				  bool long_hpd);
 void intel_edp_backlight_on(struct intel_dp *intel_dp);

Comments

On Thu, Dec 17, 2015 at 02:46:03PM +0200, Mika Kahola wrote:
> Disable DP fast link training if DP link configuration
> changes. If one of the DP link parameters i.e. link
> bandwidth, lane count, rate selection, port clock or bpp

This list of things we check should be updated to match the actual
patch.

> changes the link training does no longer apply the
> previously computed voltage swing and pre-emphasis values.
> Instead, the link training is started with zero values.
> 
> v4: Parameter and debug message naming improvements.
>     Fix for link parameter check (Ville)
> 
> v3: Remove cached old link parameters. Instead, disable
>     fast link training feature when link parameters are
>     set (Ville)
> 
> v2: Readout DPCD register to check if no aux handshaking is
>     required in link training (Ander)
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c               |  8 +++++++-
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h              |  1 +
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 31ba241..82504f0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1677,6 +1677,12 @@ found:
>  void intel_dp_set_link_params(struct intel_dp *intel_dp,
>  			      const struct intel_crtc_state *pipe_config)
>  {
> +	if (intel_dp->link_rate != pipe_config->port_clock ||
> +	    intel_dp->lane_count != pipe_config->lane_count) {
> +		intel_dp->train_set_valid = false;
> +		DRM_DEBUG_KMS("setting train set valid as false\n");

Maybe "Link parameters changed, resetting vswing and pre-emphasis" ?

Also maybe check for train_set_valid being true as well so that we
don't print the debug message unless we're really resetting the flag.

> +	}
> +
>  	intel_dp->link_rate = pipe_config->port_clock;
>  	intel_dp->lane_count = pipe_config->lane_count;
>  }
> @@ -3849,7 +3855,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	intel_dp->DP = DP;
>  }
>  
> -static bool
> +bool
>  intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 8888793..e46bad6 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -85,6 +85,23 @@ static bool
>  intel_dp_reset_link_train(struct intel_dp *intel_dp,
>  			uint8_t dp_train_pat)
>  {
> +	bool has_dpcd;
> +	bool no_aux_handshake = false;
> +
> +	has_dpcd = intel_dp_get_dpcd(intel_dp);
> +
> +	if (has_dpcd) {
> +		if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
> +			no_aux_handshake = (intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> +					    DP_NO_AUX_HANDSHAKE_LINK_TRAINING);
> +		}
> +	}
> +
> +	intel_dp->train_set_valid &= no_aux_handshake;

This should be separate patch. And I'm not convinced it's correct
anyway. At least it's not anything mandated by the spec AFAICS, so
there should be a comment why we're doing it. Also there's no
mention of  this change in the commit message either.

> +
> +	DRM_DEBUG_KMS("link training optimization: %s\n",
> +		      intel_dp->train_set_valid ? "true" : "false");
> +
>  	if (!intel_dp->train_set_valid)
>  		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
>  	intel_dp_set_signal_levels(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 798463e..94041fd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1239,6 +1239,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config);
>  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> +bool intel_dp_get_dpcd(struct intel_dp *intel_dp);
>  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
>  				  bool long_hpd);
>  void intel_edp_backlight_on(struct intel_dp *intel_dp);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2015-12-17 at 15:16 +0200, Ville Syrjälä wrote:
> On Thu, Dec 17, 2015 at 02:46:03PM +0200, Mika Kahola wrote:
> > Disable DP fast link training if DP link configuration
> > changes. If one of the DP link parameters i.e. link
> > bandwidth, lane count, rate selection, port clock or bpp
> 
> This list of things we check should be updated to match the actual
> patch.
> 
Yes, I need to update the commit message as the content of the patch
changed a bit.

> > changes the link training does no longer apply the
> > previously computed voltage swing and pre-emphasis values.
> > Instead, the link training is started with zero values.
> > 
> > v4: Parameter and debug message naming improvements.
> >     Fix for link parameter check (Ville)
> > 
> > v3: Remove cached old link parameters. Instead, disable
> >     fast link training feature when link parameters are
> >     set (Ville)
> > 
> > v2: Readout DPCD register to check if no aux handshaking is
> >     required in link training (Ander)
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c               |  8 +++++++-
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h              |  1 +
> >  3 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 31ba241..82504f0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1677,6 +1677,12 @@ found:
> >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
> >  			      const struct intel_crtc_state *pipe_config)
> >  {
> > +	if (intel_dp->link_rate != pipe_config->port_clock ||
> > +	    intel_dp->lane_count != pipe_config->lane_count) {
> > +		intel_dp->train_set_valid = false;
> > +		DRM_DEBUG_KMS("setting train set valid as false\n");
> 
> Maybe "Link parameters changed, resetting vswing and pre-emphasis" ?
> 
> Also maybe check for train_set_valid being true as well so that we
> don't print the debug message unless we're really resetting the flag.
> 
Actually, I forgot to clean up this debug message. This was only
intended for my personal use when I did the testing. I'll remove this
debug message. Very good catch!

> > +	}
> > +
> >  	intel_dp->link_rate = pipe_config->port_clock;
> >  	intel_dp->lane_count = pipe_config->lane_count;
> >  }
> > @@ -3849,7 +3855,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> >  	intel_dp->DP = DP;
> >  }
> >  
> > -static bool
> > +bool
> >  intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 8888793..e46bad6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -85,6 +85,23 @@ static bool
> >  intel_dp_reset_link_train(struct intel_dp *intel_dp,
> >  			uint8_t dp_train_pat)
> >  {
> > +	bool has_dpcd;
> > +	bool no_aux_handshake = false;
> > +
> > +	has_dpcd = intel_dp_get_dpcd(intel_dp);
> > +
> > +	if (has_dpcd) {
> > +		if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
> > +			no_aux_handshake = (intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> > +					    DP_NO_AUX_HANDSHAKE_LINK_TRAINING);
> > +		}
> > +	}
> > +
> > +	intel_dp->train_set_valid &= no_aux_handshake;
> 
> This should be separate patch. And I'm not convinced it's correct
> anyway. At least it's not anything mandated by the spec AFAICS, so
> there should be a comment why we're doing it. Also there's no
> mention of  this change in the commit message either.
> 
I could split this patch in two parts and leave this part as a follow
up.

The spec says

"When DP Source device is resuming the transmission (e.g., after waking
from sleep), the Source device may skip the AUX transactions for the
link training if the following conditions are met:

 - The Source device has determined that the HPD signal has remained
asserted continuously (apart from IRQ_HPD notifications) since the link
was last in full operation.
- The Source device has read 1 in NO_AUX_TRANSACTION_LINK_TRAINING bit
after the initial Sink device detection"

My interpretation of the spec is that we would need to check this bit
just to ensure that we are capable of doing link training without AUX
transactions. Without this check the we are probably too optimistic of
training the link by reusing the known good drive current and
pre-emphasis level. 

BTW, should we change the naming of the define
"DP_NO_AUX_HANDSHAKE_LINK_TRAINING" as the spec discusses of
"DP_NO_AUX_TRANSACTION_LINK_TRAINING"?

> > +
> > +	DRM_DEBUG_KMS("link training optimization: %s\n",
> > +		      intel_dp->train_set_valid ? "true" : "false");
> > +
> >  	if (!intel_dp->train_set_valid)
> >  		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> >  	intel_dp_set_signal_levels(intel_dp);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 798463e..94041fd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1239,6 +1239,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
> >  bool intel_dp_compute_config(struct intel_encoder *encoder,
> >  			     struct intel_crtc_state *pipe_config);
> >  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> > +bool intel_dp_get_dpcd(struct intel_dp *intel_dp);
> >  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
> >  				  bool long_hpd);
> >  void intel_edp_backlight_on(struct intel_dp *intel_dp);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
On Fri, Dec 18, 2015 at 10:37:26AM +0200, Mika Kahola wrote:
> On Thu, 2015-12-17 at 15:16 +0200, Ville Syrjälä wrote:
> > On Thu, Dec 17, 2015 at 02:46:03PM +0200, Mika Kahola wrote:
> > > Disable DP fast link training if DP link configuration
> > > changes. If one of the DP link parameters i.e. link
> > > bandwidth, lane count, rate selection, port clock or bpp
> > 
> > This list of things we check should be updated to match the actual
> > patch.
> > 
> Yes, I need to update the commit message as the content of the patch
> changed a bit.
> 
> > > changes the link training does no longer apply the
> > > previously computed voltage swing and pre-emphasis values.
> > > Instead, the link training is started with zero values.
> > > 
> > > v4: Parameter and debug message naming improvements.
> > >     Fix for link parameter check (Ville)
> > > 
> > > v3: Remove cached old link parameters. Instead, disable
> > >     fast link training feature when link parameters are
> > >     set (Ville)
> > > 
> > > v2: Readout DPCD register to check if no aux handshaking is
> > >     required in link training (Ander)
> > > 
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c               |  8 +++++++-
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h              |  1 +
> > >  3 files changed, 25 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 31ba241..82504f0 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -1677,6 +1677,12 @@ found:
> > >  void intel_dp_set_link_params(struct intel_dp *intel_dp,
> > >  			      const struct intel_crtc_state *pipe_config)
> > >  {
> > > +	if (intel_dp->link_rate != pipe_config->port_clock ||
> > > +	    intel_dp->lane_count != pipe_config->lane_count) {
> > > +		intel_dp->train_set_valid = false;
> > > +		DRM_DEBUG_KMS("setting train set valid as false\n");
> > 
> > Maybe "Link parameters changed, resetting vswing and pre-emphasis" ?
> > 
> > Also maybe check for train_set_valid being true as well so that we
> > don't print the debug message unless we're really resetting the flag.
> > 
> Actually, I forgot to clean up this debug message. This was only
> intended for my personal use when I did the testing. I'll remove this
> debug message. Very good catch!
> 
> > > +	}
> > > +
> > >  	intel_dp->link_rate = pipe_config->port_clock;
> > >  	intel_dp->lane_count = pipe_config->lane_count;
> > >  }
> > > @@ -3849,7 +3855,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
> > >  	intel_dp->DP = DP;
> > >  }
> > >  
> > > -static bool
> > > +bool
> > >  intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >  {
> > >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > index 8888793..e46bad6 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > @@ -85,6 +85,23 @@ static bool
> > >  intel_dp_reset_link_train(struct intel_dp *intel_dp,
> > >  			uint8_t dp_train_pat)
> > >  {
> > > +	bool has_dpcd;
> > > +	bool no_aux_handshake = false;
> > > +
> > > +	has_dpcd = intel_dp_get_dpcd(intel_dp);
> > > +
> > > +	if (has_dpcd) {
> > > +		if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
> > > +			no_aux_handshake = (intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> > > +					    DP_NO_AUX_HANDSHAKE_LINK_TRAINING);
> > > +		}
> > > +	}
> > > +
> > > +	intel_dp->train_set_valid &= no_aux_handshake;
> > 
> > This should be separate patch. And I'm not convinced it's correct
> > anyway. At least it's not anything mandated by the spec AFAICS, so
> > there should be a comment why we're doing it. Also there's no
> > mention of  this change in the commit message either.
> > 
> I could split this patch in two parts and leave this part as a follow
> up.
> 
> The spec says
> 
> "When DP Source device is resuming the transmission (e.g., after waking
> from sleep), the Source device may skip the AUX transactions for the
> link training if the following conditions are met:
> 
>  - The Source device has determined that the HPD signal has remained
> asserted continuously (apart from IRQ_HPD notifications) since the link
> was last in full operation.
> - The Source device has read 1 in NO_AUX_TRANSACTION_LINK_TRAINING bit
> after the initial Sink device detection"
> 
> My interpretation of the spec is that we would need to check this bit
> just to ensure that we are capable of doing link training without AUX
> transactions. Without this check the we are probably too optimistic of
> training the link by reusing the known good drive current and
> pre-emphasis level. 

Starting a full training cycle with non-zero is mentioned as something
that is allowed in another part of the spec. To me it would make sense
to attempt it in any case. And then someone should perhaps implement
the real fast link training support for sinks that support it.

Though I still think we need
http://lists.freedesktop.org/archives/intel-gfx/2015-October/079195.html
since the current aux retry limits aren't high enough to make that
dongle happy otherwise.

> 
> BTW, should we change the naming of the define
> "DP_NO_AUX_HANDSHAKE_LINK_TRAINING" as the spec discusses of
> "DP_NO_AUX_TRANSACTION_LINK_TRAINING"?
> 
> > > +
> > > +	DRM_DEBUG_KMS("link training optimization: %s\n",
> > > +		      intel_dp->train_set_valid ? "true" : "false");
> > > +
> > >  	if (!intel_dp->train_set_valid)
> > >  		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> > >  	intel_dp_set_signal_levels(intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 798463e..94041fd 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1239,6 +1239,7 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc);
> > >  bool intel_dp_compute_config(struct intel_encoder *encoder,
> > >  			     struct intel_crtc_state *pipe_config);
> > >  bool intel_dp_is_edp(struct drm_device *dev, enum port port);
> > > +bool intel_dp_get_dpcd(struct intel_dp *intel_dp);
> > >  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port,
> > >  				  bool long_hpd);
> > >  void intel_edp_backlight_on(struct intel_dp *intel_dp);
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
>