[Intel-gfx] [PATCH 4/4] drm/i915/psr: Add HBR3 support

[Dhinakaran Pandiyan]

On Wed, 2019-01-16 at 15:43 -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.
> 
Sounds like TP3 and TP4 are used only with PSR1, please document that
in the commit message. 
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
> 
> Still trying to understand how PSR1 was working on ICL while sending
> TP4 to a panel that only supports HBR2.

That's a good point, along with that please find out what Bit 11: "TPS4
Control" does. I'd like us get these questions answered, if possible,
before merging this series.

> 
>  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;
>  
>  	/* 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) &&
Now that you are removing a caller, make this function static and move
to intel_dp_link_tranining.c instead ? Or just inline the code there.

> -	    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
> +		 */
IMHO I think we should skip this comment, we'd have to write
documentation for every other register if we are going to do this :)

> +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP4_TIME_SHIFT;

So, the EDP_PSR_TP1_TP3_SEL bit has no effect when the TP4 duration is
non-zero, seems like an indirect way of switching to TP4 compared to
TP2 and TP3.

> +	} 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;