[Intel-gfx] [PATCH 37/40] drm/i915: Fix eDP link training when switching pipes

[Daniel Vetter]

On Tue, Jul 29, 2014 at 10:18:46PM +0300, Ville Syrjälä wrote:
> On Tue, Jul 29, 2014 at 08:06:57PM +0200, Daniel Vetter wrote:
> > On Mon, Jun 30, 2014 at 02:52:12PM -0700, Jesse Barnes wrote:
> > > On Sat, 28 Jun 2014 02:04:28 +0300
> > > ville.syrjala at linux.intel.com wrote:
> > > 
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > When switching from one pipe to another, the power sequencer of the new
> > > > pipe seems to need a bit of kicking to lock into the port. Even the vdd
> > > > force bit doesn't work before the power sequencer has been sufficiently
> > > > kicked, so this must be done even before any AUX transactions.
> > > > 
> > > > This sequence has been found to do the trick:
> > > > 1) enable port with idle pattern
> > > > 2) enable the power sequencer
> > > > 3) proceed with link training
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 34 ++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 32 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 65ab54c..07b0320 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -2010,6 +2010,37 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
> > > >  	mutex_unlock(&dev_priv->dpio_lock);
> > > >  }
> > > >  
> > > > +static void intel_edp_init_train(struct intel_dp *intel_dp)
> > > > +{
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +
> > > > +	if (!is_edp(intel_dp))
> > > > +		return;
> > 
> > This changes the order of events as observed by the sink, so I really
> > wonder why this is edp specific?
> 
> It's not really. I need to kick the power sequencer for regular DP ports
> too (in a later patch), and recently I started to wonder if we also need
> it for HDMI ports but I didn't test that theory yet.
> 
> Based on my observations there are several problems intermingled here:
> 1. the power sequencer prevents the port from starting up until the
>    power up sequence has finished
> 2. vdd force bit doesn't work until the power sequencer has finished
> 3. the power sequencer won't finish the power up sequence unless idle
>    pattern is used
> 
> So the fix is to enable the port with idle pattern and enable the power
> sequencer even before doing any aux transactions (including the sink
> dpms write).
> 
> Once the power sequencer has finished powering up on to the port once.
> the vdd force bit will keep working on the port even if the port and
> power sequencer are later disabled. Also iirc the power sequencer will
> no longer prevent the port from starting up even if the power sequencer
> is left disabled when re-enabling the port later. But the same problem
> will reappear when we change the pipe->port mapping, and then we need
> to kick the power sequencer again.
> 
> > We do have bug reports about external DP monitors not waking up from the
> > sink_dpms call properly ...
> 
> On vlv or something else? I'm not quite sure if the same problems would
> be possible on other platforms since they only have one power sequencer.
> But maybe that too locks into the port and would need a similar kick.
> 
> But IIRC on PCH platforms the spec says that we must enable the port
> with training pattern 1. So the use of idle pattern would at least go
> against the spec. Which is why I left that part as vlv/chv specific.

Hm ... tricky. And especially since it seems to be required only once. I'm
just concerned about the slight behavioural difference between the general
dp link enabling where we do the sink dpms aux transaction first, then
start with link training. But on vlv/chv here we first kick the port a bit
and enable the idle pattern. If now a few displays don't like this or
require this we have a problem.

In general I really want us to try as hard as possible to have 0
differences in sink-visible behaviour. DP is simply too fickle imo. But if
there's nothing we can do I guess good explanations in commit message and
comments is all I can ask for. I.e. something like the above should go
into the commit message and maybe we should make the comment a bit more
dangerously-sounding.
-Daniel
> 
> > -Daniel
> > 
> > > > +
> > > > +	/*
> > > > +	 * Need to enable the port with idle pattern to allow the power
> > > > +	 * sequencer to lock into the port. Otherwise the power sequencer
> > > > +	 * (including vdd force bit!) doesn't work on this port.
> > > > +	 */
> > > > +	if (IS_VALLEYVIEW(dev)) {
> > > > +		intel_dp->DP |= DP_PORT_EN;
> > > > +
> > > > +		if (IS_CHERRYVIEW(dev))
> > > > +			intel_dp->DP &= ~DP_LINK_TRAIN_MASK_CHV;
> > > > +		else
> > > > +			intel_dp->DP &= ~DP_LINK_TRAIN_MASK;
> > > > +		intel_dp->DP |= DP_LINK_TRAIN_PAT_IDLE;
> > > > +
> > > > +		I915_WRITE(intel_dp->output_reg, intel_dp->DP);
> > > > +		POSTING_READ(intel_dp->output_reg);
> > > > +	}
> > > > +
> > > > +	intel_edp_panel_on(intel_dp);
> > > > +	edp_panel_vdd_off(intel_dp, true);
> > > > +}
> > > > +
> > > >  static void intel_enable_dp(struct intel_encoder *encoder)
> > > >  {
> > > >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > > > @@ -2021,10 +2052,9 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> > > >  		return;
> > > >  
> > > >  	intel_edp_panel_vdd_on(intel_dp);
> > > > +	intel_edp_init_train(intel_dp);
> > > >  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > > >  	intel_dp_start_link_train(intel_dp);
> > > > -	intel_edp_panel_on(intel_dp);
> > > > -	edp_panel_vdd_off(intel_dp, true);
> > > >  	intel_dp_complete_link_train(intel_dp);
> > > >  	intel_dp_stop_link_train(intel_dp);
> > > >  }
> > > 
> > > Yeah I think this matches the doc too.  I never pushed this change
> > > because I could never find anything that it actually fixed.
> > > 
> > > I guess you have something now though!
> > > 
> > > Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > > 
> > > -- 
> > > Jesse Barnes, Intel Open Source Technology Center
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch