On Thu, 2018-11-29 at 15:10 -0800, Rodrigo Vivi wrote:
> 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.


The vbt idle_frames should not be used for PSR2 as PSR2 just wait for X
frames(idle or not) to activate PSR2 so just rely in sink sync_latency
should be enough but I can bring it back for safety.

> 
> > +	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
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel