Re: [PATCH] drm/i915/psr: vbt change for psr

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 20 Apr 2018, vathsala nagaraju <vathsala.nagaraju@intel.com> wrote:
> On Thursday 19 April 2018 07:05 PM, Jani Nikula wrote:
>> On Thu, 19 Apr 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 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@intel.com>
>>> CC: Puthikorn Voravootivat <puthik@chromium.org>
>>>
>>> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela@intel.com>
>>> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@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]

Like I said, if we think the VBT has the new format, and it has values
in range 0..3, map those to corresponding delays in us. 0 -> 500 us, 1
-> 100 us, 2 -> 2500 us, 3 -> 0 us. If the value is not in range 0..3,
*then* default to max i.e. 2500 us.

BR,
Jani.


>>>   
>>>   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.
>>
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx