From: Eric Engestrom <eric.engestrom@imgtec.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: linux-rockchip@lists.infradead.org,
dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v4] drm: Introduce DRM_DEV_* log messages
Date: Tue, 16 Aug 2016 17:56:21 +0100 [thread overview]
Message-ID: <20160816165621.GS10429@imgtec.com> (raw)
In-Reply-To: <CAOw6vbKidR8bp5_U5o8xhvL08WMHvB+=MYug3Ehj75=vj5H=Qw@mail.gmail.com>
On Tue, Aug 16, 2016 at 09:18:34AM -0700, Sean Paul wrote:
> On Tue, Aug 16, 2016 at 5:28 AM, Eric Engestrom
> <eric.engestrom@imgtec.com> wrote:
> > On Mon, Aug 15, 2016 at 04:18:04PM -0700, Sean Paul wrote:
> >> This patch consolidates all the various log functions/macros into
> >> one uber function, drm_log. It also introduces some new DRM_DEV_*
> >> variants that print the device name to delineate multiple devices
> >> of the same type.
> >>
> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> >> ---
> >>
> >> Changes in v2:
> >> - Use dev_printk for the dev variant (Chris Wilson)
> >>
> >> Changes in v3:
> >> - Rename drm_log to drm_dev_printk (Chris Wilson)
> >> - Break out drm_printk from drm_dev_printk to reduce
> >> image growth due to passing NULL around (Chris Wilson)
> >>
> >> Changes in v4:
> >> - Pull format string out into #define (Eric Engestrom)
> >>
> >>
> >> drivers/gpu/drm/drm_drv.c | 27 ++++++---
> >> include/drm/drmP.h | 140 +++++++++++++++++++++++++++-------------------
> >> 2 files changed, 103 insertions(+), 64 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 57ce973..a7f6282 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -63,37 +63,48 @@ static struct idr drm_minors_idr;
> >>
> >> static struct dentry *drm_debugfs_root;
> >>
> >> -void drm_err(const char *format, ...)
> >> +#define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"
> >> +
> >> +void drm_dev_printk(const struct device *dev, const char *level,
> >> + unsigned int category, const char *function_name,
> >> + const char *prefix, const char *format, ...)
> >> {
> >> struct va_format vaf;
> >> va_list args;
> >>
> >> - va_start(args, format);
> >> + if (category != DRM_UT_NONE && !(drm_debug & category))
> >> + return;
> >>
> >> + va_start(args, format);
> >> vaf.fmt = format;
> >> vaf.va = &args;
> >>
> >> - printk(KERN_ERR "[" DRM_NAME ":%ps] *ERROR* %pV",
> >> - __builtin_return_address(0), &vaf);
> >> + dev_printk(level, dev, DRM_PRINTK_FMT, function_name, prefix,
> >> + &vaf);
> >>
> >> va_end(args);
> >> }
> >> -EXPORT_SYMBOL(drm_err);
> >> +EXPORT_SYMBOL(drm_dev_printk);
> >>
> >> -void drm_ut_debug_printk(const char *function_name, const char *format, ...)
> >> +void drm_printk(const char *level, unsigned int category,
> >> + const char *function_name, const char *prefix,
> >> + const char *format, ...)
> >> {
> >> struct va_format vaf;
> >> va_list args;
> >>
> >> + if (category != DRM_UT_NONE && !(drm_debug & category))
> >> + return;
> >> +
> >> va_start(args, format);
> >> vaf.fmt = format;
> >> vaf.va = &args;
> >>
> >> - printk(KERN_DEBUG "[" DRM_NAME ":%s] %pV", function_name, &vaf);
> >> + printk("%s" DRM_PRINTK_FMT, level, function_name, prefix, &vaf);
> >>
> >> va_end(args);
> >> }
> >> -EXPORT_SYMBOL(drm_ut_debug_printk);
> >> +EXPORT_SYMBOL(drm_printk);
> >>
> >> /*
> >> * DRM Minors
> >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> >> index f8e87fd..94eb138 100644
> >> --- a/include/drm/drmP.h
> >> +++ b/include/drm/drmP.h
> >> @@ -127,6 +127,7 @@ struct dma_buf_attachment;
> >> * run-time by echoing the debug value in its sysfs node:
> >> * # echo 0xf > /sys/module/drm/parameters/debug
> >> */
> >> +#define DRM_UT_NONE 0x00
> >> #define DRM_UT_CORE 0x01
> >> #define DRM_UT_DRIVER 0x02
> >> #define DRM_UT_KMS 0x04
> >> @@ -134,11 +135,15 @@ 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, ...);
> >> -extern __printf(1, 2)
> >> -void drm_err(const char *format, ...);
> >> +extern __printf(6, 7)
> >> +void drm_dev_printk(const struct device *dev, const char *level,
> >> + unsigned int category, const char *function_name,
> >> + const char *prefix, const char *format, ...);
> >> +
> >> +extern __printf(5, 6)
> >> +void drm_printk(const char *level, unsigned int category,
> >> + const char *function_name, const char *prefix,
> >> + const char *format, ...);
> >>
> >> /***********************************************************************/
> >> /** \name DRM template customization defaults */
> >> @@ -169,8 +174,12 @@ void drm_err(const char *format, ...);
> >> * \param fmt printf() like format string.
> >> * \param arg arguments
> >> */
> >> -#define DRM_ERROR(fmt, ...) \
> >> - drm_err(fmt, ##__VA_ARGS__)
> >> +#define DRM_DEV_ERROR(dev, fmt, ...) \
> >> + drm_dev_printk(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*",\
> >> + fmt, ##__VA_ARGS__)
> >> +#define DRM_ERROR(fmt, ...) \
> >> + drm_printk(KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt, \
> >> + ##__VA_ARGS__)
> >>
> >> /**
> >> * Rate limited error output. Like DRM_ERROR() but won't flood the log.
> >> @@ -178,21 +187,33 @@ void drm_err(const char *format, ...);
> >> * \param fmt printf() like format string.
> >> * \param arg arguments
> >> */
> >> -#define DRM_ERROR_RATELIMITED(fmt, ...) \
> >> +#define DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...) \
> >> ({ \
> >> static DEFINE_RATELIMIT_STATE(_rs, \
> >> DEFAULT_RATELIMIT_INTERVAL, \
> >> DEFAULT_RATELIMIT_BURST); \
> >> \
> >> if (__ratelimit(&_rs)) \
> >> - drm_err(fmt, ##__VA_ARGS__); \
> >> + DRM_DEV_ERROR(dev, fmt, ##__VA_ARGS__); \
> >> })
> >> +#define DRM_ERROR_RATELIMITED(fmt, ...) \
> >> + DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
> >
> > Shouldn't this be another define that calls DRM_ERROR(...) instead of
> > DRM_DEV_ERROR(NULL, ...)? Same for the other ones.
> >
>
> No, I don't think so. The key here is the _RATELIMITED part (and _ONCE
> below). I didn't want to duplicate the ratelimit code in the above
> macro for the non-dev case, so I just call the dev version with NULL.
> It doesn't have the bloat problem above since there aren't too many
> consumers of DRM_ERROR_RATELIMITED, so it's Ok to simplify the paths.
>
> Sean
Happy to to hear that, as I don't really like code duplication.
So, the current version is:
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
Cheers,
Eric
>
> > With that changed, this is:
> > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> >
> > Cheers,
> > Eric
> >
> >>
> >> -#define DRM_INFO(fmt, ...) \
> >> - printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
> >> +#define DRM_DEV_INFO(dev, fmt, ...) \
> >> + drm_dev_printk(dev, KERN_INFO, DRM_UT_NONE, __func__, "", fmt, \
> >> + ##__VA_ARGS__)
> >> +#define DRM_INFO(fmt, ...) \
> >> + drm_printk(KERN_INFO, DRM_UT_NONE, __func__, "", fmt, ##__VA_ARGS__)
> >>
> >> -#define DRM_INFO_ONCE(fmt, ...) \
> >> - printk_once(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
> >> +#define DRM_DEV_INFO_ONCE(dev, fmt, ...) \
> >> +({ \
> >> + static bool __print_once __read_mostly; \
> >> + if (!__print_once) { \
> >> + __print_once = true; \
> >> + DRM_DEV_INFO(dev, fmt, ##__VA_ARGS__); \
> >> + } \
> >> +})
> >> +#define DRM_INFO_ONCE(fmt, ...) DRM_DEV_INFO_ONCE(NULL, fmt, ##__VA_ARGS__)
> >>
> >> /**
> >> * Debug output.
> >> @@ -200,52 +221,51 @@ void drm_err(const char *format, ...);
> >> * \param fmt printf() like format string.
> >> * \param arg arguments
> >> */
> >> +#define DRM_DEV_DEBUG(dev, fmt, args...) \
> >> + drm_dev_printk(dev, KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt, \
> >> + ##args)
> >> #define DRM_DEBUG(fmt, args...) \
> >> - do { \
> >> - if (unlikely(drm_debug & DRM_UT_CORE)) \
> >> - drm_ut_debug_printk(__func__, fmt, ##args); \
> >> - } while (0)
> >> + drm_printk(KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt, ##args)
> >>
> >> +#define DRM_DEV_DEBUG_DRIVER(dev, fmt, args...) \
> >> + drm_dev_printk(dev, KERN_DEBUG, DRM_UT_DRIVER, __func__, "", \
> >> + fmt, ##args)
> >> #define DRM_DEBUG_DRIVER(fmt, args...) \
> >> - do { \
> >> - if (unlikely(drm_debug & DRM_UT_DRIVER)) \
> >> - drm_ut_debug_printk(__func__, fmt, ##args); \
> >> - } while (0)
> >> + drm_printk(KERN_DEBUG, DRM_UT_DRIVER, __func__, "", fmt, ##args)
> >> +
> >> +#define DRM_DEV_DEBUG_KMS(dev, fmt, args...) \
> >> + drm_dev_printk(dev, KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt, \
> >> + ##args)
> >> #define DRM_DEBUG_KMS(fmt, args...) \
> >> - do { \
> >> - if (unlikely(drm_debug & DRM_UT_KMS)) \
> >> - drm_ut_debug_printk(__func__, fmt, ##args); \
> >> - } while (0)
> >> + drm_printk(KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt, ##args)
> >> +
> >> +#define DRM_DEV_DEBUG_PRIME(dev, fmt, args...) \
> >> + drm_dev_printk(dev, KERN_DEBUG, DRM_UT_PRIME, __func__, "", \
> >> + fmt, ##args)
> >> #define DRM_DEBUG_PRIME(fmt, args...) \
> >> - do { \
> >> - if (unlikely(drm_debug & DRM_UT_PRIME)) \
> >> - drm_ut_debug_printk(__func__, fmt, ##args); \
> >> - } while (0)
> >> + drm_printk(KERN_DEBUG, DRM_UT_PRIME, __func__, "", fmt, ##args)
> >> +
> >> +#define DRM_DEV_DEBUG_ATOMIC(dev, fmt, args...) \
> >> + drm_dev_printk(dev, KERN_DEBUG, DRM_UT_ATOMIC, __func__, "", \
> >> + fmt, ##args)
> >> #define DRM_DEBUG_ATOMIC(fmt, args...) \
> >> - do { \
> >> - if (unlikely(drm_debug & DRM_UT_ATOMIC)) \
> >> - drm_ut_debug_printk(__func__, fmt, ##args); \
> >> - } while (0)
> >> + drm_printk(KERN_DEBUG, DRM_UT_ATOMIC, __func__, "", fmt, ##args)
> >> +
> >> +#define DRM_DEV_DEBUG_VBL(dev, fmt, args...) \
> >> + drm_dev_printk(dev, KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt, \
> >> + ##args)
> >> #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_DEFINE_DEBUG_RATELIMITED(level, fmt, args...) \
> >> - do { \
> >> - if (unlikely(drm_debug & DRM_UT_ ## level)) { \
> >> - static DEFINE_RATELIMIT_STATE( \
> >> - _rs, \
> >> - DEFAULT_RATELIMIT_INTERVAL, \
> >> - DEFAULT_RATELIMIT_BURST); \
> >> - \
> >> - if (__ratelimit(&_rs)) { \
> >> - drm_ut_debug_printk(__func__, fmt, \
> >> - ##args); \
> >> - } \
> >> - } \
> >> - } while (0)
> >> + drm_printk(KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt, ##args)
> >> +
> >> +#define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, level, fmt, args...) \
> >> +({ \
> >> + static DEFINE_RATELIMIT_STATE(_rs, \
> >> + DEFAULT_RATELIMIT_INTERVAL, \
> >> + DEFAULT_RATELIMIT_BURST); \
> >> + if (__ratelimit(&_rs)) \
> >> + drm_dev_printk(dev, KERN_DEBUG, DRM_UT_ ## level, \
> >> + __func__, "", fmt, ##args); \
> >> +})
> >>
> >> /**
> >> * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
> >> @@ -253,14 +273,22 @@ void drm_err(const char *format, ...);
> >> * \param fmt printf() like format string.
> >> * \param arg arguments
> >> */
> >> +#define DRM_DEV_DEBUG_RATELIMITED(dev, fmt, args...) \
> >> + DEV__DRM_DEFINE_DEBUG_RATELIMITED(dev, CORE, fmt, ##args)
> >> #define DRM_DEBUG_RATELIMITED(fmt, args...) \
> >> - _DRM_DEFINE_DEBUG_RATELIMITED(CORE, fmt, ##args)
> >> + DRM_DEV_DEBUG_RATELIMITED(NULL, fmt, ##args)
> >> +#define DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, args...) \
> >> + _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRIVER, fmt, ##args)
> >> #define DRM_DEBUG_DRIVER_RATELIMITED(fmt, args...) \
> >> - _DRM_DEFINE_DEBUG_RATELIMITED(DRIVER, fmt, ##args)
> >> + DRM_DEV_DEBUG_DRIVER_RATELIMITED(NULL, fmt, ##args)
> >> +#define DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, args...) \
> >> + _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, KMS, fmt, ##args)
> >> #define DRM_DEBUG_KMS_RATELIMITED(fmt, args...) \
> >> - _DRM_DEFINE_DEBUG_RATELIMITED(KMS, fmt, ##args)
> >> + DRM_DEV_DEBUG_KMS_RATELIMITED(NULL, fmt, ##args)
> >> +#define DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, args...) \
> >> + _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, PRIME, fmt, ##args)
> >> #define DRM_DEBUG_PRIME_RATELIMITED(fmt, args...) \
> >> - _DRM_DEFINE_DEBUG_RATELIMITED(PRIME, fmt, ##args)
> >> + DRM_DEV_DEBUG_PRIME_RATELIMITED(NULL, fmt, ##args)
> >>
> >> /*@}*/
> >>
> >> --
> >> 2.8.0.rc3.226.g39d4020
> >>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2016-08-16 16:56 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-12 17:00 [PATCH 0/2] Introduce DRM_DEV_* logging Sean Paul
2016-08-12 17:00 ` [PATCH 1/2] drm: Introduce DRM_DEV_* log messages Sean Paul
[not found] ` <1471021254-2563-2-git-send-email-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2016-08-12 17:13 ` Chris Wilson
2016-08-12 17:23 ` Sean Paul
2016-08-12 17:30 ` [PATCH v2 " Sean Paul
[not found] ` <1471023000-3820-1-git-send-email-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2016-08-12 18:39 ` Chris Wilson
2016-08-12 19:04 ` Sean Paul
2016-08-12 19:26 ` Lukas Wunner
2016-08-12 19:44 ` Chris Wilson
2016-08-12 19:48 ` Sean Paul
2016-08-12 19:50 ` Lukas Wunner
2016-08-12 20:29 ` [PATCH v3 " Sean Paul
[not found] ` <1471033777-9922-1-git-send-email-seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
2016-08-12 20:53 ` Chris Wilson
2016-08-15 12:54 ` Eric Engestrom
2016-08-15 23:18 ` [PATCH v4] " Sean Paul
2016-08-16 12:28 ` Eric Engestrom
2016-08-16 16:18 ` Sean Paul
2016-08-16 16:56 ` Eric Engestrom [this message]
2016-08-18 16:39 ` Sean Paul
2016-08-12 17:00 ` [PATCH 2/2] drm/rockchip: Use DRM_DEV_ERROR in vop Sean Paul
2016-08-18 8:53 ` Mark yao
2016-08-18 16:38 ` Sean Paul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160816165621.GS10429@imgtec.com \
--to=eric.engestrom@imgtec.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=seanpaul@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.