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

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

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?

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.

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.

-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;

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