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

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

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.

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