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
