Re: [PATCH 3/3] drm/i915: Record error state capture reason

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Feb 12, 2014 at 08:13:50AM +0000, Chris Wilson wrote:
> On Tue, Feb 11, 2014 at 06:32:24PM -0800, Ben Widawsky wrote:
> > > @@ -3234,7 +3241,9 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
> > >  		 */
> > >  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > >  		if (iir & I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT)
> > > -			i915_handle_error(dev, false);
> > > +			i915_handle_error(dev, false,
> > > +					  "Command parser error, iir 0x%08x",
> > > +					  iir);
> > >  
> > >  		for_each_pipe(pipe) {
> > >  			int reg = PIPESTAT(pipe);
> > > @@ -3416,7 +3425,9 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
> > >  		 */
> > >  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > >  		if (iir & I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT)
> > > -			i915_handle_error(dev, false);
> > > +			i915_handle_error(dev, false,
> > > +					  "Command parser error, iir 0x%08x",
> > > +					  iir);
> > >  
> > >  		for_each_pipe(pipe) {
> > >  			int reg = PIPESTAT(pipe);
> > > @@ -3653,7 +3664,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
> > >  		 */
> > >  		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > >  		if (iir & I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT)
> > > -			i915_handle_error(dev, false);
> > > +			i915_handle_error(dev, false,
> > > +					  "Command parser error, iir 0x%08x",
> > > +					  iir);
> > >  
> > 
> > Since you're not directly using any of the DRM_ printers, we lose the
> > file/line. Would you care to add __FILE__/__LINE? I think it would be
> > helpful - not sure what others thing.
> 
> In a user-facing message, I don't think so. In the error state we have
> the GPU generation and reason, so we know which irq handler fired in
> that case. I don't see what extra information func:line gives us since
> the messenger (the irq handler) is pretty uninteresting wrt the hang.
> -Chris
> 

My gripe was with the same string for all errors - but I *just* noticed
that they are for different GENs, and not different interrupts. So
ignore me.

> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center