Re: [Intel-gfx] [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again"

[Jani Nikula]

On Thu, 08 Sep 2016, Jani Nikula  wrote:
> On Thu, 08 Sep 2016, Ville Syrjälä  wrote:
>> On Wed, Sep 07, 2016 at 05:42:31PM -0700, Rodrigo Vivi wrote:
>>> This reverts commit 1c80c25fb622973dd135878e98d172be20859049.
>>> 
>>> There are panels that needs 4 idle frames before entering PSR,
>>> but VBT is unproperly set.
>>> 
>>> Also lately it was identified that idle frame count calculated at HW
>>> can be off by 1, what makes the minimum of 2, at least.
>>> 
>>> Without the current vbt+1 we are with the risk of having HW calculating
>>> 0 idle frames and entering PSR when it shouldn't. Regardless the lack
>>> of link training.
>>> 
>>> Cc: Dominik Brodowski 
>>> Cc: Jani Nikula 
>>> Cc: Daniel Vetter 
>>> Signed-off-by: Rodrigo Vivi 
>>> ---
>>>  drivers/gpu/drm/i915/intel_psr.c | 14 +++---
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
>>> b/drivers/gpu/drm/i915/intel_psr.c
>>> index 59a21c9..108ba1e 100644
>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>> @@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp 
>>> *intel_dp)
>>> struct drm_i915_private *dev_priv = to_i915(dev);
>>>  
>>> uint32_t max_sleep_time = 0x1f;
>>> -   /* Lately it was identified that depending on panel idle frame count
>>> -* calculated at HW can be off by 1. So let's use what came
>>> -* from VBT + 1.
>>> -* There are also other cases where panel demands at least 4
>>> -* but VBT is not being set. To cover these 2 cases lets use
>>> -* at least 5 when VBT isn't set to be on the safest side.
>>> +   /*
>>> +* Let's respect VBT in case VBT asks a higher idle_frame value.
>>> +* Let's use 6 as the minimum to cover all known cases including
>>> +* the off-by-one issue that HW has in some cases. Also there are
>>> +* cases where sink should be able to train
>>> +* with the 5 or 6 idle patterns.
>>>  */
>>> -   uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
>>> +   uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>>
>> I don't really understand this logic compared to the explanations.
>>
>> Shouldn't we do something like 'idle_frames = max(4, psr.idle_frames) + 1;'?
>
> We're at rc5, smells like revert, not trial and error.

Pushed.

BR,
Jani.

>
> Side note, looking at the VBT spec on tp1, tp2/tp3 wakeup times, there
> seems to be some confusion about what the values mean. Has there perhaps
> been a change in the spec? What's the VBT version where the problems
> happen?
>
> BR,
> Jani.
>
>>
>>> uint32_t val = EDP_PSR_ENABLE;
>>>  
>>> val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
>>> -- 
>>> 1.9.1
>>> 
>>> ___
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again"

[Jani Nikula]

On Thu, 08 Sep 2016, Ville Syrjälä  wrote:
> On Wed, Sep 07, 2016 at 05:42:31PM -0700, Rodrigo Vivi wrote:
>> This reverts commit 1c80c25fb622973dd135878e98d172be20859049.
>> 
>> There are panels that needs 4 idle frames before entering PSR,
>> but VBT is unproperly set.
>> 
>> Also lately it was identified that idle frame count calculated at HW
>> can be off by 1, what makes the minimum of 2, at least.
>> 
>> Without the current vbt+1 we are with the risk of having HW calculating
>> 0 idle frames and entering PSR when it shouldn't. Regardless the lack
>> of link training.
>> 
>> Cc: Dominik Brodowski 
>> Cc: Jani Nikula 
>> Cc: Daniel Vetter 
>> Signed-off-by: Rodrigo Vivi 
>> ---
>>  drivers/gpu/drm/i915/intel_psr.c | 14 +++---
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
>> b/drivers/gpu/drm/i915/intel_psr.c
>> index 59a21c9..108ba1e 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp 
>> *intel_dp)
>>  struct drm_i915_private *dev_priv = to_i915(dev);
>>  
>>  uint32_t max_sleep_time = 0x1f;
>> -/* Lately it was identified that depending on panel idle frame count
>> - * calculated at HW can be off by 1. So let's use what came
>> - * from VBT + 1.
>> - * There are also other cases where panel demands at least 4
>> - * but VBT is not being set. To cover these 2 cases lets use
>> - * at least 5 when VBT isn't set to be on the safest side.
>> +/*
>> + * Let's respect VBT in case VBT asks a higher idle_frame value.
>> + * Let's use 6 as the minimum to cover all known cases including
>> + * the off-by-one issue that HW has in some cases. Also there are
>> + * cases where sink should be able to train
>> + * with the 5 or 6 idle patterns.
>>   */
>> -uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
>> +uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>
> I don't really understand this logic compared to the explanations.
>
> Shouldn't we do something like 'idle_frames = max(4, psr.idle_frames) + 1;'?

We're at rc5, smells like revert, not trial and error.

Side note, looking at the VBT spec on tp1, tp2/tp3 wakeup times, there
seems to be some confusion about what the values mean. Has there perhaps
been a change in the spec? What's the VBT version where the problems
happen?

BR,
Jani.

>
>>  uint32_t val = EDP_PSR_ENABLE;
>>  
>>  val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
>> -- 
>> 1.9.1
>> 
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [Intel-gfx] [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again"

[Ville Syrjälä]

On Wed, Sep 07, 2016 at 05:42:31PM -0700, Rodrigo Vivi wrote:
> This reverts commit 1c80c25fb622973dd135878e98d172be20859049.
> 
> There are panels that needs 4 idle frames before entering PSR,
> but VBT is unproperly set.
> 
> Also lately it was identified that idle frame count calculated at HW
> can be off by 1, what makes the minimum of 2, at least.
> 
> Without the current vbt+1 we are with the risk of having HW calculating
> 0 idle frames and entering PSR when it shouldn't. Regardless the lack
> of link training.
> 
> Cc: Dominik Brodowski 
> Cc: Jani Nikula 
> Cc: Daniel Vetter 
> Signed-off-by: Rodrigo Vivi 
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 59a21c9..108ba1e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp 
> *intel_dp)
>   struct drm_i915_private *dev_priv = to_i915(dev);
>  
>   uint32_t max_sleep_time = 0x1f;
> - /* Lately it was identified that depending on panel idle frame count
> -  * calculated at HW can be off by 1. So let's use what came
> -  * from VBT + 1.
> -  * There are also other cases where panel demands at least 4
> -  * but VBT is not being set. To cover these 2 cases lets use
> -  * at least 5 when VBT isn't set to be on the safest side.
> + /*
> +  * Let's respect VBT in case VBT asks a higher idle_frame value.
> +  * Let's use 6 as the minimum to cover all known cases including
> +  * the off-by-one issue that HW has in some cases. Also there are
> +  * cases where sink should be able to train
> +  * with the 5 or 6 idle patterns.
>*/
> - uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
> + uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);

I don't really understand this logic compared to the explanations.

Shouldn't we do something like 'idle_frames = max(4, psr.idle_frames) + 1;'?

>   uint32_t val = EDP_PSR_ENABLE;
>  
>   val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> -- 
> 1.9.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx