| Message ID | 1526633726-22145-1-git-send-email-vathsala.nagaraju@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 18 May 2018, 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. > v8: Puthik > add new function vbt_psr_to_us. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Puthikorn Voravootivat <puthik@chromium.org> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > > 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 | 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); > }
On 5/18/2018 3:01 PM, Jani Nikula wrote: > On Fri, 18 May 2018, 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. >> v8: Puthik >> add new function vbt_psr_to_us. >> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Puthikorn Voravootivat <puthik@chromium.org> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> Cc: José Roberto de Souza <jose.souza@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> >> 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 | 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? Which parts you find it complicated? new version is 1) check if vbt data is wake_options 2) if wake options , then if out of range print the message 3)then for both new and old version , we call vbt_psr_to_us , which takes care of the mapping and conversion to us. > > I'd rather take the previous version, even with the duplication. puthik suggested to use vbt_psr_to_us to remove code duplication. Puthik , is it okay if we switch to v7 version? > 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); >> }
On Tue, 22 May 2018, "Nagaraju, Vathsala" <vathsala.nagaraju@intel.com> wrote: > On 5/18/2018 3:01 PM, Jani Nikula wrote: >> On Fri, 18 May 2018, 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. >>> v8: Puthik >>> add new function vbt_psr_to_us. >>> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: Puthikorn Voravootivat <puthik@chromium.org> >>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> Cc: José Roberto de Souza <jose.souza@intel.com> >>> Cc: Jani Nikula <jani.nikula@intel.com> >>> >>> 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 | 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? > Which parts you find it complicated? This splits and duplicates the checks for valid values to two places that you need to keep in sync as the spec inevitably gets updated. The responsibilities between the caller and the callee are unclear, which is just silly for such a small amount of code. You have checks for negative values but the field is unsigned. I could go on. The bottom line is that the spec makes a fundamentally simple thing unnecessarily complicated, and it's our job to implement that in a way that makes it again simple for human beings. That's the primary goal, make the code easy for the reader. We have enough hard things, we should not have to stop and think about simple things. BR, Jani. > new version is > 1) check if vbt data is wake_options > 2) if wake options , then if out of range print the message > 3)then for both new and old version , we call vbt_psr_to_us , which > takes care of the mapping and conversion to us. >> >> I'd rather take the previous version, even with the duplication. > puthik suggested to use vbt_psr_to_us to remove code duplication. > Puthik , is it okay if we switch to v7 version? > >> 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); >>> } >
On 5/22/2018 1:35 PM, Jani Nikula wrote: > On Tue, 22 May 2018, "Nagaraju, Vathsala" <vathsala.nagaraju@intel.com> wrote: >> On 5/18/2018 3:01 PM, Jani Nikula wrote: >>> On Fri, 18 May 2018, 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. >>>> v8: Puthik >>>> add new function vbt_psr_to_us. >>>> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Cc: Puthikorn Voravootivat <puthik@chromium.org> >>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>>> Cc: José Roberto de Souza <jose.souza@intel.com> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> >>>> 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 | 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? >> Which parts you find it complicated? > This splits and duplicates the checks for valid values to two places > that you need to keep in sync as the spec inevitably gets updated. The > responsibilities between the caller and the callee are unclear, which is > just silly for such a small amount of code. You have checks for negative > values but the field is unsigned. I could go on. > > The bottom line is that the spec makes a fundamentally simple thing > unnecessarily complicated, and it's our job to implement that in a way > that makes it again simple for human beings. That's the primary goal, > make the code easy for the reader. We have enough hard things, we should > not have to stop and think about simple things. > > BR, > Jani. > Thanks, will stick with the previous version (v7) >> new version is >> 1) check if vbt data is wake_options >> 2) if wake options , then if out of range print the message >> 3)then for both new and old version , we call vbt_psr_to_us , which >> takes care of the mapping and conversion to us. >>> I'd rather take the previous version, even with the duplication. >> puthik suggested to use vbt_psr_to_us to remove code duplication. >> Puthik , is it okay if we switch to v7 version? >> >>> 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); >>>> }
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); + } + } + + 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); }