[Intel-gfx] [PATCH 8/8] drm/print: introduce new struct drm_device based logging macros

[Ruhl, Michael J]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
>Jani Nikula
>Sent: Wednesday, October 9, 2019 11:38 AM
>To: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
>Cc: Nikula, Jani <jani.nikula at intel.com>; Sam Ravnborg <sam at ravnborg.org>
>Subject: [Intel-gfx] [PATCH 8/8] drm/print: introduce new struct drm_device
>based logging macros
>
>Add new struct drm_device based logging macros modeled after the core
>kernel device based logging macros. These would be preferred over the
>drm printk and struct device based macros in drm code, where possible.
>
>We have existing drm specific struct device based logging functions, but
>they are too verbose to use for two main reasons:
>
> * The names are unnecessarily long, for example DRM_DEV_DEBUG_KMS().
>
> * The use of struct device over struct drm_device is too generic for
>   most users, leading to an extra dereference.
>
>For example:
>
>	DRM_DEV_DEBUG_KMS(drm->dev, "Hello, world\n");
>
>vs.
>
>	drm_dbg_kms(drm, "Hello, world\n");
>
>It's a matter of taste, but the SHOUTING UPPERCASE has been argued to be
>less readable than lowercase.
>
>Some names are changed from old DRM names to be based on the core
>kernel
>logging functions. For example, NOTE -> notice, ERROR -> err, DEBUG ->
>dbg.
>
>Due to the conflation of DRM_DEBUG and DRM_DEBUG_DRIVER macro use
>(DRM_DEBUG is used widely in drivers though it's supposed to be a core
>debugging category), they are named as drm_dbg_core and drm_dbg,
>respectively.
>
>The drm_err and _once/_ratelimited variants no longer include the
>function name in order to be able to use the core device based logging
>macros. Arguably this is not a significant change; error messages should
>not be so common to be only distinguishable by the function name.
>
>Ratelimited debug logging macros are to be added later.
>
>Cc: Sam Ravnborg <sam at ravnborg.org>
>Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>
>---
>
>With something like this, I think i915 could start migrating to
>drm_device based logging. I have a hard time convincing myself or anyone
>about migrating to the DRM_DEV_* variants.
>---
> include/drm/drm_print.h | 65
>+++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
>diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>index 085a9685270c..e4040dea0d8c 100644
>--- a/include/drm/drm_print.h
>+++ b/include/drm/drm_print.h
>@@ -322,6 +322,8 @@ static inline bool drm_debug_enabled(enum
>drm_debug_category category)
>
> /*
>  * struct device based logging
>+ *
>+ * Prefer drm_device based logging over device or prink based logging.
>  */
>
> __printf(3, 4)
>@@ -417,8 +419,71 @@ void drm_dev_dbg(const struct device *dev, enum
>drm_debug_category category,
> 	_DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_PRIME,
>		\
> 					  fmt, ##__VA_ARGS__)
>
>+/*
>+ * struct drm_device based logging
>+ *
>+ * Prefer drm_device based logging over device or prink based logging.
>+ */
>+
>+/* Helper for struct drm_device based logging. */
>+#define __drm_printk(drm, level, type, fmt, ...)			\
>+	dev_##level##type(drm->dev, "[drm] " fmt, ##__VA_ARGS__)

In the past, I have been able to do a:

#undef pr_fmt
#define pr_fmt(fmt) "[myinfo here] " fmt

And have the "[myinfo here]" portion show up the output.

Is it possible that you might be able to use this instead of "[drm] " fmt?

I think that the this will be the same result, but might be more in line with the printk interface?

Mike


>+
>+
>+#define drm_info(drm, fmt, ...)					\
>+	__drm_printk(drm, info,, fmt, ##__VA_ARGS__)
>+
>+#define drm_notice(drm, fmt, ...)				\
>+	__drm_printk(drm, notice,, fmt, ##__VA_ARGS__)
>+
>+#define drm_warn(drm, fmt, ...)					\
>+	__drm_printk(drm, warn,, fmt, ##__VA_ARGS__)
>+
>+#define drm_err(drm, fmt, ...)					\
>+	__drm_printk(drm, err,, "*ERROR* " fmt, ##__VA_ARGS__)
>+
>+
>+#define drm_info_once(drm, fmt, ...)				\
>+	__drm_printk(drm, info, _once, fmt, ##__VA_ARGS__)
>+
>+#define drm_notice_once(drm, fmt, ...)				\
>+	__drm_printk(drm, notice, _once, fmt, ##__VA_ARGS__)
>+
>+#define drm_warn_once(drm, fmt, ...)				\
>+	__drm_printk(drm, warn, _once, fmt, ##__VA_ARGS__)
>+
>+#define drm_err_once(drm, fmt, ...)				\
>+	__drm_printk(drm, err, _once, "*ERROR* " fmt, ##__VA_ARGS__)
>+
>+
>+#define drm_err_ratelimited(drm, fmt, ...)				\
>+	__drm_printk(drm, err, _ratelimited, "*ERROR* " fmt,
>##__VA_ARGS__)
>+
>+
>+#define drm_dbg_core(drm, fmt, ...)					\
>+	drm_dev_dbg(drm->dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
>+#define drm_dbg(drm, fmt, ...)
>	\
>+	drm_dev_dbg(drm->dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>+#define drm_dbg_kms(drm, fmt, ...)					\
>+	drm_dev_dbg(drm->dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
>+#define drm_dbg_prime(drm, fmt, ...)
>	\
>+	drm_dev_dbg(drm->dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
>+#define drm_dbg_atomic(drm, fmt, ...)
>	\
>+	drm_dev_dbg(drm->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
>+#define drm_dbg_vbl(drm, fmt, ...)					\
>+	drm_dev_dbg(drm->dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
>+#define drm_dbg_state(drm, fmt, ...)					\
>+	drm_dev_dbg(drm->dev, DRM_UT_STATE, fmt, ##__VA_ARGS__)
>+#define drm_dbg_lease(drm, fmt, ...)					\
>+	drm_dev_dbg(drm->dev, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
>+#define drm_dbg_dp(drm, fmt, ...)					\
>+	drm_dev_dbg(drm->dev, DRM_UT_DP, fmt, ##__VA_ARGS__)
>+
>+
> /*
>  * printk based logging
>+ *
>+ * Prefer drm_device based logging over device or prink based logging.
>  */
>
> __printf(2, 3)
>--
>2.20.1
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx