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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx