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

[vathsala nagaraju <vathsala.nagaraju@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx