Re: [PATCH] drm/i915: Remove function details from device error messages

From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Remove function details from device error messages
Date: Mon, 9 Jul 2018 10:51:02 -0700	[thread overview]
Message-ID: <20180709175102.GD28253@intel.com> (raw)
In-Reply-To: <20180709134858.12446-1-chris@chris-wilson.co.uk>

On Mon, Jul 09, 2018 at 02:48:58PM +0100, Chris Wilson wrote:
> Error messages are intended to be addressed to the user; be clear,
> succinct, instructive and unambiguous. Adding the function name to
> that message does not add any information the user requires and in
> the process makes the message less clear.
> 
> E.g.
> 
> [  245.539711] i915 0000:00:02.0: [drm:i915_gem_init [i915]] Failed to initialize GPU, declaring it wedged!

Overall I like the idea...

The down side is that for us when debugging we would need to always trust grep like
searches and many debug messages are constructed out of variables what makes it a bit
hard to find sometimes. Ok, nothing that we couldn't figure out...

> 
> becomes
> 
> [  245.539711] i915 0000:00:02.0: Failed to initialize GPU, declaring it wedged!

What about adding an "ERROR:" ?

[  245.539711] i915 0000:00:02.0: ERROR: Failed to initialize GPU, declaring it wedged!

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2959c88a37a5..c2b9a4a0ee49 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -104,8 +104,13 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level,
>  	vaf.fmt = fmt;
>  	vaf.va = &args;
>  
> -	dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV",
> -		   __builtin_return_address(0), &vaf);
> +	if (is_error)
> +		dev_printk(level, kdev, "%pV", &vaf);
> +	else
> +		dev_printk(level, kdev, "[" DRM_NAME ":%ps] %pV",
> +			   __builtin_return_address(0), &vaf);
> +
> +	va_end(args);
>  
>  	if (is_error && !shown_bug_once) {
>  		/*
> @@ -117,8 +122,6 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level,
>  			dev_notice(kdev, "%s", FDO_BUG_MSG);
>  		shown_bug_once = true;
>  	}
> -
> -	va_end(args);
>  }
>  
>  /* Map PCH device id to PCH type, or PCH_NONE if unknown. */
> -- 
> 2.18.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx