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

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, 25 May 2012 10:34:58 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> 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).
> 

Got it, Thanks.

> > + */
> > +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.

Got it, Thanks.

> 
> > +	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. :)
> 

I've come up with some similarly complex ideas in previous versions of
the patch, and Daniel seemed to be not so interested because of the
expectedly infrequent nature of the events. 

If Daniel is willing to accept the patch with your suggestion, I would
be happy to implement it. Of course the metric for "rapid succession"
would need to be agreed upon. I suggestion.

> > + +	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.

Good point. I had that info in an earlier version of the patch. Not sure
when it got dropped. Also noted the DRM_DEBUG suggestion, thanks.

> 
> > + +	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.

It is really just to prevent developers from enabling this interrupt on
GEN6, or VLV... It really should have been if (!IS_IVYBRIDGE). I'll drop
the WARN_ON and just return.

> 
> > + +	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>
> 
I'll wait for Daniel to address your request about auto-remapping before
I add your r-b.

Thanks.



-- 
Ben Widawsky, Intel Open Source Technology Center