[Intel-gfx] [PATCH v5] drm/i915: Tune down init error message due to failure injection

[Imre Deak]

On Fri, 2016-03-18 at 07:58 +0000, Chris Wilson wrote:
> On Fri, Mar 18, 2016 at 08:50:37AM +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 and change it
> > to
> > """
> > Device initialization failed (errno)
> > Please file a bug at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
> > against DRM/Intel providing the dmesg log by booting with drm.debug=0xf
> > """
> > and export a helper printing error messages using this same format.
> > A follow-up patch will convert all uses of DRM_ERROR reporting a user
> > facing problem to use this new helper instead.
> > 
> > v2:
> > - Include the problematic error message in the commit log, add a
> >   request to file an fdo bug to the message (Chris)
> > v3:
> > - Include the new error message too in the commit log, make the
> >   fdo link more precise and print part of the message with info log
> >   level (Chris)
> > v4: (Chris)
> > - Use dev_printk instead of DRM_ERROR/INFO and use NOTICE instead of
> >   INFO loglevel
> > - Export a helper for printing user facing error messages
> > v5:
> > - Keep the DRM_ERROR message prefix used by piglit-igt/CI to filter
> >   relevant dmesg lines
> > - Use dev_notice(), instead of dev_printk(KERN_NOTICE,...)
> > 
> > CC: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 48 ++++++++++++++++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/i915_drv.h |  7 ++++++
> >  2 files changed, 50 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 68592b0..096dc35 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -66,6 +66,44 @@ bool __i915_inject_load_failure(const char *func, int line)
> >  	return false;
> >  }
> >  
> > +#define FDO_BUG_URL "https://bugs.freedesktop.org/enter_bug.cgi?product=DRI"
> > +#define FDO_BUG_MSG "Please file a bug at " FDO_BUG_URL " against DRM/Intel " \
> > +		    "providing the dmesg log by booting with drm.debug=0xf"
> > +
> > +void
> > +__i915_printk(struct drm_i915_private *dev_priv, const char *level,
> > +	      const char *fmt, ...)
> > +{
> 
> static int shown_bug_once;
> 
> > +	struct device *dev = dev_priv->dev->dev;
> > +	bool is_error = level[1] <= KERN_ERR[1];
> > +	struct va_format vaf;
> > +	va_list args;
> > +
> > +	va_start(args, fmt);
> > +
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > +
> > +	dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
> > +		   __builtin_return_address(0), &vaf);
> 
> Also on my wishlist is to
> #undef DRM_NAME
> #define DRM_NAME "drm/i915"
> in i915_drv.h
> 
> Don't tell me that would break CI!

The current regex it uses on all messages with a loglevel <=
KERN_NOTICE:
(\[drm:|drm_|intel_|i915_)

Probably could be changed to 'i915 0000:00:02.0:' if we converted all
user facing DRM_ERRORs to use the new helper. Btw, according to your
plan the rest non-user facing DRM_ERRORs would be printed with debug
log level I assume, so isn't it a problem that they won't be treated as
test failures by CI?

> > +	if (is_error)
> 
> if (is_error && !shown_bug_once) {
> > +		dev_notice(dev, "%s", FDO_BUG_MSG);
> shown_bug_once = 1;
> }

Ok, will respin it.

--Imre

> 
> With just showing the bug once,
> Reviwed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris
>