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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 24, 2016 at 03:11:48PM +0200, Joonas Lahtinen wrote:
> On ma, 2016-03-21 at 17:12 +0200, Imre Deak wrote:
> > On ma, 2016-03-21 at 10:28 +0100, Daniel Vetter wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 11:15:35AM +0200, Imre Deak wrote:
> > > > 
> > > > On Fri, 2016-03-18 at 10:59 +0200, Joonas Lahtinen wrote:
> > > > > 
> > > > > On pe, 2016-03-18 at 00:18 +0200, Imre Deak wrote:
> > > > > > 
> > > > > > On Thu, 2016-03-17 at 22:14 +0000, Chris Wilson wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Fri, Mar 18, 2016 at 12:09:30AM +0200, Imre Deak wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Thu, 2016-03-17 at 21:48 +0000, Chris Wilson wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I would also like this to be the preferred
> > > > > > > > > DRM_ERROR reporting mechanism i.e. anytime we emit an
> > > > > > > > > ERROR
> > > > > > > > > we
> > > > > > > > > should
> > > > > > > > > be
> > > > > > > > > encouraging the user to file a bug, and do so in a user
> > > > > > > > > friendly
> > > > > > > > > fashion.
> > > > > > > > Ok, but only in i915 I assume. Should we also convert then
> > > > > > > > all
> > > > > > > > DRM_ERROR to dev_err - with an *ERROR* prefix - or still
> > > > > > > > use
> > > > > > > > DRM_ERROR?
> > > > > > > Within i915. I am thinking along the lines that no DRM_ERROR
> > > > > > > should
> > > > > > > exist that doesn't acknowlege that it is a user facing error
> > > > > > > message
> > > > > > > (i.e. written in plain English and is informative, and
> > > > > > > includes a
> > > > > > > bug
> > > > > > > reporting reference). So i915_report_error() or somesuch.
> > > > > > Ok, will give it a go.
> > > > > Daniel didn't want i915 debugging/erroring mechanisms to deviate
> > > > > from
> > > > > core DRM. So I guess this would follow in the same category.
> > > > > 
> > > > > I'm all in for structuring a coherent debugging/error message
> > > > > logic
> > > > > and
> > > > > functions for it and then everyone can follow the suit.
> > > > The dev_err/dbg method has obvious advantages, like dynamic debug
> > > > or
> > > > showing the device instance, so I think that's something we want in
> > > > any
> > > > case. I don't see a problem with first rolling it out in i915 then
> > > > proposing it for more generic use.
> > > Well that's just silly me trying to apply some pressure into making
> > > something that's compatible with DRM_*. Or well, porting those macros
> > > over
> > > to whatever the newfangled fancy thing is. Including keeping
> > > drm.debug
> > > alive with some hacks (we can write our own store functions, which
> > > could
> > > enable the right stuff with dynamic debugging, if we export a few
> > > funcs)
> > > so that the gazillion of howtos out there don't all break.
> > Yea, currently we'd output debug messages even in case of drm.debug==0.
> > I sent a patch that makes __i915_printk() behave the same as
> > DRM_DEBUG_DRIVER for debug messages. I think on top of that a more
> > fancy dynamic debug based filtering can be implemented later as you
> > suggest.
> > 
> 
> Yeah, when dynamic debugging is disabled drm.debug==0 would produce all
> the debug, that's the biggest problem I hit. Over-verbosity.
> 
> I have the drm.debug working in dynamic debugging cases already (I
> exposed one interface from kernel and tadah).
> 
> Problem is only, if we want to have drm.debug doing its filtering thing
> for the old and new code in transitino phase when dynamic debugging is
> disabled. Then we'll have to go little bit further and do a few #undefs
> (but we currently have those, too), because dynamic debugging also
> makes itself add the line numbers and file names to allow filtering by
> those.
> 
> I copy-pasted my changes at the end so you get a rough idea.

Just very quick pass over it, 2 comments below.
-Danil

> 
> Regards, Joonas
> 
> > --Imre
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> 
> ------------------------------8<-----------------------------------
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..da818a0 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -32,6 +32,7 @@
>  #include <linux/moduleparam.h>
>  #include <linux/mount.h>
>  #include <linux/slab.h>
> +#include <linux/dynamic_debug.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_core.h>
>  #include "drm_legacy.h"
> @@ -43,8 +44,11 @@ EXPORT_SYMBOL(drm_debug);
>  MODULE_AUTHOR(CORE_AUTHOR);
>  MODULE_DESCRIPTION(CORE_DESC);
>  MODULE_LICENSE("GPL and additional rights");
> +
> +#ifdef CONFIG_DYNAMIC_DEBUG
>  MODULE_PARM_DESC(debug, "Enable debug output");
>  module_param_named(debug, drm_debug, int, 0600);
> +#endif
>  
>  static DEFINE_SPINLOCK(drm_minor_lock);
>  static struct idr drm_minors_idr;
> @@ -61,28 +65,13 @@ void drm_err(const char *format, ...)
>  	vaf.fmt = format;
>  	vaf.va = &args;
>  
> -	printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
> +	printk(KERN_ERR DRM_NAME ": [%ps] *ERROR* %pV",
>  	       __builtin_return_address(0), &vaf);
>  
>  	va_end(args);
>  }
>  EXPORT_SYMBOL(drm_err);
>  
> -void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> -{
> -	struct va_format vaf;
> -	va_list args;
> -
> -	va_start(args, format);
> -	vaf.fmt = format;
> -	vaf.va = &args;
> -
> -	printk(KERN_DEBUG "[" DRM_NAME ":%s] %pV", function_name, &vaf);
> -
> -	va_end(args);
> -}
> -EXPORT_SYMBOL(drm_ut_debug_printk);
> -
>  struct drm_master *drm_master_create(struct drm_minor *minor)
>  {
>  	struct drm_master *master;
> @@ -879,10 +868,21 @@ static const struct file_operations drm_stub_fops = {
>  	.llseek = noop_llseek,
>  };
>  
> +static void drm_debug_init(void)
> +{
> +	struct ddebug_query query = { };
> +	unsigned int flags = _DPRINTK_FLAGS_PRINT | _DPRINTK_FLAGS_INCL_FUNCNAME;
> +	unsigned int mask = ~_DPRINTK_FLAGS_PRINT;
> +
> +	query.format = DRM_NAME ": " DRM_PREFIX_CORE ": ";
> +	ddebug_change(&query, drm_debug & DRM_UT_CORE ? flags : 0, mask);
> +}

Shouldn't we put this into the store function for the module paramater, so
that changing at runtime still works? Personally I adjust drm.debug a lot
in a bunch of scripts I have lying around, I guess others do the same.

> +
>  static int __init drm_core_init(void)
>  {
>  	int ret = -ENOMEM;
>  
> +	drm_debug_init();
>  	drm_global_init();
>  	drm_connector_ida_init();
>  	idr_init(&drm_minors_idr);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c..628f27e 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -130,9 +130,13 @@ struct dma_buf_attachment;
>  #define DRM_UT_ATOMIC		0x10
>  #define DRM_UT_VBL		0x20
>  
> -extern __printf(2, 3)
> -void drm_ut_debug_printk(const char *function_name,
> -			 const char *format, ...);
> +#define DRM_PREFIX_CORE		"core"
> +#define DRM_PREFIX_DRIVER	"driver"
> +#define DRM_PREFIX_KMS		"kms"
> +#define DRM_PREFIX_PRIME	"prime"
> +#define DRM_PREFIX_ATOMIC	"atomic"
> +#define DRM_PREFIX_VBL		"vblank"
> +
>  extern __printf(1, 2)
>  void drm_err(const char *format, ...);
>  
> @@ -184,48 +188,34 @@ void drm_err(const char *format, ...);
>  })
>  
>  #define DRM_INFO(fmt, ...)				\
> -	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
> +	printk(KERN_INFO DRM_NAME ": " fmt, ##__VA_ARGS__)
>  
>  #define DRM_INFO_ONCE(fmt, ...)				\
> -	printk_once(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
> +	printk_once(KERN_INFO DRM_NAME ": " fmt, ##__VA_ARGS__)
>  
>  /**
>   * Debug output.
>   *
>   * \param fmt printf() like format string.
> - * \param arg arguments
> + * \param args arguments
>   */
> -#define DRM_DEBUG(fmt, args...)						\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_CORE))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -
> -#define DRM_DEBUG_DRIVER(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_DRIVER))		\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_KMS(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_KMS))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_PRIME(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_PRIME))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_ATOMIC(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_ATOMIC))		\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> -#define DRM_DEBUG_VBL(fmt, args...)					\
> -	do {								\
> -		if (unlikely(drm_debug & DRM_UT_VBL))			\
> -			drm_ut_debug_printk(__func__, fmt, ##args);	\
> -	} while (0)
> +#define DRM_DEBUG(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_CORE ": " fmt, ##args)
> +
> +#define DRM_DEBUG_DRIVER(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_DRIVER ": " fmt, ##args)
> +
> +#define DRM_DEBUG_KMS(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_KMS ": " fmt, ##args)
> +
> +#define DRM_DEBUG_PRIME(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_PRIME ": " fmt, ##args)
> +
> +#define DRM_DEBUG_ATOMIC(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_ATOMIC ": " fmt, ##args)
> +
> +#define DRM_DEBUG_VBL(fmt, args...) \
> +	pr_debug(DRM_NAME ": " DRM_PREFIX_VBL ": " fmt, ##args)

Yeah, I think we need to have an

#ifndef CONFIG_DYNAMIC_DEBUG
/* old debug macros using drm.debug filtering */
#else
/* new ddebug based macros */
#endif

to address the issues you've raised.

>  
>  /*@}*/
>  
> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
> index 4f1bbc6..beae2df 100644
> --- a/include/linux/dynamic_debug.h
> +++ b/include/linux/dynamic_debug.h
> @@ -1,6 +1,14 @@
>  #ifndef _DYNAMIC_DEBUG_H
>  #define _DYNAMIC_DEBUG_H
>  
> +struct ddebug_query {
> +	const char *filename;
> +	const char *module;
> +	const char *function;
> +	const char *format;
> +	unsigned int first_lineno, last_lineno;
> +};
> +
>  /*
>   * An instance of this structure is created in a special
>   * ELF section at every dynamic debug callsite.  At runtime,
> @@ -40,6 +48,8 @@ int ddebug_add_module(struct _ddebug *tab, unsigned int n,
>  				const char *modname);
>  
>  #if defined(CONFIG_DYNAMIC_DEBUG)
> +extern int ddebug_change(const struct ddebug_query *query,
> +			 unsigned int flags, unsigned int mask);
>  extern int ddebug_remove_module(const char *mod_name);
>  extern __printf(2, 3)
>  void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
> @@ -111,6 +121,12 @@ do {								\
>  #include <linux/string.h>
>  #include <linux/errno.h>
>  
> +static inline int ddebug_change(const struct ddebug_query *query,
> +				unsigned int flags, unsigned int mask)
> +{
> +	return 0;
> +}
> +
>  static inline int ddebug_remove_module(const char *mod)
>  {
>  	return 0;
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index fe42b6e..cbe7b07 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -47,14 +47,6 @@ struct ddebug_table {
>  	struct _ddebug *ddebugs;
>  };
>  
> -struct ddebug_query {
> -	const char *filename;
> -	const char *module;
> -	const char *function;
> -	const char *format;
> -	unsigned int first_lineno, last_lineno;
> -};
> -
>  struct ddebug_iter {
>  	struct ddebug_table *table;
>  	unsigned int idx;
> @@ -135,8 +127,8 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
>   * callsites, normally the same as number of changes.  If verbose,
>   * logs the changes.  Takes ddebug_lock.
>   */
> -static int ddebug_change(const struct ddebug_query *query,
> -			unsigned int flags, unsigned int mask)
> +int ddebug_change(const struct ddebug_query *query,
> +		  unsigned int flags, unsigned int mask)
>  {
>  	int i;
>  	struct ddebug_table *dt;
> @@ -203,6 +195,7 @@ static int ddebug_change(const struct ddebug_query *query,
>  
>  	return nfound;
>  }
> +EXPORT_SYMBOL(ddebug_change);
>  
>  /*
>   * Split the buffer `buf' into space-separated words.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux