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

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

On Thu, 08 Sep 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 08 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> 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 <linux@dominikbrodowski.net>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>  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