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

[vathsala nagaraju <vathsala.nagaraju@intel.com>]

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@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
>>
>> 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/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?
> } 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);
>>   }

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