Re: [PATCH 4/4] drm/i915/psr: Add HBR3 support

[Manasi Navare <manasi.d.navare@intel.com>]

On Wed, Jan 16, 2019 at 03:43:20PM -0800, José Roberto de Souza wrote:
> If the sink and source supports HBR3, TP4 should be used as link
> training pattern.
> For PSR2 there is no register to set and enable TP4 but according to
> eDP spec TP3 is still a training pattern acceptable for HBR3 panels.
>

The spec refers to training pattern 4 as TPS4, 3 as TPS3 etc so its better
to stick with the same abbreviations in this patch commit message and comments
as well since the spec uses TP2/TP3 etc for test point 2/3.
 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> Still trying to understand how PSR1 was working on ICL while sending
> TP4 to a panel that only supports HBR2.
> 
>  drivers/gpu/drm/i915/i915_reg.h               |  1 +
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h              |  1 +
>  drivers/gpu/drm/i915/intel_psr.c              | 24 ++++++++++++++-----
>  4 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5faca634ee70..1e792309a79e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4162,6 +4162,7 @@ enum {
>  #define   EDP_PSR_TP1_TP3_SEL			(1 << 11)
>  #define   EDP_PSR_CRC_ENABLE			(1 << 10) /* BDW+ */
>  #define   EDP_PSR_TP2_TP3_TIME_SHIFT		(8)
> +#define   EDP_PSR_TP4_TIME_SHIFT		(6) /* ICL+ */
>  #define   EDP_PSR_TP1_TIME_SHIFT		(4)
>  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 30be0e39bd5f..3e9798a5498c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   * or for 1.4 devices that support it, training Pattern 3 for HBR2
>   * or 1.2 devices that support it, Training Pattern 2 otherwise.
>   */
> -static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
>  {
>  	bool source_tps3, sink_tps3, source_tps4, sink_tps4;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e5a436c33307..fc3e6ae92276 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1807,6 +1807,7 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>  					    int link_rate, uint8_t lane_count);
>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
>  int intel_dp_retrain_link(struct intel_encoder *encoder,
>  			  struct drm_modeset_acquire_ctx *ctx);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2fc537fb6e78..b0525940e5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  	u32 max_sleep_time = 0x1f;
>  	u32 val = EDP_PSR_ENABLE;
> +	u32 tp;

I would prefer calling this tps like the intel_dp_training_pattern function
calls it source_tps sink_tps.

Apart from that looks good. So with these changes

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

Manasi
 
>  
>  	/* Let's use 6 as the minimum to cover all known cases including the
>  	 * off-by-one issue that HW has in some cases.
> @@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  		val |= EDP_PSR_LINK_STANDBY;
>  
>  	val |= dev_priv->vbt.psr.tp1_wakeup_time << EDP_PSR_TP1_TIME_SHIFT;
> -	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
>  
> -	if (intel_dp_source_supports_hbr2(intel_dp) &&
> -	    drm_dp_tps3_supported(intel_dp->dpcd))
> -		val |= EDP_PSR_TP1_TP3_SEL;
> -	else
> -		val |= EDP_PSR_TP1_TP2_SEL;
> +	tp = intel_dp_training_pattern(intel_dp);
> +	if (tp == DP_TRAINING_PATTERN_4) {
> +		/*
> +		 * TP4 is selected by setting EDP_PSR_TP4_TIME with other value
> +		 * than PSR_TP_WAKEUP_TIME_NONE
> +		 */
> +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP4_TIME_SHIFT;
> +	} else {
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			val |= PSR_TP_WAKEUP_TIME_NONE << EDP_PSR_TP4_TIME_SHIFT;
> +
> +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
> +		if (tp == DP_TRAINING_PATTERN_3)
> +			val |= EDP_PSR_TP1_TP3_SEL;
> +		else
> +			val |= EDP_PSR_TP1_TP2_SEL;
> +	}
>  
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		val |= EDP_PSR_CRC_ENABLE;
> -- 
> 2.20.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx