[PATCH 0/2] Introduce DRM_DEV_* logging

[PATCH 0/2] Introduce DRM_DEV_* logging

[Sean Paul]

This patch set refactors some of the logging functions and introduces
a new class of DRM_DEV_* messages which print the device name along
with the other drm goodness.

Note that the ERROR message format remains the same (with the *ERROR*
prefix), however INFO messages now include the function name, e.g.
instead of "[drm] My info message", it is now "[drm:my_function] My 
info message".

The second patch updates rockchip vop to use the new log messages.

This is tested on rockchip and compiled on i915/multi-v7/tegra/exynos/drm-misc

Sean Paul (2):
  drm: Introduce DRM_DEV_* log messages
  drm/rockchip: Use DRM_DEV_ERROR in vop

 drivers/gpu/drm/drm_drv.c                   |  31 +++----
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  24 ++---
 include/drm/drmP.h                          | 133 +++++++++++++++-------------
 3 files changed, 96 insertions(+), 92 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

This patch consolodates 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>
---
 drivers/gpu/drm/drm_drv.c |  31 +++++------
 include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
 2 files changed, 82 insertions(+), 82 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 57ce973..e8595f0 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
 
 static struct dentry *drm_debugfs_root;
 
-void drm_err(const char *format, ...)
+void drm_log(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);
-
-	vaf.fmt = format;
-	vaf.va = &args;
-
-	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;
+	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);
+	if (dev)
+		printk("%s%s [" DRM_NAME ":%s]%s %pV", level,
+		       dev_name(dev), function_name, prefix, &vaf);
+	else
+		printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
+		       prefix, &vaf);
 
 	va_end(args);
 }
-EXPORT_SYMBOL(drm_ut_debug_printk);
+EXPORT_SYMBOL(drm_log);
 
 /*
  * DRM Minors
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f8e87fd..9a6ace2 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,10 @@ 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_log(const struct device *dev, const char *level, unsigned int category,
+	     const char *function_name, const char *prefix,
+	     const char *format, ...);
 
 /***********************************************************************/
 /** \name DRM template customization defaults */
@@ -169,8 +169,10 @@ 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_log(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt,	\
+		##__VA_ARGS__)
+#define DRM_ERROR(fmt, ...) DRM_DEV_ERROR(NULL, fmt, ##__VA_ARGS__)
 
 /**
  * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
@@ -178,21 +180,31 @@ 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__)
 
-#define DRM_INFO(fmt, ...)				\
-	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
+#define DRM_DEV_INFO(dev, fmt, ...)			\
+	drm_log(dev, KERN_INFO, DRM_UT_NONE, __func__, "", fmt, ##__VA_ARGS__)
+#define DRM_INFO(fmt, ...) DRM_DEV_INFO(NULL, 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 +212,39 @@ void drm_err(const char *format, ...);
  * \param fmt printf() like format string.
  * \param arg 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_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)
+#define DRM_DEV_DEBUG(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt, ##args)
+#define DRM_DEBUG(fmt, args...)	DRM_DEV_DEBUG(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_DRIVER(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_DRIVER, __func__, "", fmt, ##args)
+#define DRM_DEBUG_DRIVER(fmt, args...) DRM_DEV_DEBUG_DRIVER(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_KMS(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt, ##args)
+#define DRM_DEBUG_KMS(fmt, args...) DRM_DEV_DEBUG_KMS(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_PRIME(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_PRIME, __func__, "", fmt, ##args)
+#define DRM_DEBUG_PRIME(fmt, args...) DRM_DEV_DEBUG_PRIME(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_ATOMIC(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_ATOMIC, __func__, "", fmt, ##args)
+#define DRM_DEBUG_ATOMIC(fmt, args...) DRM_DEV_DEBUG_ATOMIC(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_VBL(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt, ##args)
+#define DRM_DEBUG_VBL(fmt, args...) DRM_DEV_DEBUG_VBL(NULL, 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_log(dev, KERN_DEBUG, DRM_UT_ ## level, __func__,	\
+			"", fmt, ##args);				\
+})
 
 /**
  * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
@@ -253,14 +252,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

[PATCH 2/2] drm/rockchip: Use DRM_DEV_ERROR in vop

[Sean Paul]

Since we can have multiple vops, use DRM_DEV_ERROR to
make logs easier to process.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 31744fe..ec8ad00 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -238,7 +238,7 @@ static enum vop_data_format vop_convert_format(uint32_t format)
 	case DRM_FORMAT_NV24:
 		return VOP_FMT_YUV444SP;
 	default:
-		DRM_ERROR("unsupport format[%08x]\n", format);
+		DRM_ERROR("unsupported format[%08x]\n", format);
 		return -EINVAL;
 	}
 }
@@ -315,7 +315,7 @@ static void scl_vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
 	int vskiplines = 0;
 
 	if (dst_w > 3840) {
-		DRM_ERROR("Maximum destination width (3840) exceeded\n");
+		DRM_DEV_ERROR(vop->dev, "Maximum dst width (3840) exceeded\n");
 		return;
 	}
 
@@ -353,11 +353,11 @@ static void scl_vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
 	VOP_SCL_SET_EXT(vop, win, lb_mode, lb_mode);
 	if (lb_mode == LB_RGB_3840X2) {
 		if (yrgb_ver_scl_mode != SCALE_NONE) {
-			DRM_ERROR("ERROR : not allow yrgb ver scale\n");
+			DRM_DEV_ERROR(vop->dev, "not allow yrgb ver scale\n");
 			return;
 		}
 		if (cbcr_ver_scl_mode != SCALE_NONE) {
-			DRM_ERROR("ERROR : not allow cbcr ver scale\n");
+			DRM_DEV_ERROR(vop->dev, "not allow cbcr ver scale\n");
 			return;
 		}
 		vsu_mode = SCALE_UP_BIL;
@@ -970,7 +970,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
 		VOP_CTRL_SET(vop, mipi_en, 1);
 		break;
 	default:
-		DRM_ERROR("unsupport connector_type[%d]\n", s->output_type);
+		DRM_DEV_ERROR(vop->dev, "unsupported connector_type [%d]\n",
+			      s->output_type);
 	}
 	VOP_CTRL_SET(vop, out_mode, s->output_mode);
 
@@ -1154,7 +1155,8 @@ static irqreturn_t vop_isr(int irq, void *data)
 
 	/* Unhandled irqs are spurious. */
 	if (active_irqs)
-		DRM_ERROR("Unknown VOP IRQs: %#02x\n", active_irqs);
+		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
+			      active_irqs);
 
 	return ret;
 }
@@ -1189,7 +1191,8 @@ static int vop_create_crtc(struct vop *vop)
 					       win_data->phy->nformats,
 					       win_data->type, NULL);
 		if (ret) {
-			DRM_ERROR("failed to initialize plane\n");
+			DRM_DEV_ERROR(vop->dev, "failed to init plane %d\n",
+				      ret);
 			goto err_cleanup_planes;
 		}
 
@@ -1227,7 +1230,8 @@ static int vop_create_crtc(struct vop *vop)
 					       win_data->phy->nformats,
 					       win_data->type, NULL);
 		if (ret) {
-			DRM_ERROR("failed to initialize overlay plane\n");
+			DRM_DEV_ERROR(vop->dev, "failed to init overlay %d\n",
+				      ret);
 			goto err_cleanup_crtc;
 		}
 		drm_plane_helper_add(&vop_win->base, &plane_helper_funcs);
@@ -1235,8 +1239,8 @@ static int vop_create_crtc(struct vop *vop)
 
 	port = of_get_child_by_name(dev->of_node, "port");
 	if (!port) {
-		DRM_ERROR("no port node found in %s\n",
-			  dev->of_node->full_name);
+		DRM_DEV_ERROR(vop->dev, "no port node found in %s\n",
+			      dev->of_node->full_name);
 		ret = -ENOENT;
 		goto err_cleanup_crtc;
 	}
-- 
2.8.0.rc3.226.g39d4020

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 01:00:53PM -0400, Sean Paul wrote:
> This patch consolodates 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.

> +void drm_log(const struct device *dev, const char *level, unsigned int category,
> +	     const char *function_name, const char *prefix,
> +	     const char *format, ...)
>  {
> +	if (dev)
> +		printk("%s%s [" DRM_NAME ":%s]%s %pV", level,
> +		       dev_name(dev), function_name, prefix, &vaf);
> +	else
> +		printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
> +		       prefix, &vaf);

My hope was that we would migrate towards dev_printk so that our log
messages would have better conformity, especially between our messages
and those printed by subsystems on our behalf (using our struct device).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

On Fri, Aug 12, 2016 at 1:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Aug 12, 2016 at 01:00:53PM -0400, Sean Paul wrote:
>> This patch consolodates 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.
>
>> +void drm_log(const struct device *dev, const char *level, unsigned int category,
>> +          const char *function_name, const char *prefix,
>> +          const char *format, ...)
>>  {
>> +     if (dev)
>> +             printk("%s%s [" DRM_NAME ":%s]%s %pV", level,
>> +                    dev_name(dev), function_name, prefix, &vaf);
>> +     else
>> +             printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
>> +                    prefix, &vaf);
>
> My hope was that we would migrate towards dev_printk so that our log
> messages would have better conformity, especially between our messages
> and those printed by subsystems on our behalf (using our struct device).

Yep, that makes sense, v2 incoming.

Sean

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

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)


 drivers/gpu/drm/drm_drv.c |  31 +++++------
 include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
 2 files changed, 82 insertions(+), 82 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 57ce973..edd3291 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
 
 static struct dentry *drm_debugfs_root;
 
-void drm_err(const char *format, ...)
+void drm_log(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);
-
-	vaf.fmt = format;
-	vaf.va = &args;
-
-	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;
+	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);
+	if (dev)
+		dev_printk(level, dev, "[" DRM_NAME ":%s]%s %pV", function_name,
+			   prefix, &vaf);
+	else
+		printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
+		       prefix, &vaf);
 
 	va_end(args);
 }
-EXPORT_SYMBOL(drm_ut_debug_printk);
+EXPORT_SYMBOL(drm_log);
 
 /*
  * DRM Minors
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f8e87fd..9a6ace2 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,10 @@ 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_log(const struct device *dev, const char *level, unsigned int category,
+	     const char *function_name, const char *prefix,
+	     const char *format, ...);
 
 /***********************************************************************/
 /** \name DRM template customization defaults */
@@ -169,8 +169,10 @@ 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_log(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt,	\
+		##__VA_ARGS__)
+#define DRM_ERROR(fmt, ...) DRM_DEV_ERROR(NULL, fmt, ##__VA_ARGS__)
 
 /**
  * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
@@ -178,21 +180,31 @@ 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__)
 
-#define DRM_INFO(fmt, ...)				\
-	printk(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
+#define DRM_DEV_INFO(dev, fmt, ...)			\
+	drm_log(dev, KERN_INFO, DRM_UT_NONE, __func__, "", fmt, ##__VA_ARGS__)
+#define DRM_INFO(fmt, ...) DRM_DEV_INFO(NULL, 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 +212,39 @@ void drm_err(const char *format, ...);
  * \param fmt printf() like format string.
  * \param arg 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_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)
+#define DRM_DEV_DEBUG(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_CORE, __func__, "", fmt, ##args)
+#define DRM_DEBUG(fmt, args...)	DRM_DEV_DEBUG(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_DRIVER(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_DRIVER, __func__, "", fmt, ##args)
+#define DRM_DEBUG_DRIVER(fmt, args...) DRM_DEV_DEBUG_DRIVER(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_KMS(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_KMS, __func__, "", fmt, ##args)
+#define DRM_DEBUG_KMS(fmt, args...) DRM_DEV_DEBUG_KMS(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_PRIME(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_PRIME, __func__, "", fmt, ##args)
+#define DRM_DEBUG_PRIME(fmt, args...) DRM_DEV_DEBUG_PRIME(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_ATOMIC(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_ATOMIC, __func__, "", fmt, ##args)
+#define DRM_DEBUG_ATOMIC(fmt, args...) DRM_DEV_DEBUG_ATOMIC(NULL, fmt, ##args)
+
+#define DRM_DEV_DEBUG_VBL(dev, fmt, args...)	\
+	drm_log(dev, KERN_DEBUG, DRM_UT_VBL, __func__, "", fmt, ##args)
+#define DRM_DEBUG_VBL(fmt, args...) DRM_DEV_DEBUG_VBL(NULL, 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_log(dev, KERN_DEBUG, DRM_UT_ ## level, __func__,	\
+			"", fmt, ##args);				\
+})
 
 /**
  * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
@@ -253,14 +252,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

Re: [PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 01:30:00PM -0400, 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v2:
> 	- Use dev_printk for the dev variant (Chris Wilson)
> 
> 
>  drivers/gpu/drm/drm_drv.c |  31 +++++------
>  include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
>  2 files changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 57ce973..edd3291 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
>  
>  static struct dentry *drm_debugfs_root;
>  
> -void drm_err(const char *format, ...)
> +void drm_log(const struct device *dev, const char *level, unsigned int category,

I would have called this drm_printk() to match the function it wraps.

> +	     const char *function_name, const char *prefix,
> +	     const char *format, ...)
>  {
>  	struct va_format vaf;
>  	va_list args;
>  
> -	va_start(args, format);
> -
> -	vaf.fmt = format;
> -	vaf.va = &args;
> -
> -	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;
> +	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);
> +	if (dev)
> +		dev_printk(level, dev, "[" DRM_NAME ":%s]%s %pV", function_name,
> +			   prefix, &vaf);
> +	else
> +		printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
> +		       prefix, &vaf);

lgtm.

> -#define DRM_ERROR(fmt, ...)				\
> -	drm_err(fmt, ##__VA_ARGS__)
> +#define DRM_DEV_ERROR(dev, fmt, ...)					\
> +	drm_log(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt,	\
> +		##__VA_ARGS__)
> +#define DRM_ERROR(fmt, ...) DRM_DEV_ERROR(NULL, fmt, ##__VA_ARGS__)

And these look like a reasonable solution given the constraints.

Out of curiosity, how much did the kernel build grow by adding a NULL
parameter everywhere?

Reviewed-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

On Fri, Aug 12, 2016 at 2:39 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Aug 12, 2016 at 01:30:00PM -0400, 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)
>>
>>
>>  drivers/gpu/drm/drm_drv.c |  31 +++++------
>>  include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
>>  2 files changed, 82 insertions(+), 82 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 57ce973..edd3291 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
>>
>>  static struct dentry *drm_debugfs_root;
>>
>> -void drm_err(const char *format, ...)
>> +void drm_log(const struct device *dev, const char *level, unsigned int category,
>
> I would have called this drm_printk() to match the function it wraps.
>
>> +          const char *function_name, const char *prefix,
>> +          const char *format, ...)
>>  {
>>       struct va_format vaf;
>>       va_list args;
>>
>> -     va_start(args, format);
>> -
>> -     vaf.fmt = format;
>> -     vaf.va = &args;
>> -
>> -     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;
>> +     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);
>> +     if (dev)
>> +             dev_printk(level, dev, "[" DRM_NAME ":%s]%s %pV", function_name,
>> +                        prefix, &vaf);
>> +     else
>> +             printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name,
>> +                    prefix, &vaf);
>
> lgtm.
>
>> -#define DRM_ERROR(fmt, ...)                          \
>> -     drm_err(fmt, ##__VA_ARGS__)
>> +#define DRM_DEV_ERROR(dev, fmt, ...)                                 \
>> +     drm_log(dev, KERN_ERR, DRM_UT_NONE, __func__, " *ERROR*", fmt,  \
>> +             ##__VA_ARGS__)
>> +#define DRM_ERROR(fmt, ...) DRM_DEV_ERROR(NULL, fmt, ##__VA_ARGS__)
>
> And these look like a reasonable solution given the constraints.
>
> Out of curiosity, how much did the kernel build grow by adding a NULL
> parameter everywhere?


uncompressed vmlinux is 8286 bytes larger on my i915 build, 34832 on exynos

bzImage on i915 build is 2912 bytes larger and zImage on exynos is
6752 bytes larger

Sean

>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Lukas Wunner]

On Fri, Aug 12, 2016 at 07:39:38PM +0100, Chris Wilson wrote:
> On Fri, Aug 12, 2016 at 01:30:00PM -0400, 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)
> > 
> > 
> >  drivers/gpu/drm/drm_drv.c |  31 +++++------
> >  include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
> >  2 files changed, 82 insertions(+), 82 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 57ce973..edd3291 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
> >  
> >  static struct dentry *drm_debugfs_root;
> >  
> > -void drm_err(const char *format, ...)
> > +void drm_log(const struct device *dev, const char *level, unsigned int category,
> 
> I would have called this drm_printk() to match the function it wraps.

lxr.free-electrons.com says dev_info() is used in 2056 files whereas
dev_printk() is only used in 90 files. And dev_log() doesn't exist.
So drm_info() would arguably make the most sense.

Best regards,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 09:26:32PM +0200, Lukas Wunner wrote:
> On Fri, Aug 12, 2016 at 07:39:38PM +0100, Chris Wilson wrote:
> > On Fri, Aug 12, 2016 at 01:30:00PM -0400, 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)
> > > 
> > > 
> > >  drivers/gpu/drm/drm_drv.c |  31 +++++------
> > >  include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
> > >  2 files changed, 82 insertions(+), 82 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 57ce973..edd3291 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
> > >  
> > >  static struct dentry *drm_debugfs_root;
> > >  
> > > -void drm_err(const char *format, ...)
> > > +void drm_log(const struct device *dev, const char *level, unsigned int category,
> > 
> > I would have called this drm_printk() to match the function it wraps.
> 
> lxr.free-electrons.com says dev_info() is used in 2056 files whereas
> dev_printk() is only used in 90 files. And dev_log() doesn't exist.
> So drm_info() would arguably make the most sense.

dev_printk is the underlying mechanism, dev_log() is a curry function
calling dev_printk with some parameters already provided.

Speaking of which, if we did separate drm_printk() and drm_dev_printk(),
if drm_printk just called drm_dev_printk(NULL, ...) we would barely grow
the build.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

On Fri, Aug 12, 2016 at 3:44 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Aug 12, 2016 at 09:26:32PM +0200, Lukas Wunner wrote:
>> On Fri, Aug 12, 2016 at 07:39:38PM +0100, Chris Wilson wrote:
>> > On Fri, Aug 12, 2016 at 01:30:00PM -0400, 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)
>> > >
>> > >
>> > >  drivers/gpu/drm/drm_drv.c |  31 +++++------
>> > >  include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
>> > >  2 files changed, 82 insertions(+), 82 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> > > index 57ce973..edd3291 100644
>> > > --- a/drivers/gpu/drm/drm_drv.c
>> > > +++ b/drivers/gpu/drm/drm_drv.c
>> > > @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
>> > >
>> > >  static struct dentry *drm_debugfs_root;
>> > >
>> > > -void drm_err(const char *format, ...)
>> > > +void drm_log(const struct device *dev, const char *level, unsigned int category,
>> >
>> > I would have called this drm_printk() to match the function it wraps.
>>
>> lxr.free-electrons.com says dev_info() is used in 2056 files whereas
>> dev_printk() is only used in 90 files. And dev_log() doesn't exist.
>> So drm_info() would arguably make the most sense.
>
> dev_printk is the underlying mechanism, dev_log() is a curry function
> calling dev_printk with some parameters already provided.
>
> Speaking of which, if we did separate drm_printk() and drm_dev_printk(),
> if drm_printk just called drm_dev_printk(NULL, ...) we would barely grow
> the build.

Thanks for the suggestion, will revise.

Sean

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm: Introduce DRM_DEV_* log messages

[Lukas Wunner]

On Fri, Aug 12, 2016 at 08:44:38PM +0100, Chris Wilson wrote:
> On Fri, Aug 12, 2016 at 09:26:32PM +0200, Lukas Wunner wrote:
> > On Fri, Aug 12, 2016 at 07:39:38PM +0100, Chris Wilson wrote:
> > > On Fri, Aug 12, 2016 at 01:30:00PM -0400, 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)
> > > > 
> > > > 
> > > >  drivers/gpu/drm/drm_drv.c |  31 +++++------
> > > >  include/drm/drmP.h        | 133 ++++++++++++++++++++++++----------------------
> > > >  2 files changed, 82 insertions(+), 82 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > > index 57ce973..edd3291 100644
> > > > --- a/drivers/gpu/drm/drm_drv.c
> > > > +++ b/drivers/gpu/drm/drm_drv.c
> > > > @@ -63,37 +63,30 @@ static struct idr drm_minors_idr;
> > > >  
> > > >  static struct dentry *drm_debugfs_root;
> > > >  
> > > > -void drm_err(const char *format, ...)
> > > > +void drm_log(const struct device *dev, const char *level, unsigned int category,
> > > 
> > > I would have called this drm_printk() to match the function it wraps.
> > 
> > lxr.free-electrons.com says dev_info() is used in 2056 files whereas
> > dev_printk() is only used in 90 files. And dev_log() doesn't exist.
> > So drm_info() would arguably make the most sense.
> 
> dev_printk is the underlying mechanism, dev_log() is a curry function
> calling dev_printk with some parameters already provided.

Ugh, sorry, I misread the code. You're right, drm_printk() would seem
more logical.

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/2] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

This patch consolidates all the various log functions/macros into
one uber function, drm_printk. It also introduces some new DRM_DEV_*
variants that use dev_printk to print the device name, which helps
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)

 drivers/gpu/drm/drm_drv.c |  25 ++++++---
 include/drm/drmP.h        | 140 +++++++++++++++++++++++++++-------------------
 2 files changed, 101 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 57ce973..e141ead 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -63,37 +63,46 @@ static struct idr drm_minors_idr;
 
 static struct dentry *drm_debugfs_root;
 
-void drm_err(const char *format, ...)
+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_NAME ":%s]%s %pV", 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_NAME ":%s]%s %pV", 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__)
 
-#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

Re: [PATCH v3 1/2] drm: Introduce DRM_DEV_* log messages

[Chris Wilson]

On Fri, Aug 12, 2016 at 04:29:37PM -0400, Sean Paul wrote:
> This patch consolidates all the various log functions/macros into
> one uber function, drm_printk. It also introduces some new DRM_DEV_*
> variants that use dev_printk to print the device name, which helps
> delineate multiple devices of the same type.
> 
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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)
> 
>  drivers/gpu/drm/drm_drv.c |  25 ++++++---
>  include/drm/drmP.h        | 140 +++++++++++++++++++++++++++-------------------
>  2 files changed, 101 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 57ce973..e141ead 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -63,37 +63,46 @@ static struct idr drm_minors_idr;
>  
>  static struct dentry *drm_debugfs_root;
>  
> -void drm_err(const char *format, ...)
> +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_NAME ":%s]%s %pV", function_name, prefix,
> +		   &vaf);

dev_printk does handle NULL dev, that's a relief!

>  
>  	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_NAME ":%s]%s %pV", level, function_name, prefix, &vaf);

Ok, I just tried to make a common drm_dev_printk_emit() and made a right
mess. A pair of functions is definitely better.

Reviewed-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v3 1/2] drm: Introduce DRM_DEV_* log messages

[Eric Engestrom]

On Fri, Aug 12, 2016 at 04:29:37PM -0400, Sean Paul wrote:
> This patch consolidates all the various log functions/macros into
> one uber function, drm_printk. It also introduces some new DRM_DEV_*
> variants that use dev_printk to print the device name, which helps
> 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)
> 
>  drivers/gpu/drm/drm_drv.c |  25 ++++++---
>  include/drm/drmP.h        | 140 +++++++++++++++++++++++++++-------------------
>  2 files changed, 101 insertions(+), 64 deletions(-)
> 

[snip]

> +	dev_printk(level, dev, "[" DRM_NAME ":%s]%s %pV", function_name, prefix,
> +		   &vaf);

[...]

> +	printk("%s[" DRM_NAME ":%s]%s %pV", level, function_name, prefix, &vaf);

What do you think of #define'ing them to make sure these two format strings
don't end up diverging at some point?

Cheers,
  Eric
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

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__)
 
-#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

Re: [PATCH v4] drm: Introduce DRM_DEV_* log messages

[Eric Engestrom]

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.

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

Re: [PATCH v4] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

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

> 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

Re: [PATCH v4] drm: Introduce DRM_DEV_* log messages

[Eric Engestrom]

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

Re: [PATCH 2/2] drm/rockchip: Use DRM_DEV_ERROR in vop

[Mark yao]

On 2016年08月13日 01:00, Sean Paul wrote:
> Since we can have multiple vops, use DRM_DEV_ERROR to
> make logs easier to process.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

looks good for me

Acked-by: Mark Yao <mark.yao@rockchip.com>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 31744fe..ec8ad00 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -238,7 +238,7 @@ static enum vop_data_format vop_convert_format(uint32_t format)
>   	case DRM_FORMAT_NV24:
>   		return VOP_FMT_YUV444SP;
>   	default:
> -		DRM_ERROR("unsupport format[%08x]\n", format);
> +		DRM_ERROR("unsupported format[%08x]\n", format);
>   		return -EINVAL;
>   	}
>   }
> @@ -315,7 +315,7 @@ static void scl_vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
>   	int vskiplines = 0;
>   
>   	if (dst_w > 3840) {
> -		DRM_ERROR("Maximum destination width (3840) exceeded\n");
> +		DRM_DEV_ERROR(vop->dev, "Maximum dst width (3840) exceeded\n");
>   		return;
>   	}
>   
> @@ -353,11 +353,11 @@ static void scl_vop_cal_scl_fac(struct vop *vop, const struct vop_win_data *win,
>   	VOP_SCL_SET_EXT(vop, win, lb_mode, lb_mode);
>   	if (lb_mode == LB_RGB_3840X2) {
>   		if (yrgb_ver_scl_mode != SCALE_NONE) {
> -			DRM_ERROR("ERROR : not allow yrgb ver scale\n");
> +			DRM_DEV_ERROR(vop->dev, "not allow yrgb ver scale\n");
>   			return;
>   		}
>   		if (cbcr_ver_scl_mode != SCALE_NONE) {
> -			DRM_ERROR("ERROR : not allow cbcr ver scale\n");
> +			DRM_DEV_ERROR(vop->dev, "not allow cbcr ver scale\n");
>   			return;
>   		}
>   		vsu_mode = SCALE_UP_BIL;
> @@ -970,7 +970,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>   		VOP_CTRL_SET(vop, mipi_en, 1);
>   		break;
>   	default:
> -		DRM_ERROR("unsupport connector_type[%d]\n", s->output_type);
> +		DRM_DEV_ERROR(vop->dev, "unsupported connector_type [%d]\n",
> +			      s->output_type);
>   	}
>   	VOP_CTRL_SET(vop, out_mode, s->output_mode);
>   
> @@ -1154,7 +1155,8 @@ static irqreturn_t vop_isr(int irq, void *data)
>   
>   	/* Unhandled irqs are spurious. */
>   	if (active_irqs)
> -		DRM_ERROR("Unknown VOP IRQs: %#02x\n", active_irqs);
> +		DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
> +			      active_irqs);
>   
>   	return ret;
>   }
> @@ -1189,7 +1191,8 @@ static int vop_create_crtc(struct vop *vop)
>   					       win_data->phy->nformats,
>   					       win_data->type, NULL);
>   		if (ret) {
> -			DRM_ERROR("failed to initialize plane\n");
> +			DRM_DEV_ERROR(vop->dev, "failed to init plane %d\n",
> +				      ret);
>   			goto err_cleanup_planes;
>   		}
>   
> @@ -1227,7 +1230,8 @@ static int vop_create_crtc(struct vop *vop)
>   					       win_data->phy->nformats,
>   					       win_data->type, NULL);
>   		if (ret) {
> -			DRM_ERROR("failed to initialize overlay plane\n");
> +			DRM_DEV_ERROR(vop->dev, "failed to init overlay %d\n",
> +				      ret);
>   			goto err_cleanup_crtc;
>   		}
>   		drm_plane_helper_add(&vop_win->base, &plane_helper_funcs);
> @@ -1235,8 +1239,8 @@ static int vop_create_crtc(struct vop *vop)
>   
>   	port = of_get_child_by_name(dev->of_node, "port");
>   	if (!port) {
> -		DRM_ERROR("no port node found in %s\n",
> -			  dev->of_node->full_name);
> +		DRM_DEV_ERROR(vop->dev, "no port node found in %s\n",
> +			      dev->of_node->full_name);
>   		ret = -ENOENT;
>   		goto err_cleanup_crtc;
>   	}


-- 
Ｍark Yao


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/rockchip: Use DRM_DEV_ERROR in vop

[Sean Paul]

On Thu, Aug 18, 2016 at 1:53 AM, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2016年08月13日 01:00, Sean Paul wrote:
>>
>> Since we can have multiple vops, use DRM_DEV_ERROR to
>> make logs easier to process.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>
>
> looks good for me
>
> Acked-by: Mark Yao <mark.yao@rockchip.com>
>

Applied to drm-misc

>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 24
>> ++++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 31744fe..ec8ad00 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -238,7 +238,7 @@ static enum vop_data_format
>> vop_convert_format(uint32_t format)
>>         case DRM_FORMAT_NV24:
>>                 return VOP_FMT_YUV444SP;
>>         default:
>> -               DRM_ERROR("unsupport format[%08x]\n", format);
>> +               DRM_ERROR("unsupported format[%08x]\n", format);
>>                 return -EINVAL;
>>         }
>>   }
>> @@ -315,7 +315,7 @@ static void scl_vop_cal_scl_fac(struct vop *vop, const
>> struct vop_win_data *win,
>>         int vskiplines = 0;
>>         if (dst_w > 3840) {
>> -               DRM_ERROR("Maximum destination width (3840) exceeded\n");
>> +               DRM_DEV_ERROR(vop->dev, "Maximum dst width (3840)
>> exceeded\n");
>>                 return;
>>         }
>>   @@ -353,11 +353,11 @@ static void scl_vop_cal_scl_fac(struct vop *vop,
>> const struct vop_win_data *win,
>>         VOP_SCL_SET_EXT(vop, win, lb_mode, lb_mode);
>>         if (lb_mode == LB_RGB_3840X2) {
>>                 if (yrgb_ver_scl_mode != SCALE_NONE) {
>> -                       DRM_ERROR("ERROR : not allow yrgb ver scale\n");
>> +                       DRM_DEV_ERROR(vop->dev, "not allow yrgb ver
>> scale\n");
>>                         return;
>>                 }
>>                 if (cbcr_ver_scl_mode != SCALE_NONE) {
>> -                       DRM_ERROR("ERROR : not allow cbcr ver scale\n");
>> +                       DRM_DEV_ERROR(vop->dev, "not allow cbcr ver
>> scale\n");
>>                         return;
>>                 }
>>                 vsu_mode = SCALE_UP_BIL;
>> @@ -970,7 +970,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
>>                 VOP_CTRL_SET(vop, mipi_en, 1);
>>                 break;
>>         default:
>> -               DRM_ERROR("unsupport connector_type[%d]\n",
>> s->output_type);
>> +               DRM_DEV_ERROR(vop->dev, "unsupported connector_type
>> [%d]\n",
>> +                             s->output_type);
>>         }
>>         VOP_CTRL_SET(vop, out_mode, s->output_mode);
>>   @@ -1154,7 +1155,8 @@ static irqreturn_t vop_isr(int irq, void *data)
>>         /* Unhandled irqs are spurious. */
>>         if (active_irqs)
>> -               DRM_ERROR("Unknown VOP IRQs: %#02x\n", active_irqs);
>> +               DRM_DEV_ERROR(vop->dev, "Unknown VOP IRQs: %#02x\n",
>> +                             active_irqs);
>>         return ret;
>>   }
>> @@ -1189,7 +1191,8 @@ static int vop_create_crtc(struct vop *vop)
>>                                                win_data->phy->nformats,
>>                                                win_data->type, NULL);
>>                 if (ret) {
>> -                       DRM_ERROR("failed to initialize plane\n");
>> +                       DRM_DEV_ERROR(vop->dev, "failed to init plane
>> %d\n",
>> +                                     ret);
>>                         goto err_cleanup_planes;
>>                 }
>>   @@ -1227,7 +1230,8 @@ static int vop_create_crtc(struct vop *vop)
>>                                                win_data->phy->nformats,
>>                                                win_data->type, NULL);
>>                 if (ret) {
>> -                       DRM_ERROR("failed to initialize overlay plane\n");
>> +                       DRM_DEV_ERROR(vop->dev, "failed to init overlay
>> %d\n",
>> +                                     ret);
>>                         goto err_cleanup_crtc;
>>                 }
>>                 drm_plane_helper_add(&vop_win->base, &plane_helper_funcs);
>> @@ -1235,8 +1239,8 @@ static int vop_create_crtc(struct vop *vop)
>>         port = of_get_child_by_name(dev->of_node, "port");
>>         if (!port) {
>> -               DRM_ERROR("no port node found in %s\n",
>> -                         dev->of_node->full_name);
>> +               DRM_DEV_ERROR(vop->dev, "no port node found in %s\n",
>> +                             dev->of_node->full_name);
>>                 ret = -ENOENT;
>>                 goto err_cleanup_crtc;
>>         }
>
>
>
> --
> Ｍark Yao
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm: Introduce DRM_DEV_* log messages

[Sean Paul]

On Tue, Aug 16, 2016 at 9:56 AM, Eric Engestrom
<eric.engestrom@imgtec.com> wrote:
> 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>
>


Thanks for the reviews. Applied to drm-misc

Sean

> 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