Re: [PATCH 2/5] drm/i915: Dynamic Parity Detection handling

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 27 Apr 2012 17:40:18 -0700
Ben Widawsky <ben@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@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center