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

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

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@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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@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.

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.

> 
> 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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx