[Intel-gfx] [PATCH 08/10] drm/i915: print PCH FIFO underrun interrupts
Daniel Vetter
daniel at ffwll.ch
Thu Feb 14 22:13:05 CET 2013
On Thu, Feb 14, 2013 at 06:53:42PM -0200, Paulo Zanoni wrote:
> Hi
>
> 2013/2/9 Daniel Vetter <daniel at ffwll.ch>:
> > On Fri, Feb 08, 2013 at 05:35:19PM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>
> >> Also add an "ignore" bit that avoids printing the message in two
> >> cases:
> >> - When the message is in fact expected.
> >> - After we get the first message. In tihs case, we expect to get
> >> hundreds of consecutive messages, so we just ignore all the
> >> subsequent messages until the next crtc_enable.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >
> > Ah, here's the "block underrun reporting stuff". You need to set up this
> > infrastructure first, then enable the error interrupts. Otherwise a poor
> > bloke will hit the irq strom in between these two patches in a bisect and
> > die from a heart attack when looking at dmesg.
>
> Nope. This patch is also the patch that adds the dmesg message, so
> without this patch there is no message to ignore. Without this patch
> we still get the interrupts, but this shouldn't be a problem because
> we don't print anything and because of patch 5.
>
> >
> > Also, you update the ignore_bla from the irq handler, so this needs proper
> > locking.
> Will fix.
>
> > And I vote for the bikeshed to be moved into struct intel_crtc,
> > e.g.
>
> It may be a little slightly slower, since we'll have to look for the
> crtc pointers on our structs. And on Haswell, checking which crtc is
> using the PCH transcoder will be more complicated. I'll try to think
> on a good solution.
Imo speed doesn't matter at all - we shouldn't ever get these in the
interrupt handler under normal conditions. And if we get too many of those
while modesetting, so that we suck away cpu time, it's probably best to
just disable all underrun reporting while doing a modeset.
> My initial idea was to actually group all the "interrupt-related" bits
> from dev_priv into an "interrupt struct", and put the ignore_fifo bits
> there.
>
> >
> > struct intel_crtc {
> > ...
> >
> > struct spinlock underrun_lock;
> > int underrun_reporting_enabled;
> > }
> >
> > Then smash a little helper into every crtc_enable/disable function. Aside:
> > you need to use the spin_lock_irqsafe/restore functions in the irq handler
> > (and for safety also in the helpers). Another curious question: Why do we
> > need separate ignore bits for the cpu and the pch?
>
> Because: (i) sometimes we get errors from one but not from the other;
> (ii) during "pipe/transcoder disable" we only want to ignore the PCH
> underruns, not the CPU underruns; (iii) on Haswell, there's no 1:1
> mapping between CPU pipe and PCH transcoder.
Ok, I see your aim with separate underrun reporting disables for pch/cpu.
But I also fear that we'll have tons of fun with just enabling the
underrun interrupts once the pipe is up, so a fine-grained report
disabling scheme sounds a bit like overkill. At least until we've fixed
the first set of big bugs.
The other thing is that I think we should just disable the interrupts when
we expect them (modeset) to avoid blowing through too much cpu time. So
having just one knob for everything sounds simpler.
Wrt Haswell pch interrupts: Either loop through the crtc list and check
->pch_encoder or check the DDI E port. The former shouldn't be a locking
issue as long as you grab the right irqsafe spinlocks first and check
whether reporting is enabled, since locks serve as barriers, too.
The reason for all this "can we keep this generic" bikeshed is that I'd
like to enable underrun reporting for i9xx/vlv, too. So having a bit of
abstraction would help.
-Daniel
>
> > -Daniel
> >
> >> ---
> >> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> >> drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++-----
> >> drivers/gpu/drm/i915/i915_reg.h | 7 +++++--
> >> drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++-
> >> 4 files changed, 53 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 08c5def..e96d75e 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -909,6 +909,9 @@ typedef struct drm_i915_private {
> >> struct work_struct hotplug_work;
> >> bool enable_hotplug_processing;
> >>
> >> + /* Bit 0: PCH transcoder A and so on. */
> >> + u8 ignore_pch_fifo_underrun;
> >> +
> >> int num_pipe;
> >> int num_pch_pll;
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 703a08a..7497589 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -659,10 +659,17 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
> >> if (pch_iir & (SDE_TRANSB_CRC_ERR | SDE_TRANSA_CRC_ERR))
> >> DRM_DEBUG_DRIVER("PCH transcoder CRC error interrupt\n");
> >>
> >> - if (pch_iir & SDE_TRANSB_FIFO_UNDER)
> >> - DRM_DEBUG_DRIVER("PCH transcoder B underrun interrupt\n");
> >> - if (pch_iir & SDE_TRANSA_FIFO_UNDER)
> >> - DRM_DEBUG_DRIVER("PCH transcoder A underrun interrupt\n");
> >> + if ((pch_iir & SDE_TRANSB_FIFO_UNDER) &&
> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_B))) {
> >> + DRM_DEBUG_DRIVER("PCH transcoder B underrun\n");
> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_B);
> >> + }
> >> +
> >> + if ((pch_iir & SDE_TRANSA_FIFO_UNDER) &&
> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_A))) {
> >> + DRM_DEBUG_DRIVER("PCH transcoder A underrun\n");
> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_A);
> >> + }
> >> }
> >>
> >> static void err_int_handler(struct drm_device *dev)
> >> @@ -684,6 +691,24 @@ static void serr_int_handler(struct drm_device *dev)
> >> if (serr_int & SERR_INT_POISON)
> >> DRM_ERROR("PCH poison interrupt\n");
> >>
> >> + if ((serr_int & SERR_INT_TRANS_A_FIFO_UNDERRUN) &&
> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_A))) {
> >> + DRM_ERROR("PCH transcoder A FIFO underrun\n");
> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_A);
> >> + }
> >> +
> >> + if ((serr_int & SERR_INT_TRANS_B_FIFO_UNDERRUN) &&
> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_B))) {
> >> + DRM_ERROR("PCH transcoder B FIFO underrun\n");
> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_B);
> >> + }
> >> +
> >> + if ((serr_int & SERR_INT_TRANS_C_FIFO_UNDERRUN) &&
> >> + !(dev_priv->ignore_pch_fifo_underrun & (1 << TRANSCODER_C))) {
> >> + DRM_ERROR("PCH transcoder C FIFO underrun\n");
> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_C);
> >> + }
> >> +
> >> I915_WRITE(SERR_INT, serr_int);
> >> }
> >>
> >> @@ -2000,7 +2025,9 @@ static void ibx_irq_postinstall(struct drm_device *dev)
> >> mask = SDE_HOTPLUG_MASK |
> >> SDE_GMBUS |
> >> SDE_AUX_MASK |
> >> - SDE_POISON;
> >> + SDE_POISON |
> >> + SDE_TRANSB_FIFO_UNDER |
> >> + SDE_TRANSA_FIFO_UNDER;
> >> } else {
> >> mask = SDE_HOTPLUG_MASK_CPT |
> >> SDE_GMBUS_CPT |
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index f22e27d..d565bd7 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -3554,8 +3554,11 @@
> >> #define SDEIIR 0xc4008
> >> #define SDEIER 0xc400c
> >>
> >> -#define SERR_INT 0xc4040
> >> -#define SERR_INT_POISON (1 << 31)
> >> +#define SERR_INT 0xc4040
> >> +#define SERR_INT_POISON (1 << 31)
> >> +#define SERR_INT_TRANS_C_FIFO_UNDERRUN (1 << 6)
> >> +#define SERR_INT_TRANS_B_FIFO_UNDERRUN (1 << 3)
> >> +#define SERR_INT_TRANS_A_FIFO_UNDERRUN (1 << 0)
> >>
> >> /* digital port hotplug */
> >> #define PCH_PORT_HOTPLUG 0xc4030 /* SHOTPLUG_CTL */
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index d75c6a0..67bfb58 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3274,6 +3274,9 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
> >> return;
> >>
> >> intel_crtc->active = true;
> >> +
> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << pipe);
> >> +
> >> intel_update_watermarks(dev);
> >>
> >> if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> >> @@ -3366,11 +3369,15 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
> >> return;
> >>
> >> intel_crtc->active = true;
> >> - intel_update_watermarks(dev);
> >>
> >> is_pch_port = haswell_crtc_driving_pch(crtc);
> >>
> >> if (is_pch_port)
> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << TRANSCODER_A);
> >> +
> >> + intel_update_watermarks(dev);
> >> +
> >> + if (is_pch_port)
> >> dev_priv->display.fdi_link_train(crtc);
> >>
> >> for_each_encoder_on_crtc(dev, crtc, encoder)
> >> @@ -3453,6 +3460,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >> if (dev_priv->cfb_plane == plane)
> >> intel_disable_fbc(dev);
> >>
> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << pipe);
> >> intel_disable_pipe(dev_priv, pipe);
> >>
> >> /* Disable PF */
> >> @@ -3466,6 +3474,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
> >> ironlake_fdi_disable(crtc);
> >>
> >> ironlake_disable_pch_transcoder(dev_priv, pipe);
> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << pipe);
> >>
> >> if (HAS_PCH_CPT(dev)) {
> >> /* disable TRANS_DP_CTL */
> >> @@ -3535,6 +3544,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >> if (dev_priv->cfb_plane == plane)
> >> intel_disable_fbc(dev);
> >>
> >> + if (is_pch_port)
> >> + dev_priv->ignore_pch_fifo_underrun |= (1 << TRANSCODER_A);
> >> intel_disable_pipe(dev_priv, pipe);
> >>
> >> intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
> >> @@ -3551,6 +3562,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >>
> >> if (is_pch_port) {
> >> lpt_disable_pch_transcoder(dev_priv);
> >> + dev_priv->ignore_pch_fifo_underrun &= ~(1 << TRANSCODER_A);
> >> intel_ddi_fdi_disable(crtc);
> >> }
> >>
> >> --
> >> 1.7.10.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list