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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 19 Apr 2018, vathsala nagaraju <vathsala.nagaraju@intel.com> 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.

This is an incredible mess.

> 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
>
> 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/intel_bios.c | 40 ++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_psr.c  | 26 ++++++++++++-------------
>  2 files changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 702d3fa..8913dc8 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -646,6 +646,15 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static bool
> +is_psr_options(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
> +{
> +	if (bdb->version >= 209 && IS_KABYLAKE(dev_priv))
> +		return true;
> +	else
> +		return false;
> +}
> +
>  static void
>  parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  {
> @@ -658,7 +667,6 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>  		DRM_DEBUG_KMS("No PSR BDB found.\n");
>  		return;
>  	}
> -
>  	psr_table = &psr->psr_table[panel_type];
>  
>  	dev_priv->vbt.psr.full_link = psr_table->full_link;
> @@ -687,8 +695,34 @@ 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    old decimal value interpretation
> +	 *  0 [500 us]         > 1 [500 us ]
> +	 *  1 [100 us]         > 0 [100 us ]
> +	 *  2 [2.5 ms]         > 5 [2.5 ms ]
> +	 *  3 [0   us]         = 0 [0   us ]

The old decimal value stuff was wake up time in multiples of 100 us.

> +	 */
> +	if (!is_psr_options(dev_priv, bdb)) {

You only use is_psr_options here once, please just open code the
condition. Also reverse order to not need !something in the condition.

> +		if (psr_table->tp1_wakeup_time > 5)
> +			dev_priv->vbt.psr.tp1_wakeup_time = 2;
> +		else if (psr_table->tp1_wakeup_time > 1)
> +			dev_priv->vbt.psr.tp1_wakeup_time = 0;
> +		else if (psr_table->tp1_wakeup_time > 0)
> +			dev_priv->vbt.psr.tp1_wakeup_time = 1;
> +		else
> +			dev_priv->vbt.psr.tp1_wakeup_time = 3;
> +
> +		if (psr_table->tp2_tp3_wakeup_time > 5)
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time = 2;
> +		else if (psr_table->tp2_tp3_wakeup_time > 1)
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time = 0;
> +		else if (psr_table->tp1_wakeup_time > 0)
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time = 1;
> +		else
> +			dev_priv->vbt.psr.tp2_tp3_wakeup_time = 3;
> +	} else {
> +		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;
> +	}
>  }

Please rename dev_priv->vbt.psr tp1_wakeup_time and tp2_tp3_wakeup_time
to have _us suffix, and actually assign the wakeup time in us
there. Hide all the hideous, hideous VBT stuff behind that, and doesn't
use magic numbers all over the place.

The old format becomes wakeup_time_us = vbt_value * 100. The code should
handle mismatches between the value and what the hardware can do (see
below).

The new format should just be a switch-case mapping values to us,
whining about values other than 0..3 and defaulting to max in that case.

>  
>  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 69a5b27..95658ad 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -353,21 +353,21 @@ 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)
> +	if (dev_priv->vbt.psr.tp1_wakeup_time == 0)
>  		val |= EDP_PSR_TP1_TIME_500us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time == 1)
>  		val |= EDP_PSR_TP1_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp1_wakeup_time == 2)
> +		val |= EDP_PSR_TP1_TIME_2500us;
>  	else
>  		val |= EDP_PSR_TP1_TIME_0us;
>  
> -	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 == 0)
> +		val |=  EDP_PSR_TP2_TP3_TIME_500us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 1)
>  		val |= EDP_PSR_TP2_TP3_TIME_100us;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 2)
> +		val |= EDP_PSR_TP2_TP3_TIME_2500us;
>  	else
>  		val |= EDP_PSR_TP2_TP3_TIME_0us;

Rewrite these to round up the longer wait:

        if (wakeup_time_us == 0)
		val |= EDP_PSR_TP2_TP3_TIME_0us;
	else if (wakeup_time_us <= 100)
		val |= EDP_PSR_TP2_TP3_TIME_100us;
	else if (wakeup_time_us <= 500)
		val |= EDP_PSR_TP2_TP3_TIME_500us;
	else
		val |= EDP_PSR_TP2_TP3_TIME_2500us;

>  
> @@ -406,12 +406,12 @@ 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)
> +	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 0)
>  		val |= EDP_PSR2_TP2_TIME_500;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 1)
>  		val |= EDP_PSR2_TP2_TIME_100;
> +	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 2)
> +		val |= EDP_PSR2_TP2_TIME_2500;
>  	else
>  		val |= EDP_PSR2_TP2_TIME_50;

Same here.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx