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

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

On Wed, 16 May 2018, vathsala nagaraju <vathsala.nagaraju@intel.com> 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@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?

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx