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

["Nagaraju, Vathsala" <vathsala.nagaraju@intel.com>]

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);
>>   }

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx