[Intel-gfx] [PATCH 7/7] drm/i915: print Gen 7 error interrupts

[Ben Widawsky]

On Fri, 25 Jan 2013 18:57:42 -0200
Paulo Zanoni <przanoni at gmail.com> 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 most 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.
> 
> Also notice that on Haswell the only interrupt I ever get is for
> "unclaimed registers", so it looks like at least on Haswell we're in a
> good shape.
Please take some of my cynicism, you are way too optimistic :p

I have a lot of concerns with this patch touch FPGA_DEBUG and races.

Third, I think to be super paranoid you might want to disable the ERR_INT while
handling interrupts.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   28 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h |    2 +-
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 943db10..c2136cd 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -697,6 +697,17 @@ 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)
> +		DRM_DEBUG_KMS("Error interrupt: 0x%08x\n", err_int);
> +
> +	I915_WRITE(GEN7_ERR_INT, err_int);
> +}
> +
>  static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -707,6 +718,14 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
>  
>  	atomic_inc(&dev_priv->irq_received);
>  
> +	/* We get interrupts on unclaimed registers, so check this before we do
> +	 * any I915_{READ,WRITE}. */
> +	if (drm_debug && IS_HASWELL(dev) &&
> +	    (I915_READ_NOTRACE(FPGA_DBG) & FPGA_DBG_RM_NOCLAIM)) {
> +		DRM_ERROR("Unclaimed register before interrupt\n");
> +		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
> +	}
> +
>  	/* disable master interrupt before clearing iir  */
>  	de_ier = I915_READ(DEIER);
>  	I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);

I don't really understand what you're trying to do with this, and I
think this is quite racy since you're usually protecting FPGA_DBG with
struct_mutex, but not here. Since it's mostly for debug, maybe you can
convince me why it should be here, and I'll drop my complaint.

> @@ -720,6 +739,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);
>  
> @@ -2006,7 +2028,8 @@ 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;
> @@ -2014,6 +2037,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,
> @@ -2177,6 +2201,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 ee30fb9..86cace1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3379,7 +3379,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)



-- 
Ben Widawsky, Intel Open Source Technology Center