[Intel-gfx] [PATCH] drm/i915/psr: vbt change for psr
vathsala nagaraju
vathsala.nagaraju at intel.com
Fri Apr 20 06:30:57 UTC 2018
On Thursday 19 April 2018 07:05 PM, Jani Nikula wrote:
> On Thu, 19 Apr 2018, vathsala nagaraju <vathsala.nagaraju at intel.com> wrote:
>> From: Vathsala Nagaraju <vathsala.nagaraju at 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.
> This is an incredible mess.
>
>> 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
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> CC: Puthikorn Voravootivat <puthik at chromium.org>
>>
>> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela at intel.com>
>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_bios.c | 40 ++++++++++++++++++++++++++++++++++++---
>> drivers/gpu/drm/i915/intel_psr.c | 26 ++++++++++++-------------
>> 2 files changed, 50 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index 702d3fa..8913dc8 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -646,6 +646,15 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>> }
>> }
>>
>> +static bool
>> +is_psr_options(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>> +{
>> + if (bdb->version >= 209 && IS_KABYLAKE(dev_priv))
>> + return true;
>> + else
>> + return false;
>> +}
>> +
>> static void
>> parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>> {
>> @@ -658,7 +667,6 @@ static int intel_bios_ssc_frequency(struct drm_i915_private *dev_priv,
>> DRM_DEBUG_KMS("No PSR BDB found.\n");
>> return;
>> }
>> -
>> psr_table = &psr->psr_table[panel_type];
>>
>> dev_priv->vbt.psr.full_link = psr_table->full_link;
>> @@ -687,8 +695,34 @@ 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 old decimal value interpretation
>> + * 0 [500 us] > 1 [500 us ]
>> + * 1 [100 us] > 0 [100 us ]
>> + * 2 [2.5 ms] > 5 [2.5 ms ]
>> + * 3 [0 us] = 0 [0 us ]
> The old decimal value stuff was wake up time in multiples of 100 us.
>
>> + */
>> + if (!is_psr_options(dev_priv, bdb)) {
> You only use is_psr_options here once, please just open code the
> condition. Also reverse order to not need !something in the condition.
>
>> + if (psr_table->tp1_wakeup_time > 5)
>> + dev_priv->vbt.psr.tp1_wakeup_time = 2;
>> + else if (psr_table->tp1_wakeup_time > 1)
>> + dev_priv->vbt.psr.tp1_wakeup_time = 0;
>> + else if (psr_table->tp1_wakeup_time > 0)
>> + dev_priv->vbt.psr.tp1_wakeup_time = 1;
>> + else
>> + dev_priv->vbt.psr.tp1_wakeup_time = 3;
>> +
>> + if (psr_table->tp2_tp3_wakeup_time > 5)
>> + dev_priv->vbt.psr.tp2_tp3_wakeup_time = 2;
>> + else if (psr_table->tp2_tp3_wakeup_time > 1)
>> + dev_priv->vbt.psr.tp2_tp3_wakeup_time = 0;
>> + else if (psr_table->tp1_wakeup_time > 0)
>> + dev_priv->vbt.psr.tp2_tp3_wakeup_time = 1;
>> + else
>> + dev_priv->vbt.psr.tp2_tp3_wakeup_time = 3;
>> + } else {
>> + 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;
>> + }
>> }
> Please rename dev_priv->vbt.psr tp1_wakeup_time and tp2_tp3_wakeup_time
> to have _us suffix, and actually assign the wakeup time in us
> there. Hide all the hideous, hideous VBT stuff behind that, and doesn't
> use magic numbers all over the place.
>
> The old format becomes wakeup_time_us = vbt_value * 100. The code should
> handle mismatches between the value and what the hardware can do (see
> below).
>
> The new format should just be a switch-case mapping values to us,
> whining about values other than 0..3 and defaulting to max in that case.
if we don't set anything in SRD_CTL/PSR2_CTL reg for those bits , by
default it's 0 [which is 500 us]
instead of defaulting to max value which is 3[0us], should we just
default to 0[500us]
>>
>> 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 69a5b27..95658ad 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -353,21 +353,21 @@ 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)
>> + if (dev_priv->vbt.psr.tp1_wakeup_time == 0)
>> val |= EDP_PSR_TP1_TIME_500us;
>> - else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
>> + else if (dev_priv->vbt.psr.tp1_wakeup_time == 1)
>> val |= EDP_PSR_TP1_TIME_100us;
>> + else if (dev_priv->vbt.psr.tp1_wakeup_time == 2)
>> + val |= EDP_PSR_TP1_TIME_2500us;
>> else
>> val |= EDP_PSR_TP1_TIME_0us;
>>
>> - 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 == 0)
>> + val |= EDP_PSR_TP2_TP3_TIME_500us;
>> + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 1)
>> val |= EDP_PSR_TP2_TP3_TIME_100us;
>> + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 2)
>> + val |= EDP_PSR_TP2_TP3_TIME_2500us;
>> else
>> val |= EDP_PSR_TP2_TP3_TIME_0us;
> Rewrite these to round up the longer wait:
>
> if (wakeup_time_us == 0)
> val |= EDP_PSR_TP2_TP3_TIME_0us;
> else if (wakeup_time_us <= 100)
> val |= EDP_PSR_TP2_TP3_TIME_100us;
> else if (wakeup_time_us <= 500)
> val |= EDP_PSR_TP2_TP3_TIME_500us;
> else
> val |= EDP_PSR_TP2_TP3_TIME_2500us;
>
>>
>> @@ -406,12 +406,12 @@ 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)
>> + if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 0)
>> val |= EDP_PSR2_TP2_TIME_500;
>> - else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
>> + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 1)
>> val |= EDP_PSR2_TP2_TIME_100;
>> + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time == 2)
>> + val |= EDP_PSR2_TP2_TIME_2500;
>> else
>> val |= EDP_PSR2_TP2_TIME_50;
> Same here.
>
> BR,
> Jani.
>
More information about the Intel-gfx
mailing list