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

[Puthikorn Voravootivat <puthik@chromium.org>]

FYI, Here is the the BDB version of the chip that need this change in
Google Pixelbook.

localhost ~ # uname -p
Intel(R) Core(TM) i5-7Y57 CPU @ 1.20GHz
localhost ~ # dmesg | grep VBT.signature
[    0.298850] VBT signature "$VBT SKYLAKE        ", BDB version 211

localhost ~ #  uname -p
Intel(R) Core(TM) i7-7Y75 CPU @ 1.30GHz
localhost ~ # dmesg | grep VBT.signature
[    0.273325] VBT signature "$VBT SKYLAKE        ", BDB version 211

Best,


On Thu, May 3, 2018 at 10:14 AM Nagaraju, Vathsala <
vathsala.nagaraju@intel.com> wrote:

> Rodrigo,

>          The changes are already in place on kabylake 209+ onwards, So
limiting  this change to only kabylake  and confirmed vbt version for now.
>          RCR is already raised for GOP team to resolve. Once we get the
other platform confirmation, we  can add those platforms too.

> Here is what happen in Skylake VBT description file , this is decimal ,
no wake options.
> [1] $PSR_TP_2_3_WaitTime_01 2 bytes ; TP2/TP3 wake up time in multiples
of 100
> [2] EditNum $PSR_TP_2_3_WaitTime_01, "TP2/TP3 WakeUp Time:", DEC,
>   Help "This field selects the link training TP2(Training Pattern2) or
TP3(Training Pattern3) time during PSR exit(wake up)\n"
>   "TP2/TP3 wake up time in multiples of 100us"

> Here is what happen in Kaby Lake VBT description file , this is
wakeoptions [0-3]
> [3] $PSR_TP_2_3_WaitTime_01 2 bytes ; TP2/TP3 wake up time in multiples
of 100
> [4] Combo $PSR_TP_2_3_WaitTime_01, "TP2/TP3 WakeUp Time:",
&PsrWakeupTimeOptions,
>   Help "This field selects the link training TP2(Training Pattern2) or
TP3(Training Pattern3) time during PSR exit(wake up)\n"
> [5] List &PsrWakeupTimeOptions↵
> Selection 0x00, "500 usec"↵
> Selection 0x01, "100 usec"↵
> Selection 0x02, "2.5 msec"↵
> Selection 0x03, "0 (Skip)"↵
> EndList↵

> Regards,
> Vathsala

> -----Original Message-----
> From: Vivi, Rodrigo
> Sent: Thursday, May 3, 2018 9:15 PM
> To: Nagaraju, Vathsala <vathsala.nagaraju@intel.com>
> Cc: jani.nikula@linux.intel.com; intel-gfx@lists.freedesktop.org;
Puthikorn Voravootivat <puthik@chromium.org>; Vaghela, Maulik V <
maulik.v.vaghela@intel.com>
> Subject: Re: [PATCH] drm/i915/psr: vbt change for psr

> On Thu, May 03, 2018 at 05:06:09PM +0530, vathsala nagaraju wrote:
> > From: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> >
> > For psr block #9, the vbt description has moved to options [0-3] for
> > TP1,TP2,TP3 Wakeup time from decimal value without any change to vbt
> > structure. Since spec does not  mention from which VBT version this
> > change was added to vbt.bsf file, we cannot depend on bdb->version
> > check to change for all the platforms.
> >
> > There is RCR inplace for GOP team to  provide the version number to
> > make generic change. Since Kabylake with bdb version 209 is having
> > this change, limiting this change to kbl and version 209+ to unblock
google.
> >
> > Tested on skl(bdb version 203,without options) and kabylake(bdb
> > version 209,212) having new options.
> >
> > bspec 20131
> >
> > v2: (Jani and Rodrigo)
> >     move the 165 version check to intel_bios.c
> > v3: Jani
> >     Move the abstraction to intel_bios.
> > v4: Jani
> >     Rename tp*_wakeup_time to have "us" suffix.
> >     For values outside range[0-3],default to max 2500us.
> >     Old decimal value was wake up time in multiples of 100us.
> > 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.
> >
> > 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 6268a51..a189382 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1077,8 +1077,8 @@ struct intel_vbt_data {
> >               bool require_aux_wakeup;
> >               int idle_frames;
> >               enum psr_lines_to_wait lines_to_wait;
> > -             int tp1_wakeup_time;
> > -             int tp2_tp3_wakeup_time;
> > +             int tp1_wakeup_time_us;
> > +             int tp2_tp3_wakeup_time_us;
> >       } psr;
> >
> >       struct {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 197c966..6bbd0b4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4084,10 +4084,10 @@ enum {
> >  #define   EDP_Y_COORDINATE_ENABLE    (1<<25) /* GLK and CNL+ */
> >  #define   EDP_MAX_SU_DISABLE_TIME(t) ((t)<<20)
> >  #define   EDP_MAX_SU_DISABLE_TIME_MASK       (0x1f<<20)
> > -#define   EDP_PSR2_TP2_TIME_500              (0<<8)
> > -#define   EDP_PSR2_TP2_TIME_100              (1<<8)
> > -#define   EDP_PSR2_TP2_TIME_2500     (2<<8)
> > -#define   EDP_PSR2_TP2_TIME_50               (3<<8)
> > +#define   EDP_PSR2_TP2_TIME_500us    (0<<8)
> > +#define   EDP_PSR2_TP2_TIME_100us    (1<<8)
> > +#define   EDP_PSR2_TP2_TIME_2500us   (2<<8)
> > +#define   EDP_PSR2_TP2_TIME_50us     (3<<8)
> >  #define   EDP_PSR2_TP2_TIME_MASK     (3<<8)
> >  #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
> >  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK      (0xf<<4)
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c
> > b/drivers/gpu/drm/i915/intel_bios.c
> > index 702d3fa..166f704 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -687,8 +687,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_KABYLAKE(dev_priv)) {

> Please file a BSpec bug and get that mess cleaned up first before
spreading it here.

> I'm looking to the same 20131 and it says "wake up time in multiples of
100" and VBT version 165

> Ok, I also see on 20131 the mention to:

> "
>   0 = 500 usec
>   1 = 100 usec
>   2 = 2.5 msec
>   3 = 0 usec (Skip TP1)
> "

> But it has absolutely no mention to "209" or "Kabylake" or any other
platform.

> I doubt that it would be for Kabylake and not for Coffelake for instance.


> Thanks,
> Rodrigo.

> > +             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 6233a32..f03dfba 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