On Fri, 18 May 2018, vathsala nagaraju <vathsala.nagar...@intel.com> wrote:
> From: Vathsala Nagaraju <vathsala.nagar...@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.
> v8: Puthik
> add new function vbt_psr_to_us.
>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Cc: Puthikorn Voravootivat <put...@chromium.org>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> Cc: José Roberto de Souza <jose.so...@intel.com>
> Cc: Jani Nikula <jani.nik...@intel.com>
>
> Signed-off-by: Maulik V Vaghela <maulik.v.vagh...@intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagar...@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 | 38 ++++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_psr.c | 39
> ++++++++++++++++++++-------------------
> 4 files changed, 62 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e33c380..dcfa791 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 196a0eb..513b4a4 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..5d8c29f 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -647,12 +647,26 @@ static int intel_bios_ssc_frequency(struct
> drm_i915_private *dev_priv,
> }
> }
>
> +static int vbt_psr_to_us(bool is_waketime_options, int vbt_value)
> +{
> + if (is_waketime_options) {
> + int waketime_map[] = {500, 100, 2500, 0};
> + /* Reset to value 2 = 2500us for outside range [0-3] */
> + if (vbt_value < 0 || vbt_value > 3)
> + vbt_value = 2;
> + return waketime_map[vbt_value];
> + } else {
> + return vbt_value * 100;
> + }
> +}
> +
> static void
> parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
> {
> const struct bdb_psr *psr;
> const struct psr_table *psr_table;
> int panel_type = dev_priv->vbt.panel_type;
> + bool is_waketime_options = false;
>
> psr = find_section(bdb, BDB_PSR);
> if (!psr) {
> @@ -688,8 +702,28 @@ 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 wake options 0=500us, 1=100us, 2=2500us, 3=0us
> + * Old decimal value is wake up time in multiples of 100 us.
> + * TODO: add other platforms having new psr options.
> + */
> + is_waketime_options = ((bdb->version >= 209) && IS_GEN9_BC(dev_priv));
> + if (is_waketime_options) {
> + /* Reset to value 2 = 2500us for outside range [0-3] */
> + if (psr_table->tp1_wakeup_time < 0 ||
> + psr_table->tp1_wakeup_time > 3) {
> + 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);
> + }
> + if (psr_table->tp2_tp3_wakeup_time < 0 ||
> + psr_table->tp2_tp3_wakeup_time > 3) {
> + 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);
> + }
> + }
Why did you take something that was obvious at a glance and turn it into
this complicated mess?
I'd rather take the previous version, even with the duplication.
BR,
Jani.
> +
> + dev_priv->vbt.psr.tp1_wakeup_time_us =
> vbt_psr_to_us(is_waketime_options, psr_table->tp1_wakeup_time);
> + dev_priv->vbt.psr.tp2_tp3_wakeup_time_us =
> vbt_psr_to_us(is_waketime_options, psr_table->tp2_tp3_wakeup_time);
> }
>
> 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, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
