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

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: "José Roberto de Souza" <jose.souza@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 8/9] drm/i915/psr: Set the right frames values
Date: Thu, 29 Nov 2018 15:10:57 -0800	[thread overview]
Message-ID: <20181129231057.GT8630@intel.com> (raw)
In-Reply-To: <20181127003710.18618-8-jose.souza@intel.com>

On Mon, Nov 26, 2018 at 04:37:09PM -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);

Too many changes in a single patch that I couldn't understand why we
are removing the minimal of 6 that was our safe net.

> +	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);
>  
>  	/* 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;
> -- 
> 2.19.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx