Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

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

On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote:
> On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We get spurious PCH FIFO underruns if we enable the reporting too soon
> > after enabling the crtc. Move it to be the last step, after the encoder
> > enable. Additionally we need an extra vblank wait, otherwise we still
> > get the underruns. Presumably the pipe/fdi isn't yet fully up and running
> > otherwise.
> >
> > For symmetry, disable the PCH underrun reporting as the first thing,
> > just before encoder disable, when shutting down the crtc.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 99fb33f..d5cb899 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >  	intel_crtc->active = true;
> >  
> >  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> > -	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> >  
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		if (encoder->pre_enable)
> > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >  
> >  	if (HAS_PCH_CPT(dev))
> >  		cpt_verify_modeset(dev, intel_crtc->pipe);
> > +
> > +	if (intel_crtc->config->has_pch_encoder) {
> > +		/* Must wait for vblank to avoid spurious PCH FIFO underruns */
> > +		intel_wait_for_vblank(dev, pipe);
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true);
> 
> Nitpick, moving this within the if (has_pch_encoder) isn't documented in
> the commit message. Does that change have an impact?

I don't much of a real concern here. I think the following might
happen (all on the same pipe):

1. enable PCH port
2. disable PCH port
3. PCH FIFO underrun just after we've re-enabled the PCH
   underrun reporting
4. enable port A
5. PCH FIFO underrun reporting isn't enabled anymore for this pipe

But since it's driving a non-PCH port anyway, that doesn't seem like
a huge worry. But I suppose I could change it to always enable PCH
FIFO underrun reporting even for port A. It should do no harm at least.

> 
> BR,
> Jani.
> 
> > +	}
> >  }
> >  
> >  /* IPS only exists on ULT machines and is tied to pipe A. */
> > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >  	int pipe = intel_crtc->pipe;
> >  	u32 reg, temp;
> >  
> > +	if (intel_crtc->config->has_pch_encoder)
> > +		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> >  	for_each_encoder_on_crtc(dev, crtc, encoder)
> >  		encoder->disable(encoder);
> >  
> >  	drm_crtc_vblank_off(crtc);
> >  	assert_vblank_disabled(crtc);
> >  
> > -	if (intel_crtc->config->has_pch_encoder)
> > -		intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> > -
> >  	intel_disable_pipe(intel_crtc);
> >  
> >  	ironlake_pfit_disable(intel_crtc, false);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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