On Wed, 2018-05-16 at 11:08 +0300, Jani Nikula wrote:
> On Wed, 16 May 2018, vathsala nagaraju <vathsala.nagaraju@xxxxxxxxx>
> 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@xxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > CC: Puthikorn Voravootivat <puthik@xxxxxxxxxxxx>
> > > >
> > > > Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@xxxxxxxxx>
> > > > Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@xxxxxxxxx>
> > > > ---
> > > > 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);
> > > > }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
