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
