Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Mon, 2018-11-26 at 16:37 -0800, José Roberto de Souza wrote:
> EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the number of
> frames that it should wait to enter PSR, what is wrong.
> Here it is setting this field with the highest value to avoid PSR2
> exits frequently, as when HW exit deep sleep it needs to go to idle
> state causing a PSR exit for then waiting a few frames before
> activate PSR2 again.
> This will result in more power saving as the sleep state also provide
> some power savings by doing selective updates instead of full screen
> updates.
> 
> About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of frames
> (not idle frames) that PSR2 hardware will wait to activate PSR2, so
> lets keep using the sink sync latency.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index ba7bbe3f8df2..6fd793fec5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>  	struct i915_psr *psr = &dev_priv->psr;
>  	u32 val;
>  
> -	/* Let's use 6 as the minimum to cover all known cases
> including the
> -	 * off-by-one issue that HW has in some cases.
> +	/* sink_sync_latency of 8 means source has to wait for more
> than 8
> +	 * frames, we'll go with 9 frames for now
>  	 */
> -	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> 1);
>  
> -	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> +	/* Avoid deep sleep as much as possible to avoid PSR2 idle
> state */
> +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
Avoid deep sleep as much as possible? Why? We get the best power
savings in deep sleep, why make it harder to achieve that?


> 
>  
>  	/* FIXME: selective update is probably totally broken because
> it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone
> isn't
> @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		val |= EDP_Y_COORDINATE_ENABLE;
>  
> -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> 1);
> -
>  	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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx