On Wed, 02 May 2018, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> On Wed, May 02, 2018 at 02:43:29PM +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 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.
>>
>> 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 | 45 +++++++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/intel_psr.c | 38 ++++++++++++++++-----------------
>> 4 files changed, 68 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..a246b6b 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -687,8 +687,49 @@ 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.
>> + */
>
> Please follow this style with first empty line:
>
> /*
> * Text here
> */
>
> Start with capital N on New.
>
>> + if (bdb->version >= 209 && IS_KABYLAKE(dev_priv)) {
>
> Why are we filtering per platform?
> What about Coffelake? And Geminilake? And Cannonlake? and on?
It's a mess. I hate it, but I guess no can do.
>
> Btw, where is this spec? I just checked predator and it still
> only contains the old multiple of 100 one... :(
>
>> + 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;
>> + case 2:
>
> this out of order case is not good imho...
> confusing... but well... 0 -> 2500 -> 100 -> 500 is already confusing by itself
> honestly...
>
>> + default:
>
> why default is 2? higher?
> I always thought this awkward order was to make the 0 the default with 500
> or at least I was trying to make some sense on that order :/
The spec is silly. IMO they should have stuck to using usecs
directly. If the driver has to do a per platform mapping *anyway*, it's
better to do it from usecs directly than from some platform specific
index values.
Defaulting to longest when out of range is per my instruction. It should
handle the cases where the VBT actually has the usecs/100 value but
should have the new style. Did I mention this is a mess already?
BR,
Jani.
>
>> + if (psr_table->tp1_wakeup_time != 2)
>> + DRM_DEBUG_KMS("VBT tp1 wakeup time outside range, defaulting to max value 2500us\n");
>> + 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;
>> + case 2:
>> + default:
>> + if (psr_table->tp2_tp3_wakeup_time != 2)
>> + DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time outside range, defaulting to max value 2500us\n");
>> + 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..746c5ac 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,14 @@ 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 <= 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
