drm/i915/psr: vbt change for psr
diff mbox

Message ID 1526268734-8269-1-git-send-email-vathsala.nagaraju@intel.com
State New
Headers show

Commit Message

vathsala nagaraju May 14, 2018, 3:32 a.m. UTC
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 gen9_bc 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.
v5: Jani and Rodrigo
    Handle option 2 in default condition.
    Print oustide range value.
    For negetive values default to 2500us.
v6: Jani
    Handle default first and then fall through for case 2.
v7: Rodrigo
    Apply this change for IS_GEN9_BC and vbt version > 209

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 | 46 +++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++----------------
 4 files changed, 70 insertions(+), 27 deletions(-)

Comments

Puthikorn Voravootivat May 15, 2018, 10:55 p.m. UTC | #1
On Sun, May 13, 2018 at 8:32 PM 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 gen9_bc 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.
> v5: Jani and Rodrigo
>      Handle option 2 in default condition.
>      Print oustide range value.
>      For negetive values default to 2500us.
> v6: Jani
>      Handle default first and then fall through for case 2.
> v7: Rodrigo
>      Apply this change for IS_GEN9_BC and vbt version > 209

> 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 | 46
+++++++++++++++++++++++++++++++++++++--
>   drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++----------------
>   4 files changed, 70 insertions(+), 27 deletions(-)

> diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
> index 57fb3aa..268b059 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1078,8 +1078,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 f11bb21..6820658 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4088,10 +4088,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 54270bd..695ca73 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -688,8 +688,50 @@ 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.
> +        */
> +       if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
> +               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;
> +               default:
> +                       DRM_DEBUG_KMS("VBT tp1 wakeup time value %d is
outside range[0-3], defaulting to max value 2500us\n",
> +                                       psr_table->tp1_wakeup_time);
> +               case 2:
> +                       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;
> +               default:
> +                       DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value %d
is outside range[0-3], defaulting to max value 2500us\n",
> +                                       psr_table->tp2_tp3_wakeup_time);
> +               case 2:
> +                       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;
> +       }

Quick question:
Why did you write the same thing twice instead of extract to a new function
like this?

dev_priv->vbt.psr.tp1_wakeup_time_us =
vbt_psr_to_us(psr_table->tp1_wakeup_time)
dev_priv->vbt.psr.tp2_tp3_wakeup_time_us =
vbt_psr_to_us(psr_table->tp2_tp3_wakeup_time)

static int vbt_psr_to_us(int vbt_value) {
    if (bdb->version >= 209 ...) {
       return as {0:500, 1:100, 2:2500, 3:0}
    return vbt_value * 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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
> +           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
Dhinakaran Pandiyan May 15, 2018, 11:03 p.m. UTC | #2
On Mon, 2018-05-14 at 09:02 +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 gen9_bc 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.
> v5: Jani and Rodrigo
>     Handle option 2 in default condition.
>     Print oustide range value.
>     For negetive values default to 2500us.
> v6: Jani
>     Handle default first and then fall through for case 2.
> v7: Rodrigo
>     Apply this change for IS_GEN9_BC and vbt version > 209
> 
> 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 | 46
> +++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++----------
> ------
>  4 files changed, 70 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 57fb3aa..268b059 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1078,8 +1078,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 f11bb21..6820658 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4088,10 +4088,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 54270bd..695ca73 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -688,8 +688,50 @@ 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.
> +	 */
> +	if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {

Since this is the 'new' mapping, shouldn't this check be

if (version >= 209) {

}
i.e., what versions do BXT, GLK, CFL and CNL have?

Since gen-9 tables can have ambiguous interpretations, I think we can
do this.

if (version >= 209 || (IS_GEN9() && wakeup_time <=3)) {
	// Read this as {0:500, 1:100, 2:2500, 3:0}
} else {
	// Read this as wakeup_time * 100
}

This is assuming all versions => 209 use the new mapping consistently. 

2 and 3 are invalid values in the x*100 us scheme, so we can assume it
really means 2.5 ms and 0.
1 means the same.
0 is a problem, but we can check dpcd 0071h to confirm whether the sink
needs training or not. And it is safer to assume the sink needs
training at interpret is as 500 us.



> +		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;
> +		default:
> +			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d
> is outside range[0-3], defaulting to max value 2500us\n",
> +					psr_table->tp1_wakeup_time);
> +		case 2:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> +			break;
> +		}
> +

nit: That's an interesting order, it is neither sorted by the switch
variable nor by wake up time. It is easier to read if you chose one way
or the other IMO.

 
> +		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;
> +		default:
> +			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value
> %d is outside range[0-3], defaulting to max value 2500us\n",
> +					psr_table-
> >tp2_tp3_wakeup_time);
> +		case 2:
> +			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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
> +	    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);
>  }
vathsala nagaraju May 16, 2018, 3:44 a.m. UTC | #3
On Wednesday 16 May 2018 04:33 AM, Dhinakaran Pandiyan wrote:
> On Mon, 2018-05-14 at 09:02 +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 gen9_bc 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.
>> v5: Jani and Rodrigo
>>      Handle option 2 in default condition.
>>      Print oustide range value.
>>      For negetive values default to 2500us.
>> v6: Jani
>>      Handle default first and then fall through for case 2.
>> v7: Rodrigo
>>      Apply this change for IS_GEN9_BC and vbt version > 209
>>
>> 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 | 46
>> +++++++++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++----------
>> ------
>>   4 files changed, 70 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 57fb3aa..268b059 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1078,8 +1078,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 f11bb21..6820658 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4088,10 +4088,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 54270bd..695ca73 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -688,8 +688,50 @@ 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.
>> +	 */
>> +	if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
> Since this is the 'new' mapping, shouldn't this check be
>
> if (version >= 209) {
>
> }
check is for bdb version.
> i.e., what versions do BXT, GLK, CFL and CNL have?
waiting for GOP's team confirmation on above platforms.
We can add them later.
>
> Since gen-9 tables can have ambiguous interpretations, I think we can
> do this.
>
> if (version >= 209 || (IS_GEN9() && wakeup_time <=3)) {
> 	// Read this as {0:500, 1:100, 2:2500, 3:0}
With old bsf file , it's multiple of 100 ms.
if user inputs  2  , thinking that  it's 200 ms , with above change we 
are setting this to  2500 ms.
As per old spec, it should be set to 500 ms.   (>1 , set to 500)
Jani /Maulik, is it okay to make the above change?
> } else {
> 	// Read this as wakeup_time * 100
> }
>
> This is assuming all versions => 209 use the new mapping consistently.
>
> 2 and 3 are invalid values in the x*100 us scheme, so we can assume it
> really means 2.5 ms and 0.
> 1 means the same.
> 0 is a problem, but we can check dpcd 0071h to confirm whether the sink
> needs training or not. And it is safer to assume the sink needs
> training at interpret is as 500 us.

+		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;
+		default:
+			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d
is outside range[0-3], defaulting to max value 2500us\n",
+					psr_table->tp1_wakeup_time);
+		/*fall through*/
		case 2:
+			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
+			break;
+		}
+

> nit: That's an interesting order, it is neither sorted by the switch
> variable nor by wake up time. It is easier to read if you chose one way
> or the other IMO.
As per Jani's suggesttion , we need to set 2500ms for out of range[0-3]. 
It's a fall through.
should i add comment /*fall through*/  as above?
>   
>> +		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;
>> +		default:
>> +			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value
>> %d is outside range[0-3], defaulting to max value 2500us\n",
>> +					psr_table-
>>> tp2_tp3_wakeup_time);
>> +		case 2:
>> +			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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
>> +	    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);
>>   }
vathsala nagaraju May 16, 2018, 3:48 a.m. UTC | #4
On Wednesday 16 May 2018 04:25 AM, Puthikorn Voravootivat wrote:
> On Sun, May 13, 2018 at 8:32 PM 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 gen9_bc 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.
>> v5: Jani and Rodrigo
>>       Handle option 2 in default condition.
>>       Print oustide range value.
>>       For negetive values default to 2500us.
>> v6: Jani
>>       Handle default first and then fall through for case 2.
>> v7: Rodrigo
>>       Apply this change for IS_GEN9_BC and vbt version > 209
>> 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 | 46
> +++++++++++++++++++++++++++++++++++++--
>>    drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++----------------
>>    4 files changed, 70 insertions(+), 27 deletions(-)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
>> index 57fb3aa..268b059 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1078,8 +1078,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 f11bb21..6820658 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -4088,10 +4088,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 54270bd..695ca73 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -688,8 +688,50 @@ 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.
>> +        */
>> +       if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
>> +               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;
>> +               default:
>> +                       DRM_DEBUG_KMS("VBT tp1 wakeup time value %d is
> outside range[0-3], defaulting to max value 2500us\n",
>> +                                       psr_table->tp1_wakeup_time);
>> +               case 2:
>> +                       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;
>> +               default:
>> +                       DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value %d
> is outside range[0-3], defaulting to max value 2500us\n",
>> +                                       psr_table->tp2_tp3_wakeup_time);
>> +               case 2:
>> +                       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;
>> +       }
> Quick question:
> Why did you write the same thing twice instead of extract to a new function
> like this?
>
> dev_priv->vbt.psr.tp1_wakeup_time_us =
> vbt_psr_to_us(psr_table->tp1_wakeup_time)
> dev_priv->vbt.psr.tp2_tp3_wakeup_time_us =
> vbt_psr_to_us(psr_table->tp2_tp3_wakeup_time)
>
> static int vbt_psr_to_us(int vbt_value) {
>      if (bdb->version >= 209 ...) {
>         return as {0:500, 1:100, 2:2500, 3:0}
>      return vbt_value * 100;
> }
Will make this change  in next version.
>>    }
>>    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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
>> +           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
Jani Nikula May 16, 2018, 8:08 a.m. UTC | #5
On Wed, 16 May 2018, vathsala nagaraju <vathsala.nagaraju@intel.com> wrote:
> On Wednesday 16 May 2018 04:33 AM, Dhinakaran Pandiyan wrote:
>> On Mon, 2018-05-14 at 09:02 +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 gen9_bc 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.
>>> v5: Jani and Rodrigo
>>>      Handle option 2 in default condition.
>>>      Print oustide range value.
>>>      For negetive values default to 2500us.
>>> v6: Jani
>>>      Handle default first and then fall through for case 2.
>>> v7: Rodrigo
>>>      Apply this change for IS_GEN9_BC and vbt version > 209
>>>
>>> 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 | 46
>>> +++++++++++++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++----------
>>> ------
>>>   4 files changed, 70 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 57fb3aa..268b059 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1078,8 +1078,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 f11bb21..6820658 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -4088,10 +4088,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 54270bd..695ca73 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -688,8 +688,50 @@ 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.
>>> +	 */
>>> +	if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
>> Since this is the 'new' mapping, shouldn't this check be
>>
>> if (version >= 209) {
>>
>> }
> check is for bdb version.
>> i.e., what versions do BXT, GLK, CFL and CNL have?
> waiting for GOP's team confirmation on above platforms.
> We can add them later.
>>
>> Since gen-9 tables can have ambiguous interpretations, I think we can
>> do this.
>>
>> if (version >= 209 || (IS_GEN9() && wakeup_time <=3)) {
>> 	// Read this as {0:500, 1:100, 2:2500, 3:0}
> With old bsf file , it's multiple of 100 ms.
> if user inputs  2  , thinking that  it's 200 ms , with above change we 
> are setting this to  2500 ms.
> As per old spec, it should be set to 500 ms.   (>1 , set to 500)
> Jani /Maulik, is it okay to make the above change?

I think the patch is now the way it should be. We should not change our
interpretation based on the value.

BR,
Jani.



>> } else {
>> 	// Read this as wakeup_time * 100
>> }
>>
>> This is assuming all versions => 209 use the new mapping consistently.
>>
>> 2 and 3 are invalid values in the x*100 us scheme, so we can assume it
>> really means 2.5 ms and 0.
>> 1 means the same.
>> 0 is a problem, but we can check dpcd 0071h to confirm whether the sink
>> needs training or not. And it is safer to assume the sink needs
>> training at interpret is as 500 us.
>
> +		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;
> +		default:
> +			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d
> is outside range[0-3], defaulting to max value 2500us\n",
> +					psr_table->tp1_wakeup_time);
> +		/*fall through*/
> 		case 2:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> +			break;
> +		}
> +
>
>> nit: That's an interesting order, it is neither sorted by the switch
>> variable nor by wake up time. It is easier to read if you chose one way
>> or the other IMO.
> As per Jani's suggesttion , we need to set 2500ms for out of range[0-3]. 
> It's a fall through.
> should i add comment /*fall through*/  as above?
>>   
>>> +		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;
>>> +		default:
>>> +			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value
>>> %d is outside range[0-3], defaulting to max value 2500us\n",
>>> +					psr_table-
>>>> tp2_tp3_wakeup_time);
>>> +		case 2:
>>> +			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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
>>> +	    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);
>>>   }
>
Jani Nikula May 16, 2018, 8:13 a.m. UTC | #6
On Wed, 16 May 2018, vathsala nagaraju <vathsala.nagaraju@intel.com> wrote:
> On Wednesday 16 May 2018 04:33 AM, Dhinakaran Pandiyan wrote:
>> On Mon, 2018-05-14 at 09:02 +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 gen9_bc 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.
>>> v5: Jani and Rodrigo
>>>      Handle option 2 in default condition.
>>>      Print oustide range value.
>>>      For negetive values default to 2500us.
>>> v6: Jani
>>>      Handle default first and then fall through for case 2.
>>> v7: Rodrigo
>>>      Apply this change for IS_GEN9_BC and vbt version > 209
>>>
>>> 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 | 46
>>> +++++++++++++++++++++++++++++++++++++--
>>>   drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++----------
>>> ------
>>>   4 files changed, 70 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 57fb3aa..268b059 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1078,8 +1078,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 f11bb21..6820658 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -4088,10 +4088,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 54270bd..695ca73 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -688,8 +688,50 @@ 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.
>>> +	 */
>>> +	if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
>> Since this is the 'new' mapping, shouldn't this check be
>>
>> if (version >= 209) {
>>
>> }
> check is for bdb version.

The reality is that this was screwed up royally. We have to include the
platform checks. I hate it as much as you do.

Besides, IMO there was *never* *any* need to change the encoding to
begin with. The driver has to check the values against what the hardware
is capable of, so the right course of action would have been to keep it
as a multiple of 100 us. It's the only sensible thing, and forward
compatible for all platforms to come. Now we map the VBT crap to us, and
then back to what the hardware can do. Ugh.


BR,
Jani.

>> i.e., what versions do BXT, GLK, CFL and CNL have?
> waiting for GOP's team confirmation on above platforms.
> We can add them later.
>>
>> Since gen-9 tables can have ambiguous interpretations, I think we can
>> do this.
>>
>> if (version >= 209 || (IS_GEN9() && wakeup_time <=3)) {
>> 	// Read this as {0:500, 1:100, 2:2500, 3:0}
> With old bsf file , it's multiple of 100 ms.
> if user inputs  2  , thinking that  it's 200 ms , with above change we 
> are setting this to  2500 ms.
> As per old spec, it should be set to 500 ms.   (>1 , set to 500)
> Jani /Maulik, is it okay to make the above change?
>> } else {
>> 	// Read this as wakeup_time * 100
>> }
>>
>> This is assuming all versions => 209 use the new mapping consistently.
>>
>> 2 and 3 are invalid values in the x*100 us scheme, so we can assume it
>> really means 2.5 ms and 0.
>> 1 means the same.
>> 0 is a problem, but we can check dpcd 0071h to confirm whether the sink
>> needs training or not. And it is safer to assume the sink needs
>> training at interpret is as 500 us.
>
> +		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;
> +		default:
> +			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d
> is outside range[0-3], defaulting to max value 2500us\n",
> +					psr_table->tp1_wakeup_time);
> +		/*fall through*/
> 		case 2:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> +			break;
> +		}
> +
>
>> nit: That's an interesting order, it is neither sorted by the switch
>> variable nor by wake up time. It is easier to read if you chose one way
>> or the other IMO.
> As per Jani's suggesttion , we need to set 2500ms for out of range[0-3]. 
> It's a fall through.
> should i add comment /*fall through*/  as above?
>>   
>>> +		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;
>>> +		default:
>>> +			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value
>>> %d is outside range[0-3], defaulting to max value 2500us\n",
>>> +					psr_table-
>>>> tp2_tp3_wakeup_time);
>>> +		case 2:
>>> +			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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
>>> +	    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);
>>>   }
>
Dhinakaran Pandiyan May 16, 2018, 5:44 p.m. UTC | #7
On Wed, 2018-05-16 at 11:08 +0300, Jani Nikula wrote:
> On Wed, 16 May 2018, vathsala nagaraju <vathsala.nagaraju@intel.com>
> wrote:
> > 
> > On Wednesday 16 May 2018 04:33 AM, Dhinakaran Pandiyan wrote:
> > > 
> > > On Mon, 2018-05-14 at 09:02 +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 gen9_bc 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.
> > > > v5: Jani and Rodrigo
> > > >      Handle option 2 in default condition.
> > > >      Print oustide range value.
> > > >      For negetive values default to 2500us.
> > > > v6: Jani
> > > >      Handle default first and then fall through for case 2.
> > > > v7: Rodrigo
> > > >      Apply this change for IS_GEN9_BC and vbt version > 209
> > > > 
> > > > 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 | 46
> > > > +++++++++++++++++++++++++++++++++++++--
> > > >   drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++-----
> > > > -----
> > > > ------
> > > >   4 files changed, 70 insertions(+), 27 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 57fb3aa..268b059 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1078,8 +1078,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 f11bb21..6820658 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -4088,10 +4088,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 54270bd..695ca73 100644
> > > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > > @@ -688,8 +688,50 @@ 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.
> > > > +	 */
> > > > +	if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
> > > Since this is the 'new' mapping, shouldn't this check be
> > > 
> > > if (version >= 209) {
> > > 
> > > }
> > check is for bdb version.
> > > 
> > > i.e., what versions do BXT, GLK, CFL and CNL have?
> > waiting for GOP's team confirmation on above platforms.
> > We can add them later.
> > > 
> > > 
> > > Since gen-9 tables can have ambiguous interpretations, I think we
> > > can
> > > do this.
> > > 
> > > if (version >= 209 || (IS_GEN9() && wakeup_time <=3)) {
> > > 	// Read this as {0:500, 1:100, 2:2500, 3:0}
> > With old bsf file , it's multiple of 100 ms.
> > if user inputs  2  , thinking that  it's 200 ms , with above change
> > we 
> > are setting this to  2500 ms.
> > As per old spec, it should be set to 500 ms.   (>1 , set to 500)
> > Jani /Maulik, is it okay to make the above change?
> I think the patch is now the way it should be. We should not change
> our
> interpretation based on the value.

Is it correct to infer, from your response, that VBT values are not
always set based on hardware capability as documented in bspec?



> 
> BR,
> Jani.
> 
> 
> 
> > 
> > > 
> > > } else {
> > > 	// Read this as wakeup_time * 100
> > > }
> > > 
> > > This is assuming all versions => 209 use the new mapping
> > > consistently.
> > > 
> > > 2 and 3 are invalid values in the x*100 us scheme, so we can
> > > assume it
> > > really means 2.5 ms and 0.
> > > 1 means the same.
> > > 0 is a problem, but we can check dpcd 0071h to confirm whether
> > > the sink
> > > needs training or not. And it is safer to assume the sink needs
> > > training at interpret is as 500 us.
> > +		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;
> > +		default:
> > +			DRM_DEBUG_KMS("VBT tp1 wakeup time value
> > %d
> > is outside range[0-3], defaulting to max value 2500us\n",
> > +					psr_table-
> > >tp1_wakeup_time);
> > +		/*fall through*/
> > 		case 2:
> > +			dev_priv->vbt.psr.tp1_wakeup_time_us =
> > 2500;
> > +			break;
> > +		}
> > +
> > 
> > > 
> > > nit: That's an interesting order, it is neither sorted by the
> > > switch
> > > variable nor by wake up time. It is easier to read if you chose
> > > one way
> > > or the other IMO.
> > As per Jani's suggesttion , we need to set 2500ms for out of
> > range[0-3]. 
> > It's a fall through.
> > should i add comment /*fall through*/  as above?
> > > 
> > >   
> > > > 
> > > > +		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;
> > > > +		default:
> > > > +			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time
> > > > value
> > > > %d is outside range[0-3], defaulting to max value 2500us\n",
> > > > +					psr_table-
> > > > > 
> > > > > tp2_tp3_wakeup_time);
> > > > +		case 2:
> > > > +			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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
> > > > +	    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);
> > > >   }
Dhinakaran Pandiyan May 16, 2018, 10:04 p.m. UTC | #8
On Wed, 2018-05-16 at 09:14 +0530, vathsala nagaraju wrote:
> On Wednesday 16 May 2018 04:33 AM, Dhinakaran Pandiyan wrote:
> > 
> > On Mon, 2018-05-14 at 09:02 +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 gen9_bc 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.
> > > v5: Jani and Rodrigo
> > >      Handle option 2 in default condition.
> > >      Print oustide range value.
> > >      For negetive values default to 2500us.
> > > v6: Jani
> > >      Handle default first and then fall through for case 2.
> > > v7: Rodrigo
> > >      Apply this change for IS_GEN9_BC and vbt version > 209
> > > 
> > > 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 | 46
> > > +++++++++++++++++++++++++++++++++++++--
> > >   drivers/gpu/drm/i915/intel_psr.c  | 39 +++++++++++++++++-------
> > > ---
> > > ------
> > >   4 files changed, 70 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 57fb3aa..268b059 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1078,8 +1078,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 f11bb21..6820658 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4088,10 +4088,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 54270bd..695ca73 100644
> > > --- a/drivers/gpu/drm/i915/intel_bios.c
> > > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > > @@ -688,8 +688,50 @@ 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.
> > > +	 */
> > > +	if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
> > Since this is the 'new' mapping, shouldn't this check be
> > 
> > if (version >= 209) {
> > 
> > }
> check is for bdb version.
> > 
> > i.e., what versions do BXT, GLK, CFL and CNL have?
> waiting for GOP's team confirmation on above platforms.
> We can add them later.
> > 
> > 
> > Since gen-9 tables can have ambiguous interpretations, I think we
> > can
> > do this.
> > 
> > if (version >= 209 || (IS_GEN9() && wakeup_time <=3)) {
> > 	// Read this as {0:500, 1:100, 2:2500, 3:0}
> With old bsf file , it's multiple of 100 ms.
> if user inputs  2  , thinking that  it's 200 ms , with above change
> we 
> are setting this to  2500 ms.
> As per old spec, it should be set to 500 ms.   (>1 , set to 500)
> Jani /Maulik, is it okay to make the above change?
> > 
> > } else {
> > 	// Read this as wakeup_time * 100
> > }
> > 
> > This is assuming all versions => 209 use the new mapping
> > consistently.
> > 
> > 2 and 3 are invalid values in the x*100 us scheme, so we can assume
> > it
> > really means 2.5 ms and 0.
> > 1 means the same.
> > 0 is a problem, but we can check dpcd 0071h to confirm whether the
> > sink
> > needs training or not. And it is safer to assume the sink needs
> > training at interpret is as 500 us.
> +		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;
> +		default:
> +			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d
> is outside range[0-3], defaulting to max value 2500us\n",
> +					psr_table->tp1_wakeup_time);
> +		/*fall through*/
> 		case 2:
> +			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> +			break;
> +		}
> +
> 
> > 
> > nit: That's an interesting order, it is neither sorted by the
> > switch
> > variable nor by wake up time. It is easier to read if you chose one
> > way
> > or the other IMO.
> As per Jani's suggesttion , we need to set 2500ms for out of range[0-
> 3]. 
This is what I meant -
               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;
                default:
                        DRM_DEBUG_KMS("VBT tp1 wakeup time value %d is
outside range[0-3], defaulting to max value 2500us\n",
                                        psr_table->tp1_wakeup_time);
                case 2:
                        dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
                        break;

                case 3:
                        dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
                        break;
                }



> It's a fall through.
> should i add comment /*fall through*/  as above?

Yeah, no harm in adding that and iirc you get a gcc warning if the fall
through isn't marked..

> > 
> >   
> > > 
> > > +		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;
> > > +		default:
> > > +			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time
> > > value
> > > %d is outside range[0-3], defaulting to max value 2500us\n",
> > > +					psr_table-
> > > > 
> > > > tp2_tp3_wakeup_time);
> > > +		case 2:
> > > +			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 db27f2f..d64f039 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,15 @@ 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 >= 0 &&
> > > +	    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);
> > >   }
Jani Nikula May 17, 2018, 8:02 a.m. UTC | #9
On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> On Wed, 2018-05-16 at 11:08 +0300, Jani Nikula wrote:
>> I think the patch is now the way it should be. We should not change
>> our interpretation based on the value.
>
> Is it correct to infer, from your response, that VBT values are not
> always set based on hardware capability as documented in bspec?

Correct. I think it was the good intention of the VBT change to only
allow values that map to valid hardware values, but I think it has
caused much more trouble than it has helped.

First, the change was not universally tied to a VBT version, and I've
seen VBTs in the wild with version >= 209 that still use multiples of
100 us. (And the spec is still in contradiction with itself.)

Second, regardless of mapping or multiples of 100 us we need to verify
our input. Because I've seen VBTs in the wild that are bogus regardless
of how they're supposed to be interpreted.

Third, we already had the code in place to map multiples of 100 us to
hardware. We'll need to keep that practically forever. And now we need
to have *another* mapping.

Fourth, the multiples of 100 us does not require *any* spec change when
hardware changes to support different values. The new mapping requires
changes throughout the stack that looks at the values. (Basically I
object to any VBT specification that says anything platform
dependent. It should be generic.)

Now, the patch at hand uses the best guesses we can make to translate
whatever the VBT has to microseconds, in intel_bios.c. That part does
not care about hardware capability. For validity, it only looks at what
we think is according to VBT spec. It should be hardware agnostic,
except for the IS_GEN9_BC() thing, which should probably include gen10+
too.

The code in intel_psr.c gets the microseconds as input, and maps that to
hardware capability as best we can, erring towards longer delays.

Two different abstractions for two different things. One to abstract
VBT, another to abstract hardware. This is in line with the direction
I've tried to take intel_bios.c and VBT parsing and the VBT spec (the
little I've had influence for that) for the longest time. We must not
let the VBT abstractions to leak into the driver code.

BR,
Jani.
Dhinakaran Pandiyan May 17, 2018, 8:12 p.m. UTC | #10
On Thu, 2018-05-17 at 11:02 +0300, Jani Nikula wrote:
> On Wed, 16 May 2018, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.c
> om> wrote:
> > 
> > On Wed, 2018-05-16 at 11:08 +0300, Jani Nikula wrote:
> > > 
> > > I think the patch is now the way it should be. We should not
> > > change
> > > our interpretation based on the value.
> > Is it correct to infer, from your response, that VBT values are not
> > always set based on hardware capability as documented in bspec?
> Correct. I think it was the good intention of the VBT change to only
> allow values that map to valid hardware values, but I think it has
> caused much more trouble than it has helped.
> 
> First, the change was not universally tied to a VBT version, and I've
> seen VBTs in the wild with version >= 209 that still use multiples of
> 100 us. (And the spec is still in contradiction with itself.)
> 
> Second, regardless of mapping or multiples of 100 us we need to
> verify
> our input. Because I've seen VBTs in the wild that are bogus
> regardless
> of how they're supposed to be interpreted.
> 
> Third, we already had the code in place to map multiples of 100 us to
> hardware. We'll need to keep that practically forever. And now we
> need
> to have *another* mapping.
> 
> Fourth, the multiples of 100 us does not require *any* spec change
> when
> hardware changes to support different values. The new mapping
> requires
> changes throughout the stack that looks at the values. (Basically I
> object to any VBT specification that says anything platform
> dependent. It should be generic.)
> 
> Now, the patch at hand uses the best guesses we can make to translate
> whatever the VBT has to microseconds, in intel_bios.c. That part does
> not care about hardware capability. For validity, it only looks at
> what
> we think is according to VBT spec. It should be hardware agnostic,
> except for the IS_GEN9_BC() thing, which should probably include
> gen10+
> too.
> 
> The code in intel_psr.c gets the microseconds as input, and maps that
> to
> hardware capability as best we can, erring towards longer delays.
> 
> Two different abstractions for two different things. One to abstract
> VBT, another to abstract hardware. This is in line with the direction
> I've tried to take intel_bios.c and VBT parsing and the VBT spec (the
> little I've had influence for that) for the longest time. We must not
> let the VBT abstractions to leak into the driver code.

Thanks for the detailed explanation of the problem and the idea behind
the design.

-DK


> 
> BR,
> Jani.
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 57fb3aa..268b059 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1078,8 +1078,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 f11bb21..6820658 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4088,10 +4088,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 54270bd..695ca73 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -688,8 +688,50 @@  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.
+	 */
+	if (bdb->version >= 209 && IS_GEN9_BC(dev_priv)) {
+		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;
+		default:
+			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
+					psr_table->tp1_wakeup_time);
+		case 2:
+			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;
+		default:
+			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
+					psr_table->tp2_tp3_wakeup_time);
+		case 2:
+			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 db27f2f..d64f039 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,15 @@  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 >= 0 &&
+	    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);
 }