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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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;'?

>  	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