On Fri, 27 Apr 2012 17:40:18 -0700 Ben Widawsky <ben at bwidawsk.net> wrote: > + > +/** > + * ivybridge_parity_work - Workqueue called when a parity error interrupt > + * occurred. > + * > + * Doesn't actually do anything except notify userspace so that userspace may > + * disable things later on. kdoc style comment but missing the @work: parameter. Might be a good place to describe what userspace can do in response (i.e. duplicate some of your commit message here). > + */ > +static void ivybridge_parity_work(struct work_struct *work) > +{ > + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t, > + parity_error_work); > + Stray newline. > + u32 error_status, row, bank, subbank; > + char *parity_event[5]; > + uint32_t misccpctl; > + unsigned long flags; > + > + /* We must turn off DOP level clock gating to access the L3 registers. > + * In order to prevent a get/put style interface, acquire struct mutex > + * any time we access those registers. > + */ > + mutex_lock(&dev_priv->dev->struct_mutex); > + > + misccpctl = I915_READ(GEN7_MISCCPCTL); > + I915_WRITE(GEN7_MISCCPCTL, misccpctl & ~GEN7_DOP_CLOCK_GATE_ENABLE); DOP clock gating should be unconditionally disabled, you can move this to the clock gating routine. > + POSTING_READ(GEN7_MISCCPCTL); > + > + error_status = I915_READ(GEN7_L3CDERRST1); > + row = GEN7_PARITY_ERROR_ROW(error_status); > + bank = GEN7_PARITY_ERROR_BANK(error_status); > + subbank = GEN7_PARITY_ERROR_SUBBANK(error_status); > + > + I915_WRITE(GEN7_L3CDERRST1, GEN7_PARITY_ERROR_VALID | > + GEN7_L3CDERRST1_ENABLE); > + POSTING_READ(GEN7_L3CDERRST1); > + > + I915_WRITE(GEN7_MISCCPCTL, misccpctl); > + > + spin_lock_irqsave(&dev_priv->irq_lock, flags); > + dev_priv->gt_irq_mask &= ~GT_GEN7_L3_PARITY_ERROR_INTERRUPT; > + I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > + spin_unlock_irqrestore(&dev_priv->irq_lock, flags); Is there a debug register we can read to see if we missed any events at this point? A followon patch might be to see when the last event occured, and if we get two or more in rapid succession (like right when we re-enable interrupts) the kernel could automatically do the remapping. But that's totally optional; I doubt we'll see that in practice except on machines we've specially broken in the lab. :) > + > + mutex_unlock(&dev_priv->dev->struct_mutex); > + > + parity_event[0] = "L3_PARITY_ERROR=1"; > + parity_event[1] = kasprintf(GFP_KERNEL, "ROW=%d", row); > + parity_event[2] = kasprintf(GFP_KERNEL, "BANK=%d", bank); > + parity_event[3] = kasprintf(GFP_KERNEL, "SUBBANK=%d", subbank); > + parity_event[4] = NULL; > + > + kobject_uevent_env(&dev_priv->dev->primary->kdev.kobj, > + KOBJ_CHANGE, parity_event); You have a DRM_INFO when the interrupt comes in, may as well spit out the row/bank/subbank info here too. Though I'd make them both DEBUG. > + > + kfree(parity_event[3]); > + kfree(parity_event[2]); > + kfree(parity_event[1]); > +} Hm we really need a man page for our events and such, but that's a separate patch. > + > +void ivybridge_handle_parity_error(struct drm_device *dev) > +{ > + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > + unsigned long flags; > + > + if (WARN_ON(IS_GEN6(dev))) > + return; What's this for? The user won't be able to do anything at this point except get scared... I'd either just make this return if GEN6 or add a GEN7 check before we branch here in the first place from the interrupt handler. > + > + spin_lock_irqsave(&dev_priv->irq_lock, flags); > + dev_priv->gt_irq_mask |= GT_GEN7_L3_PARITY_ERROR_INTERRUPT; > + I915_WRITE(GTIMR, dev_priv->gt_irq_mask); > + spin_unlock_irqrestore(&dev_priv->irq_lock, flags); > + > + queue_work(dev_priv->wq, &dev_priv->parity_error_work); > + DRM_INFO("Parity error interrupt. Scheduling work\n"); > +} > + > static void snb_gt_irq_handler(struct drm_device *dev, > struct drm_i915_private *dev_priv, > u32 gt_iir) > @@ -449,6 +526,9 @@ static void snb_gt_irq_handler(struct drm_device *dev, > DRM_ERROR("GT error interrupt 0x%08x\n", gt_iir); > i915_handle_error(dev, false); > } > + > + if (gt_iir & GT_GEN7_L3_PARITY_ERROR_INTERRUPT) > + ivybridge_handle_parity_error(dev); > } > > static void gen6_queue_rps_work(struct drm_i915_private *dev_priv, > @@ -1978,6 +2058,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev) > if (IS_GEN6(dev) || IS_IVYBRIDGE(dev)) > INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work); > > + if (IS_IVYBRIDGE(dev)) > + INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work); Looks good otherwise, so with the above fixed up or denied: Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org> -- Jesse Barnes, Intel Open Source Technology Center