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.
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);
>>> }
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
