[Intel-gfx] [PATCH 8/9] drm/i915/psr: Set the right frames values

[Souza, Jose]

On Thu, 2018-11-29 at 15:33 -0800, Dhinakaran Pandiyan wrote:
> 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 at intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at 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?

As said in commit message a small frame count to enter in deep sleep
will cause frequent PSR exits and when HW comes back from deep sleep it
needs to go to idle state. So it will need to wait for
EDP_PSR2_FRAMES_BEFORE_ACTIVATE() frames before activate PSR again.

A regular productivity tools(Office and email) user would benefit from
that as the mouse cursor blinking would make PSR2 go from deep sleep to
idle state and stay in idle as long as cursor is blinking. With 15
frames user will stay most of the time in PSR2 sleep state that already
provide some power savings.

> 
> 
> >  
> >  	/* 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;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20181130/8f50d0da/attachment.sig>