Re: [PATCH] drm/i915/psr: vbt change for psr

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, May 02, 2018 at 02:43:29PM +0530, vathsala nagaraju wrote:
> From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> 
> For psr block #9, the vbt description has moved to options [0-3] for
> TP1,TP2,TP3 Wakeup time from decimal value without any change to vbt
> structure. Since spec does not  mention from which VBT version this
> change was added to vbt.bsf file, we cannot depend on bdb->version check
> to change for all the platforms.
> 
> There is RCR inplace for GOP team to  provide the version number
> to make generic change. Since Kabylake with bdb version 209 is having this
> change, limiting this change to kbl and version 209+ to unblock google.
> 
> Tested on skl(bdb version 203,without options) and
> kabylake(bdb version 209,212) having new options.
> 
> bspec 20131
> 
> v2: (Jani and Rodrigo)
>     move the 165 version check to intel_bios.c
> v3: Jani
>     Move the abstraction to intel_bios.
> v4: Jani
>     Rename tp*_wakeup_time to have "us" suffix.
>     For values outside range[0-3],default to max 2500us.
>     Old decimal value was wake up time in multiples of 100us.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> CC: Puthikorn Voravootivat <puthik@chromium.org>
> 
> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  4 ++--
>  drivers/gpu/drm/i915/i915_reg.h   |  8 +++----
>  drivers/gpu/drm/i915/intel_bios.c | 45 +++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_psr.c  | 38 ++++++++++++++++-----------------
>  4 files changed, 68 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6268a51..a189382 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1077,8 +1077,8 @@ struct intel_vbt_data {
>  		bool require_aux_wakeup;
>  		int idle_frames;
>  		enum psr_lines_to_wait lines_to_wait;
> -		int tp1_wakeup_time;
> -		int tp2_tp3_wakeup_time;
> +		int tp1_wakeup_time_us;
> +		int tp2_tp3_wakeup_time_us;
>  	} psr;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 197c966..6bbd0b4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4084,10 +4084,10 @@ enum {
>  #define   EDP_Y_COORDINATE_ENABLE	(1<<25) /* GLK and CNL+ */
>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t)<<20)
>  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f<<20)
> -#define   EDP_PSR2_TP2_TIME_500		(0<<8)
> -#define   EDP_PSR2_TP2_TIME_100		(1<<8)
> -#define   EDP_PSR2_TP2_TIME_2500	(2<<8)
> -#define   EDP_PSR2_TP2_TIME_50		(3<<8)
> +#define   EDP_PSR2_TP2_TIME_500us	(0<<8)
> +#define   EDP_PSR2_TP2_TIME_100us	(1<<8)
> +#define   EDP_PSR2_TP2_TIME_2500us	(2<<8)
> +#define   EDP_PSR2_TP2_TIME_50us	(3<<8)
>  #define   EDP_PSR2_TP2_TIME_MASK	(3<<8)
>  #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 702d3fa..a246b6b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -687,8 +687,49 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>  		break;
>  	}
>  
> -	dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
> -	dev_priv->vbt.psr.tp2_tp3_wakeup_time = psr_table->tp2_tp3_wakeup_time;
> +	/* new psr options 0=500us, 1=100us, 2=2500us, 3=0us
> +	 * Old decimal value is wake up time in multiples of 100 us.
> +	 */

Please follow this style with first empty line:

/*
 * Text here
 */

Start with capital N on New.

> +	if (bdb->version >= 209 && IS_KABYLAKE(dev_priv)) {

Why are we filtering per platform?
What about Coffelake? And Geminilake? And Cannonlake? and on?

Btw, where is this spec? I just checked predator and it still
only contains the old multiple of 100 one... :(

> +		switch (psr_table->tp1_wakeup_time) {
> +		case 0:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
> +			break;
> +		case 1:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
> +			break;
> +		case 3:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
> +			break;
> +		case 2:

this out of order case is not good imho...
confusing... but well... 0 -> 2500 -> 100 -> 500 is already confusing by itself
honestly...

> +		default:

why default is 2? higher?
I always thought this awkward order was to make the 0 the default with 500
or at least I was trying to make some sense on that order :/

> +			if (psr_table->tp1_wakeup_time != 2)
> +				DRM_DEBUG_KMS("VBT tp1 wakeup time outside range, defaulting to max value 2500us\n");
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> +			break;
> +		}
> +
> +		switch (psr_table->tp2_tp3_wakeup_time) {
> +		case 0:
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
> +			break;
> +		case 1:
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
> +			break;
> +		case 3:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
> +			break;
> +		case 2:
> +		default:
> +			if (psr_table->tp2_tp3_wakeup_time != 2)
> +				DRM_DEBUG_KMS("VBT tp2_tp3  wakeup time outside range, defaulting to max value 2500us\n");
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
> +		break;
> +		}
> +	} else {
> +		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
> +		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
> +	}
>  }
>  
>  static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 6233a32..746c5ac 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -461,23 +461,23 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	if (dev_priv->psr.link_standby)
>  		val |= EDP_PSR_LINK_STANDBY;
>  
> -	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
> -		val |= EDP_PSR_TP1_TIME_2500us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
> -		val |= EDP_PSR_TP1_TIME_500us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
> +	if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 0)
> +		val |=  EDP_PSR_TP1_TIME_0us;
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
>  		val |= EDP_PSR_TP1_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
> +		val |= EDP_PSR_TP1_TIME_500us;
>  	else
> -		val |= EDP_PSR_TP1_TIME_0us;
> +		val |= EDP_PSR_TP1_TIME_2500us;
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> -		val |= EDP_PSR_TP2_TP3_TIME_2500us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> -		val |= EDP_PSR_TP2_TP3_TIME_500us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 0)
> +		val |=  EDP_PSR_TP2_TP3_TIME_0us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
>  		val |= EDP_PSR_TP2_TP3_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +		val |= EDP_PSR_TP2_TP3_TIME_500us;
>  	else
> -		val |= EDP_PSR_TP2_TP3_TIME_0us;
> +		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>  
>  	if (intel_dp_source_supports_hbr2(intel_dp) &&
>  	    drm_dp_tps3_supported(intel_dp->dpcd))
> @@ -513,14 +513,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  
>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> -		val |= EDP_PSR2_TP2_TIME_2500;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> -		val |= EDP_PSR2_TP2_TIME_500;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
> -		val |= EDP_PSR2_TP2_TIME_100;
> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> +		val |= EDP_PSR2_TP2_TIME_50us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
> +		val |= EDP_PSR2_TP2_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +		val |= EDP_PSR2_TP2_TIME_500us;
>  	else
> -		val |= EDP_PSR2_TP2_TIME_50;
> +		val |= EDP_PSR2_TP2_TIME_2500us;
>  
>  	I915_WRITE(EDP_PSR2_CTL, val);
>  }
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx