[Intel-gfx] [PATCH 08/10] drm/i915: print PCH FIFO underrun interrupts
Paulo Zanoni
przanoni at gmail.com
Thu Feb 14 21:53:42 CET 2013
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.
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.
> -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
More information about the Intel-gfx
mailing list