[Intel-gfx] [PATCH v2] drm/i915: Tune down init error message due to failure injection
Joonas Lahtinen
joonas.lahtinen at linux.intel.com
Thu Mar 24 13:11:48 UTC 2016
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.
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);
+}
+
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)
/*@}*/
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.
More information about the Intel-gfx
mailing list