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.
Also, you update the ignore_bla from the irq handler, so this needs proper
locking. And I vote for the bikeshed to be moved into struct intel_crtc,
e.g.
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?
-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
