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

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5705 bytes --]

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@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?
> > 
> > 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
* 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;

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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