Re: [PATCH] drm/i915: Change order of operations for VLV/CHV to not train DP link before PHYs are ready

[Todd Previte <tprevite@xxxxxxxxx>]

On 10/10/14 1:04 AM, Jani Nikula wrote:
> On Thu, 09 Oct 2014, Todd Previte<tprevite@xxxxxxxxx>  wrote:
> > Reorder the function calls in chv/vlv_pre_enable_dp() such that link training is not initiated
> > before the PHYs come up out of reset. Also check the status of vlv_wait_port_ready() and
> > only attempt to train if the PHYs are actually running.
> >
> > The specification lists the wait for the PHYs as one of the final steps in enabling
> > the Displayport hardware for use.  While the PHYs are in reset, no communication is p
> > ossible across the link. Attempting to train the link while the PHYs are in reset will
> > result in link training failure with one or more WARN() in the logs. Moving the
> > intel_enable_dp() function after vlv_wait_port_ready() and only when the PHYs are ready
> > helps ensure reliable operation of the Displayport link.
> There's been some back and forth about the mode set sequence ordering in
> the past. There's also some disagreement between some specs about
> this. The published display spec at 01.org says,
>
> • Enable ports
> • Wait for DPIO phystatus ready in 6014
> • Enable pipe A/B
> • Enable planes (VGA or hires)
>
> This is in line with the current implementation. However, there's no
> details about the link training - perhaps you could try waiting for DPIO
> phystatus ready right after intel_dp_enable_port() but before link
> training?
>
> BR,
> Jani.
>
>
> PS. Please wrap the commit messages at, say, 72 characters. Below 80
> anyway.
Thanks for the feedback, Jani. That might be a good option. That 
essentially moves the wait_port_ready into the intel_enable_dp() 
function, so it would need a platform check there as well. Not a big 
deal, really. Alternatively,  it looks like intel_dp_enable_port() can 
be moved out of intel_enable_dp() and placed right before the call to 
vlv_wait_port_ready(). There's only a few places where intel_enable_dp() 
is called, it shouldn't be too much of an issue to pull 
intel_dp_enable_port() out and place it where necessary. I'll post the 
updated patch as soon as it's ready.

I'll amend the commit message and wrap it under 80 chars, too.

-T

> > Signed-off-by: Todd Previte<tprevite@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c |  8 ++++++--
> >   drivers/gpu/drm/i915/intel_dp.c      | 10 ++++------
> >   drivers/gpu/drm/i915/intel_drv.h     |  2 +-
> >   3 files changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c51d950..4b280c1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1723,7 +1723,7 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
> >   	mutex_unlock(&dev_priv->dpio_lock);
> >   }
> >   
> > -void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> > +int vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >   		struct intel_digital_port *dport)
> >   {
> >   	u32 port_mask;
> > @@ -1746,9 +1746,13 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >   		BUG();
> >   	}
> >   
> > -	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000))
> > +	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000)) {
> >   		WARN(1, "timed out waiting for port %c ready: 0x%08x\n",
> >   		     port_name(dport->port), I915_READ(dpll_reg));
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> >   }
> >   
> >   static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index a8352c4..ada8b07 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2705,9 +2705,8 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
> >   		pps_unlock(intel_dp);
> >   	}
> >   
> > -	intel_enable_dp(encoder);
> > -
> > -	vlv_wait_port_ready(dev_priv, dport);
> > +	if (vlv_wait_port_ready(dev_priv, dport) == 0)
> > +	    intel_enable_dp(encoder);
> >   }
> >   
> >   static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
> > @@ -2805,9 +2804,8 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
> >   		pps_unlock(intel_dp);
> >   	}
> >   
> > -	intel_enable_dp(encoder);
> > -
> > -	vlv_wait_port_ready(dev_priv, dport);
> > +	if (vlv_wait_port_ready(dev_priv, dport) == 0)
> > +	    intel_enable_dp(encoder);
> >   }
> >   
> >   static void chv_dp_pre_pll_enable(struct intel_encoder *encoder)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index dc80444..2ff2c8c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -876,7 +876,7 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
> >   	drm_wait_one_vblank(dev, pipe);
> >   }
> >   int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
> > -void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> > +int vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> >   			 struct intel_digital_port *dport);
> >   bool intel_get_load_detect_pipe(struct drm_connector *connector,
> >   				struct drm_display_mode *mode,
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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