On Fri, 18 Jan 2013, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Fri, Jan 18, 2013 at 06:29:11PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> If we get one of these messages it means we did something wrong, and
>> the first step to fix wrong things is to detect them and recognize
>> they exist.
>>
>> For now, leave these messages as DRM_DEBUG_KMS. After we conclude that
>> a certain message should not be print since our code is correct, then
>> we can promote that specific message to DRM_ERROR.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> A couple of recommendations below, but either way it is
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
>
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 56 ++++++++++++++++++++++++++++++++++++++-
>> drivers/gpu/drm/i915/i915_reg.h | 23 ++++++++++++++--
>> 2 files changed, 76 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index cc49a6d..2f17b54 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>> I915_READ(FDI_RX_IIR(pipe)));
>> }
>>
>> +static void ivb_err_int_handler(struct drm_device *dev)
>> +{
>> + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
>> + u32 err_int = I915_READ(GEN7_ERR_INT);
>> +
>> + if (err_int & ERR_INT_POISON)
>> + DRM_DEBUG_KMS("Poison interrupt\n");
>> + if (err_int & ERR_INT_INVALID_GTT_PTE)
>> + DRM_DEBUG_KMS("Invalid GTT PTE\n");
>> + if (err_int & ERR_INT_INVALID_PTE_DATA)
>> + DRM_DEBUG_KMS("Invalid GTT PTE data\n");
>> + if (err_int & ERR_INT_SPRITE_GTT_FAULT_C)
>> + DRM_DEBUG_KMS("Sprite C GTT fault\n");
>> + if (err_int & ERR_INT_PRIMARY_GTT_FAULT_C)
>> + DRM_DEBUG_KMS("Primary place C GTT fault\n");
>> + if (err_int & ERR_INT_CURSOR_GTT_FAULT_C)
>> + DRM_DEBUG_KMS("Cursor C GTT fault\n");
>> + if (err_int & ERR_INT_SPRITE_GTT_FAULT_B)
>> + DRM_DEBUG_KMS("Sprite B GTT fault\n");
>> + if (err_int & ERR_INT_PRIMARY_GTT_FAULT_B)
>> + DRM_DEBUG_KMS("Primary place B GTT fault\n");
>> + if (err_int & ERR_INT_CURSOR_GTT_FAULT_B)
>> + DRM_DEBUG_KMS("Cursor B GTT fault\n");
>> + if (err_int & ERR_INT_SPRITE_GTT_FAULT_A)
>> + DRM_DEBUG_KMS("Sprite A GTT fault\n");
>> + if (err_int & ERR_INT_PRIMARY_GTT_FAULT_A)
>> + DRM_DEBUG_KMS("Primary place A GTT fault\n");
>> + if (err_int & ERR_INT_CURSOR_GTT_FAULT_A)
>> + DRM_DEBUG_KMS("Cursor A GTT fault\n");
>> + if (err_int & ERR_INT_PIPE_CRR_ERROR_C)
>> + DRM_DEBUG_KMS("Pipe C CRC error\n");
>> + if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_C)
>> + DRM_DEBUG_KMS("Pipe C FIFO underrun\n");
>> + if (err_int & ERR_INT_PIPE_CRR_ERROR_B)
>> + DRM_DEBUG_KMS("Pipe B CRC error\n");
>> + if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_B)
>> + DRM_DEBUG_KMS("Pipe B FIFO underrun\n");
>> + if (err_int & ERR_INT_PIPE_CRR_ERROR_A)
>> + DRM_DEBUG_KMS("Pipe A CRC error\n");
>> + if (err_int & ERR_INT_PIPE_FIFO_UNDERRRUN_A)
>> + DRM_DEBUG_KMS("Pipe A FIFO underrun\n");
>> +
>> + if (IS_HASWELL(dev) && (err_int & ERR_INT_MMIO_UNCLAIMED))
>> + DRM_DEBUG_KMS("MMIO cycle not claimed\n");
>> +
>> + I915_WRITE(GEN7_ERR_INT, err_int);
>> +}
>> +
>
> We probably don't need the IS_HASWELL() check, but it shouldn't hurt
> either.
>
> I shutter to think of debugging what will happen if this handler itself
> generates ERR_INT_MMIO_UNCLAIMED. Hopefully the DRM_DEBUG_KMS will get a
> chance to get spewed out.
>
> Since the only thing you're doing is printing, you could do something
> like:
> u32 err_int = I915_READ(GEN7_ERR_INT);
> if (drm_debug & DRM_UT_KMS == 0) {
Review-of-review, that needs braces or it's always false.
BR,
Jani.
> I915_WRITE(GEN7_ERR_INT, err_int);
> return;
> }
>
>> static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>> {
>> struct drm_device *dev = (struct drm_device *) arg;
>> @@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>>
>> de_iir = I915_READ(DEIIR);
>> if (de_iir) {
>> + if (de_iir & DE_ERR_INT_IVB)
>> + ivb_err_int_handler(dev);
>> +
>> if (de_iir & DE_AUX_CHANNEL_A_IVB)
>> dp_aux_irq_handler(dev);
>>
>> @@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>> DE_PLANEC_FLIP_DONE_IVB |
>> DE_PLANEB_FLIP_DONE_IVB |
>> DE_PLANEA_FLIP_DONE_IVB |
>> - DE_AUX_CHANNEL_A_IVB;
>> + DE_AUX_CHANNEL_A_IVB | DE_ERR_INT_IVB;
>> u32 render_irqs;
>> u32 hotplug_mask;
>> u32 pch_irq_mask;
>> @@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
>> dev_priv->irq_mask = ~display_mask;
>>
>> /* should always can generate irq */
>> + I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>> I915_WRITE(DEIIR, I915_READ(DEIIR));
>> I915_WRITE(DEIMR, dev_priv->irq_mask);
>> I915_WRITE(DEIER,
>> @@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
>> I915_WRITE(DEIMR, 0xffffffff);
>> I915_WRITE(DEIER, 0x0);
>> I915_WRITE(DEIIR, I915_READ(DEIIR));
>> + if (IS_GEN7(dev))
>> + I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT));
>>
>> I915_WRITE(GTIMR, 0xffffffff);
>> I915_WRITE(GTIER, 0x0);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index f054554..e8ecdc4 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -511,7 +511,26 @@
>>
>> #define ERROR_GEN6 0x040a0
>> #define GEN7_ERR_INT 0x44040
>> -#define ERR_INT_MMIO_UNCLAIMED (1<<13)
>> +#define ERR_INT_POISON (1<<31)
>> +#define ERR_INT_INVALID_GTT_PTE (1<<29)
>> +#define ERR_INT_INVALID_PTE_DATA (1<<28)
>> +#define ERR_INT_SPRITE_GTT_FAULT_C (1<<23)
>> +#define ERR_INT_PRIMARY_GTT_FAULT_C (1<<22)
>> +#define ERR_INT_CURSOR_GTT_FAULT_C (1<<21)
>> +#define ERR_INT_SPRITE_GTT_FAULT_B (1<<20)
>> +#define ERR_INT_PRIMARY_GTT_FAULT_B (1<<19)
>> +#define ERR_INT_CURSOR_GTT_FAULT_B (1<<18)
>> +#define ERR_INT_SPRITE_GTT_FAULT_A (1<<17)
>> +#define ERR_INT_PRIMARY_GTT_FAULT_A (1<<16)
>> +#define ERR_INT_CURSOR_GTT_FAULT_A (1<<15)
>> +#define ERR_INT_PIPE_CRR_ERROR_C (1<<7)
>> +#define ERR_INT_PIPE_FIFO_UNDERRRUN_C (1<<6)
>> +#define ERR_INT_PIPE_CRR_ERROR_B (1<<4)
>> +#define ERR_INT_PIPE_FIFO_UNDERRRUN_B (1<<3)
>> +#define ERR_INT_PIPE_CRR_ERROR_A (1<<1)
>> +#define ERR_INT_PIPE_FIFO_UNDERRRUN_A (1<<0)
>> +/* Haswell only: */
>> +#define ERR_INT_MMIO_UNCLAIMED (1<<13)
>>
>> /* GM45+ chicken bits -- debug workaround bits that may be required
>> * for various sorts of correct behavior. The top 16 bits of each are
>> @@ -3374,7 +3393,7 @@
>> #define DE_PIPEA_FIFO_UNDERRUN (1 << 0)
>>
>> /* More Ivybridge lolz */
>> -#define DE_ERR_DEBUG_IVB (1<<30)
>> +#define DE_ERR_INT_IVB (1<<30)
>> #define DE_GSE_IVB (1<<29)
>> #define DE_PCH_EVENT_IVB (1<<28)
>> #define DE_DP_A_HOTPLUG_IVB (1<<27)
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
