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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 30, 2015 at 02:08:51PM +0200, Ville Syrjälä wrote:
> 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.

Iirc we still fail to enable fifo underrun reporting with fastboot (should
fix this now since we update watermarks on takeover). That was the reason
to unconditionally enable fifo underruns even on the pch, to make it work
on platforms where the pch interrupt source is shared. See the pile of
hurt at the end of intel_sanitize_crtc.

I'd just keep it out of the if for now.
-Daniel

> 
> > 
> > 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx