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

[Dhinakaran Pandiyan]

On Fri, 2018-11-30 at 13:18 -0800, Souza, Jose wrote:
> On Fri, 2018-11-30 at 11:35 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-11-29 at 17:00 -0800, Souza, Jose wrote:
> > > 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.
> > 
> > Do you have any numbers to justify that not entering deep sleep
> > (just
> > doing SU) is better than entering deep sleep and exiting?
> 
> I don't have power data, just stimations of how many frames it would
> state in each state.
> 
> > 
> > Even with a blinking cursor at 2 flips/second, there is enough time
> > to
> > wait for 9 idle frames (max currently), enter deep sleep and
> > exit(~2
> > frames) between flips.
> 
> 2 flips per second is too low, lets take a 4 flips per seconds(user
> will be typing faster than 2 keys per second):
> 
> # Base
> Modeset at 60hz
> 9 frames to activate PSR2
> 
> # With 1 frame to enter deep sleep
> 36 frames of PSR2 idle
> 4 frames of sleep
> 20 frames of deep sleep
> + tranning at every PSR2 exit
> 
> # With 15 frames to enter deep sleep
> 9 frames of PSR2 idle
> 4 frames of sleep reading memory
> 47 frames of sleep without read memory

We are comparing apples to oranges, I have no idea how much power this
SU sleep state actually saves in comparison to deep sleep. Without any
numbers, I'm not sold on the optimization you are proposing.

-DK

> * It would not enter in deep sleep so in the next second it would be
> 4 frames of sleep reading memory
> 56 frames of sleep without read memory
> 
> And when screen is realy idle like a savings screen it would still go
> to deep sleep.
> 
> > 
> > Why not leave EDP_PSR2_FRAMES_BEFORE_ACTIVATE as it is and reduce
> > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP to the minimum? But then again,
> > I'd
> > like to see some numbers if it's possible.
> 
> Other problem of leaving the number of frames to enter deep sleep to
> minumum is that it would almost never do selective update it would be
> like a PSR1.
> 
> 
> > 
> > -DK
> > 
> > > > 
> > > > >  
> > > > >  	/* 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;