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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com wrote:
> > 
> > > From: Ville Syrjälä <ville.syrjala@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@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.

> -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@virtuousgeek.org>
> > 
> > -- 
> > Jesse Barnes, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@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