Re: [PATCH 09/14] drm/i915: Hide underruns from eDP PLL and port enable on ILK

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

On Thu, Oct 29, 2015 at 12:39:53PM -0700, Jesse Barnes wrote:
> On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We get underruns on the other pipe when enabling the CPU eDP PLL and
> > port on ILK.
> > 
> > Bspec knows about the PLL issue, and recommends doing a vblank wait just
> > prior to enabling the PLL. That does seem to help, but unfortunately we
> > get another underrun when actually enabling the CPU eDP port. Bspec
> > doesn't mention that at all, and the same vblank wait trick doesn't
> > appear to be effective there.
> > 
> > Since I have no better clue how to deal with this, just hide the errors.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 34 +++++++++++++++++++++++++++++++---
> >  1 file changed, 31 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 4a0fb63..0b9b440 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2575,6 +2575,8 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> >  	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> > +	enum port port = dp_to_dig_port(intel_dp)->port;
> > +	enum pipe pipe = crtc->pipe;
> >  
> >  	if (WARN_ON(dp_reg & DP_PORT_EN))
> >  		return;
> > @@ -2586,6 +2588,17 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  
> >  	intel_dp_enable_port(intel_dp);
> >  
> > +	if (port == PORT_A && IS_GEN5(dev_priv)) {
> > +		/*
> > +		 * Underrun reporting for the other pipe was disabled in
> > +		 * g4x_pre_enable_dp(). The eDP PLL and port have now been
> > +		 * enabled, so it's now safe to re-enable underrun reporting.
> > +		 */
> > +		intel_wait_for_vblank_if_active(dev_priv->dev, !pipe);
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, true);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, true);
> > +	}
> > +
> >  	edp_panel_vdd_on(intel_dp);
> >  	edp_panel_on(intel_dp);
> >  	edp_panel_vdd_off(intel_dp, true);
> > @@ -2608,7 +2621,7 @@ static void intel_enable_dp(struct intel_encoder *encoder)
> >  
> >  	if (crtc->config->has_audio) {
> >  		DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> > -				 pipe_name(crtc->pipe));
> > +				 pipe_name(pipe));
> >  		intel_audio_codec_enable(encoder);
> >  	}
> >  }
> > @@ -2631,13 +2644,28 @@ static void vlv_enable_dp(struct intel_encoder *encoder)
> >  
> >  static void g4x_pre_enable_dp(struct intel_encoder *encoder)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > -	struct intel_digital_port *dport = dp_to_dig_port(intel_dp);
> > +	enum port port = dp_to_dig_port(intel_dp)->port;
> > +	enum pipe pipe = to_intel_crtc(encoder->base.crtc)->pipe;
> >  
> >  	intel_dp_prepare(encoder);
> >  
> > +	if (port == PORT_A && IS_GEN5(dev_priv)) {
> > +		/*
> > +		 * We get FIFO underruns on the other pipe when
> > +		 * enabling the CPU eDP PLL, and when enabling CPU
> > +		 * eDP port. We could potentially avoid the PLL
> > +		 * underrun with a vblank wait just prior to enabling
> > +		 * the PLL, but that doesn't appear to help the port
> > +		 * enable case. Just sweep it all under the rug.
> > +		 */
> > +		intel_set_cpu_fifo_underrun_reporting(dev_priv, !pipe, false);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, !pipe, false);
> > +	}
> > +
> >  	/* Only ilk+ has port A */
> > -	if (dport->port == PORT_A) {
> > +	if (port == PORT_A) {
> >  		ironlake_set_pll_cpu_edp(intel_dp);
> >  		ironlake_edp_pll_on(intel_dp);
> >  	}
> > 
> 
> Wish we had a nice hook to hide the gen5 bits somewhere better, but it's
> fine as is with the comment.

I did consider adding ilk_pre_enable_dp and ilk_enable_dp for this, but I
decided that it would be more confusing since then both pre-ilk (g4x) and
post-ilk (snb+) would use the g4x functions.

I did manage to confuse myself already once with the g4x vs. pch vs.
platform specific vs. totally generic hooks we have for some port
types. So it might make sense to take a small code duplication hit,
and just make sure there is always a full set of hook with the same
prefix, even if some of those do exactly the same thing.

> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx