[Intel-gfx] [PATCH v2 2/2] drm/i915: Ignore transcoder B/C on LPT/WPT FIFO underrun handling

[Daniel Vetter]

On Mon, Nov 30, 2015 at 04:08:34PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 30, 2015 at 09:25:03AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 26, 2015 at 10:55:53PM +0200, ville.syrjala at linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > LPT/WPT only have transcoder A, so we shouldn't look at FIFO underruns
> > > for transocoder B/C. And more importnatnly we should not consider
> > > the state of underrun reporting for transcoders B/C when checking
> > > whether we can enable the south error interrupt.
> > > 
> > > The whole thing is a bit of mess since we store the underrun reporting
> > > state for transcoder A under intel_crtc for pipe A, irrespective of
> > > which pipe may actually be driving the transcoder. But I figured trying
> > > to change that would result in more churn.
> > > 
> > > Caveat: Still untested
> > > 
> > > v2: Use HAS_PCH_LPT_H instead of HAS_DDI
> > >     Use cpt_check_pch_fifo_underruns() on LPT-H/WPT-H too
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > I've battled hsw lpt fifo underrun issues last week and never seen a fifo
> > underrun on pipe B/C. Have you seen them anywhere really?
> 
> No, I've not seen it. We mark underrun detection as disabled for active
> pipes in intel_sanitize_crtc(), and we never turn it back on except for
> transcoder A, so I'm thinking we'll never actually enable the south
> error interrup on LPT if a non-pch port is enabled on boot.

Hm right this is broken for pch ... I guess we need an IS_HSW(dev) : PCH_A
: pipe in there to get this right.

I do get plenty pch fifo underruns here, but I guess my bios boots with
vga on pipe A and hence I get away with this bug ;-)
 
> Note that so far I didn't check if that's actually the case, but based
> on the code I can't come to any other conclusion.

Yeah seems indeed broken, but I think simpler to fix in the crtc sanitize
code.
-Daniel

> 
> > 
> > If not I think we can skip this patch here, since I think I tracked it all
> > down.
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_fifo_underrun.c | 27 ++++++++++++++++++++++-----
> > >  1 file changed, 22 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_fifo_underrun.c b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > index bda526660e20..3d3acc8b8367 100644
> > > --- a/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > +++ b/drivers/gpu/drm/i915/intel_fifo_underrun.c
> > > @@ -48,6 +48,14 @@
> > >   * The code also supports underrun detection on the PCH transcoder.
> > >   */
> > >  
> > > +static bool cpt_transcoder_exists(struct drm_i915_private *dev_priv,
> > > +				  enum transcoder pch_transcoder)
> > > +{
> > > +	/* LPT-H/WPT-H have only transcoder A */
> > > +	return HAS_PCH_CPT(dev_priv) ||
> > > +		(HAS_PCH_LPT_H(dev_priv) && pch_transcoder == TRANSCODER_A);
> > > +}
> > > +
> > >  static bool ivb_can_enable_err_int(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > @@ -69,13 +77,16 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
> > >  static bool cpt_can_enable_serr_int(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > -	enum pipe pipe;
> > > -	struct intel_crtc *crtc;
> > > +	enum transcoder pch_transcoder;
> > >  
> > >  	assert_spin_locked(&dev_priv->irq_lock);
> > >  
> > > -	for_each_pipe(dev_priv, pipe) {
> > > -		crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> > > +	for_each_pipe(dev_priv, pch_transcoder) {
> > > +		struct intel_crtc *crtc =
> > > +			to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pch_transcoder]);
> > > +
> > > +		if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > > +			continue;
> > >  
> > >  		if (crtc->pch_fifo_underrun_disabled)
> > >  			return false;
> > > @@ -206,6 +217,9 @@ static void cpt_check_pch_fifo_underruns(struct intel_crtc *crtc)
> > >  
> > >  	assert_spin_locked(&dev_priv->irq_lock);
> > >  
> > > +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > > +		return;
> > > +
> > >  	if ((serr_int & SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder)) == 0)
> > >  		return;
> > >  
> > > @@ -222,6 +236,9 @@ static void cpt_set_fifo_underrun_reporting(struct drm_device *dev,
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > > +	if (!cpt_transcoder_exists(dev_priv, pch_transcoder))
> > > +		return;
> > > +
> > >  	if (enable) {
> > >  		I915_WRITE(SERR_INT,
> > >  			   SERR_INT_TRANS_FIFO_UNDERRUN(pch_transcoder));
> > > @@ -436,7 +453,7 @@ void intel_check_pch_fifo_underruns(struct drm_i915_private *dev_priv)
> > >  		if (crtc->pch_fifo_underrun_disabled)
> > >  			continue;
> > >  
> > > -		if (HAS_PCH_CPT(dev_priv))
> > > +		if (HAS_PCH_CPT(dev_priv) || HAS_PCH_LPT_H(dev_priv))
> > >  			cpt_check_pch_fifo_underruns(crtc);
> > >  	}
> > >  
> > > -- 
> > > 2.4.10
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch