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@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
