Re: [PATCH] drm/i915/psr: Set idle frame count based on sink synchronization latency

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

On Tue, 2018-05-29 at 12:51 -0700, Rodrigo Vivi wrote:
> On Thu, May 24, 2018 at 08:30:47PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > DPCD 2009h "Synchronization latency in sink" has bits that tell us
> > the
> > maximum number of frames sink can take to resynchronize to source
> > timing
> > when exiting PSR. More importantly, as per eDP 1.4b, this is the
> > "Minimum
> > number of frames following PSR exit that the Source device needs to
> > wait for PSR entry."
> > 
> > We currently use this value only to setup the number frames to wait
> > before
> > PSR2 selective update. But, based on the above description it makes
> > more
> > sense to use this to configure idle frames for both PSR1 and and
> > PSR2. This
> > will ensure we wait the required number of frames before
> > activation whether it is PSR1 or PSR2.
> > 
> > The minimum number of idle frames remains 6, while allowing sink
> > synchronization latency and VBT to increase this value.
> > 
> > This also solves the flip-flop between sink and source frames that
> > I
> > noticed on my Thinkpad X260 during PSR exit. This specific panel
> > has a
> > value of 8h, which according to the spec means the "Source device
> > must
> > wait for more than eight active frames after PSR exit before
> > initiating PSR
> > entry. (In this case, should be provided by the panel supplier.)"
> > VBT
> > however has a value of 0.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Jose Roberto de Souza <jose.souza@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 40 ++++++++++++++++++++------
> > --------------
> >  1 file changed, 20 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index ebc483f06c6f..71dfe541740f 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -247,6 +247,8 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  		return;
> >  	}
> >  	dev_priv->psr.sink_support = true;
> > +	dev_priv->psr.sink_sync_latency =
> > +		intel_dp_get_sink_sync_latency(intel_dp);
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9 &&
> >  	    (intel_dp->psr_dpcd[0] ==
> > DP_PSR2_WITH_Y_COORD_IS_SUPPORTED)) {
> > @@ -272,8 +274,6 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  		if (dev_priv->psr.sink_psr2_support) {
> >  			dev_priv->psr.colorimetry_support =
> >  				intel_dp_get_colorimetry_status(in
> > tel_dp);
> > -			dev_priv->psr.sink_sync_latency =
> > -				intel_dp_get_sink_sync_latency(int
> > el_dp);
> >  		}
> >  	}
> >  }
> > @@ -370,21 +370,21 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  	struct intel_digital_port *dig_port =
> > dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	u32 max_sleep_time = 0x1f;
> > +	u32 val = EDP_PSR_ENABLE;
> >  
> > -	uint32_t max_sleep_time = 0x1f;
> > -	/*
> > -	 * Let's respect VBT in case VBT asks a higher idle_frame
> > value.
> > -	 * Let's use 6 as the minimum to cover all known cases
> > including
> > -	 * the off-by-one issue that HW has in some cases. Also
> > there are
> > -	 * cases where sink should be able to train
> > -	 * with the 5 or 6 idle patterns.
> > +	/* Let's use 6 as the minimum to cover all known cases
> > including the
> > +	 * off-by-one issue that HW has in some cases.
> >  	 */
> > -	uint32_t idle_frames = max(6, dev_priv-
> > >vbt.psr.idle_frames);
> > -	uint32_t val = EDP_PSR_ENABLE;
> > +	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> >  
> > -	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> > +	/* sink_sync_latency of 8 means source has to wait for
> > more than 8
> > +	 * frames, we'll go with 9 frames for now
> > +	 */
> > +	idle_frames = max(idle_frames, dev_priv-
> > >psr.sink_sync_latency + 1);
> >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> >  
> > +	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> >  	if (IS_HASWELL(dev_priv))
> >  		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> >  
> > @@ -424,15 +424,15 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	struct intel_digital_port *dig_port =
> > dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	/*
> > -	 * Let's respect VBT in case VBT asks a higher idle_frame
> > value.
> > -	 * Let's use 6 as the minimum to cover all known cases
> > including
> > -	 * the off-by-one issue that HW has in some cases. Also
> > there are
> > -	 * cases where sink should be able to train
> > -	 * with the 5 or 6 idle patterns.
> > +	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.
> >  	 */
> > -	uint32_t idle_frames = max(6, dev_priv-
> > >vbt.psr.idle_frames);
> > -	u32 val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> > +	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > +
> > +	idle_frames = max(idle_frames, dev_priv-
> > >psr.sink_sync_latency + 1);
> > +	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> I wonder if we should consolidate all this login in a single function
> since they
> are identical and only the shift is different...
> 
> But the logic is correct. I checked eDP 1.3 and 1.4 specs and I
> believe we
> need to move forward with this change and do clean-ups on follow-ups.
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Also I don't believe that we need cc:stable because we never enabled
> it anywhere
> anyway besides the fact that it wont be a clean backport.

Yeah, we've had a couple of commits that touch this code, won't be a
clean back port.
 
Thanks for reviewing and merging this patch.

> 
> > 
> >  
> >  	/* FIXME: selective update is probably totally broken
> > because it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw
> > alone isn't
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx