Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection

From: Imre Deak <imre.deak@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/i915: Tune down init error message due to failure injection
Date: Thu, 17 Mar 2016 18:08:05 +0200	[thread overview]
Message-ID: <1458230885.13116.5.camel@intel.com> (raw)
In-Reply-To: <20160317155539.GB14143@nuc-i3427.alporthouse.com>

On Thu, 2016-03-17 at 15:55 +0000, Chris Wilson wrote:
> On Thu, Mar 17, 2016 at 05:46:11PM +0200, Imre Deak wrote:
> > Atm, in case failure injection forces an error the subsequent
> > "*ERROR* failed to init modeset" error message will make automated
> > tests (CI) report this event as a breakage even though the event is
> > expected. To fix this print the error message with debug log level
> > in this case.
> > 
> > While at it print the error message for any init failure for
> > consistency.
> > 
> > v2:
> > - Include the problematic error message in the commit log, add a
> >   request to file an fdo bug to the message (Chris)
> 
> And the new one! Helps when cross-referencing a message in dmesg.
> > 
> > CC: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 68592b0..b4ece9e 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -66,6 +66,14 @@ bool __i915_inject_load_failure(const char
> > *func, int line)
> >  	return false;
> >  }
> >  
> > +#define i915_load_error(fmt, ...) do {				
> > 	\
> > +	if (i915.inject_load_failure &&				
> > 	\
> > +	    i915_load_fail_count == i915.inject_load_failure)	
> > 	\
> > +		DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__);		
> > 	\
> > +	else							
> > 	\
> > +		DRM_ERROR(fmt, ##__VA_ARGS__);			
> > 	\
> > +} while(0)
> > +
> >  static int i915_getparam(struct drm_device *dev, void *data,
> >  			 struct drm_file *file_priv)
> >  {
> > @@ -1332,10 +1340,8 @@ int i915_driver_load(struct drm_device *dev,
> > unsigned long flags)
> >  	}
> >  
> >  	ret = i915_load_modeset_init(dev);
> > -	if (ret < 0) {
> > -		DRM_ERROR("failed to init modeset\n");
> > +	if (ret < 0)
> >  		goto out_cleanup_vblank;
> > -	}
> >  
> >  	i915_driver_register(dev_priv);
> >  
> > @@ -1357,6 +1363,8 @@ out_runtime_pm_put:
> >  out_free_priv:
> >  	kfree(dev_priv);
> >  
> > +	i915_load_error("Device initialization failed (%d), please
> > file a bug at https://bugs.freedesktop.org providing the dmesg log
> > by booting with drm.debug=0xf\n", ret);
> 
> 80cols rules still apply to messages :)
> 
> "Device initialization failed (%d). "
> "Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi "
> "against DRI/DRM/Intel providing the dmesg log by booting "
> "with drm.debug=0xf\n",

I thought for strings emitted to dmesg it's discouraged, since you
can't easily grep then. But I can make the link more precise.

> I would personally make i915_load_error() a proper function and add
> the "Please..." output there at a lower logging level than ERROR.

Using a function is not straightforward since there is no vprintf like
interface for DRM_ERROR. But I can move that part of the message to
macro.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx