[RFC PATCH 0/9] drm: add more new-style logging functions

[RFC PATCH 0/9] drm: add more new-style logging functions

[Sam Ravnborg]

Jani Nikula introduced the new nice drm_* logging functions.
I looked at updating drm/panel/* to use these,
but was quickly faced with the fact that in
many cases there was a device * but no drm_device *.
And in some cases not even a device *.

So we would end up with a mixture of different type
of logging making it confusing when to use what.
Somethign that alos IMO hurts the readability of the code.

Likewise for review feedback where we cannot simply tell
people to use new style logging but have to explain when
to use what. This will be confusing.

This patchset starts by introducing brief documentation
of the new style logging functions - something I have missed.
And then it introduces new style logging for the remaining
types of logging.

    drm_dev_*	- when a device * is avialable
    drm_pr_*	- when no identification is available

The last patch change the now legacy logging functions to
use the new style logging functions and in the process
delete a few now unused functions.
No conversion of actual users included - that will come
later when we have agreed on namign etc.

The patchset is not yet tested - but it builds.
This is an early drop of the patches to trigger
feedback on naming, get early feedback on documentation,
and any other good comments.

	Sam

Sam Ravnborg (8):
      drm/print: document logging functions
      drm/print: move new style logging functions
      drm/print: add new logging helper for drm logging
      drm/print: add kernel-doc for drm_debug_enabled
      drm/print: rename drm_dev_dbg
      drm/print: add drm_dev_* logging functions
      drm/print: add drm_pr_ logging
      drm/print: let legacy logging use new style functions

 Documentation/gpu/drm-internals.rst |   6 +
 drivers/gpu/drm/drm_print.c         |  80 ------
 include/drm/drm_print.h             | 538 +++++++++++++++++++++++-------------
 3 files changed, 351 insertions(+), 273 deletions(-)


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

[PATCH v1 1/8] drm/print: document logging functions

[Sam Ravnborg]

This is the documentation I have missed when I looked for help
how to do proper logging. Hopefully it can help others.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 Documentation/gpu/drm-internals.rst |  6 ++
 include/drm/drm_print.h             | 91 ++++++++++++++++++++++++++---
 2 files changed, 90 insertions(+), 7 deletions(-)

diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index a73320576ca9..c2093611999c 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -164,6 +164,12 @@ File Operations
 Misc Utilities
 ==============
 
+Logging
+-------
+
+.. kernel-doc:: include/drm/drm_print.h
+   :doc: logging
+
 Printer
 -------
 
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 8f99d389792d..e9e31ace0afa 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -250,22 +250,42 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
 }
 
 /**
- * enum drm_debug_category - The DRM debug categories
+ * DOC: logging
+ *
+ * There is a set of functions/macros available used for logging
+ * in the DRM subsystem.
+ * Using the drm logging function enables that the logging is consistently
+ * prefixed with *[drm]* thus the logging is easy to recognize.
+ *
+ * Example of logging with *[drm]* prefix::
  *
- * Each of the DRM debug logging macros use a specific category, and the logging
- * is filtered by the drm.debug module parameter. This enum specifies the values
- * for the interface.
+ *   [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
+ *   [drm] Driver supports precise vblank timestamp query.
  *
- * Each DRM_DEBUG_<CATEGORY> macro logs to DRM_UT_<CATEGORY> category, except
- * DRM_DEBUG() logs to DRM_UT_CORE.
+ *
+ * Each of the debug logging macros use a specific category, and the logging
+ * is filtered by the drm.debug module parameter. The &drm_debug_category enum
+ * specifies the values for the interface.
+ *
+ * Each drm_dbg_<category> macro logs to a DRM_UT_<category> category,
+ * except drm_dbg() that logs to DRM_UT_DRIVER.
  *
  * Enabling verbose debug messages is done through the drm.debug parameter, each
  * category being enabled by a bit:
  *
  *  - drm.debug=0x1 will enable CORE messages
  *  - drm.debug=0x2 will enable DRIVER messages
+ *  - drm.debug=0x4 will enable KMS messages
+ *  - drm.debug=0x8 will enable PRIME messages
+ *  - drm.debug=0x10 will enable ATOMIC messages
+ *  - drm.debug=0x20 will enable VBL messages
+ *  - drm.debug=0x40 will enable STATE messages
+ *  - drm.debug=0x80 will enable LEASE messages
+ *  - drm.debug=0x100 will enable DP messages
+ *
+ * To enable more than one category OR the values - examples:
+ *
  *  - drm.debug=0x3 will enable CORE and DRIVER messages
- *  - ...
  *  - drm.debug=0x1ff will enable all messages
  *
  * An interesting feature is that it's possible to enable verbose logging at
@@ -273,6 +293,63 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
  *
  *   # echo 0xf > /sys/module/drm/parameters/debug
  *
+ *
+ * When a &drm_device * is available use one of the following logging functions.
+ * The same prototype is shared by all the logging functions
+ * that take a &drm_device * as first argument:
+ *
+ * .. code-block:: c
+ *
+ *   void drm_xxx(struct drm_device *, char * fmt, ...)
+ *
+ * Drivers can use the following functions for logging.
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   drm_dbg()
+ *   drm_info()
+ *   drm_notice()
+ *   drm_warn()
+ *   drm_err()
+ *
+ *   # Log only once
+ *   drm_info_once()
+ *   drm_notice_once()
+ *   drm_warn_once()
+ *   drm_err_once()
+ *
+ *   # Ratelimited - do not flood the logs
+ *   drm_err_ratelimited()
+ *
+ *   # Logging with a specific category
+ *   drm_dbg_core()
+ *   drm_dbg()		# Uses the DRIVER category
+ *   drm_dbg_kms()
+ *   drm_dbg_prime()
+ *   drm_dbg_atomic()
+ *   drm_dbg_vbl()
+ *   drm_dbg_state()
+ *   drm_dbg_lease()
+ *   drm_dbg_dp()
+ *
+ * See enum &drm_debug_category for a description of the categories.
+ *
+ * Logging when a &device * is available, but no &drm_device *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * TODO
+ *
+ * Logging when no &device * nor &drm_device * is available
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * TODO
+ *
+ * Obsoleted logging functions
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * The DRM_*() logging functions are deprecated - do not use them in new code.
+ */
+
+/**
+ * enum drm_debug_category - The DRM debug categories
  */
 enum drm_debug_category {
 	/**
-- 
2.20.1

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

[PATCH v1 2/8] drm/print: move new style logging functions

[Sam Ravnborg]

Move the new style logging function up right after the documentation of
their usage.
And note that remaining logging functions are legacy.

While moving, drop some extra lines.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_print.h | 117 +++++++++++++++++++---------------------
 1 file changed, 56 insertions(+), 61 deletions(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index e9e31ace0afa..d2b9ac6a6e18 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -397,6 +397,62 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
 	return unlikely(__drm_debug & category);
 }
 
+/*
+ * struct drm_device based logging
+ *
+ * Prefer drm_device based logging over device or printk based logging.
+ */
+
+/* Helper for struct drm_device based logging. */
+#define __drm_printk(drm, level, type, fmt, ...)			\
+	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
+
+
+#define drm_info(drm, fmt, ...)						\
+	__drm_printk((drm), info,, fmt, ##__VA_ARGS__)
+#define drm_notice(drm, fmt, ...)					\
+	__drm_printk((drm), notice,, fmt, ##__VA_ARGS__)
+#define drm_warn(drm, fmt, ...)						\
+	__drm_printk((drm), warn,, fmt, ##__VA_ARGS__)
+#define drm_err(drm, fmt, ...)						\
+	__drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
+
+#define drm_info_once(drm, fmt, ...)					\
+	__drm_printk((drm), info, _once, fmt, ##__VA_ARGS__)
+#define drm_notice_once(drm, fmt, ...)					\
+	__drm_printk((drm), notice, _once, fmt, ##__VA_ARGS__)
+#define drm_warn_once(drm, fmt, ...)					\
+	__drm_printk((drm), warn, _once, fmt, ##__VA_ARGS__)
+#define drm_err_once(drm, fmt, ...)					\
+	__drm_printk((drm), err, _once, "*ERROR* " fmt, ##__VA_ARGS__)
+
+#define drm_err_ratelimited(drm, fmt, ...)				\
+	__drm_printk((drm), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__)
+
+#define drm_dbg_core(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
+#define drm_dbg(drm, fmt, ...)						\
+	drm_dev_dbg((drm)->dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
+#define drm_dbg_kms(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
+#define drm_dbg_prime(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+#define drm_dbg_atomic(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
+#define drm_dbg_vbl(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
+#define drm_dbg_state(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_STATE, fmt, ##__VA_ARGS__)
+#define drm_dbg_lease(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
+#define drm_dbg_dp(drm, fmt, ...)					\
+	drm_dev_dbg((drm)->dev, DRM_UT_DP, fmt, ##__VA_ARGS__)
+
+
+/*
+ * LEGACY logging support - do not use in new code
+ */
+
 /*
  * struct device based logging
  *
@@ -496,67 +552,6 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 	_DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_PRIME,		\
 					  fmt, ##__VA_ARGS__)
 
-/*
- * struct drm_device based logging
- *
- * Prefer drm_device based logging over device or prink based logging.
- */
-
-/* Helper for struct drm_device based logging. */
-#define __drm_printk(drm, level, type, fmt, ...)			\
-	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
-
-
-#define drm_info(drm, fmt, ...)					\
-	__drm_printk((drm), info,, fmt, ##__VA_ARGS__)
-
-#define drm_notice(drm, fmt, ...)				\
-	__drm_printk((drm), notice,, fmt, ##__VA_ARGS__)
-
-#define drm_warn(drm, fmt, ...)					\
-	__drm_printk((drm), warn,, fmt, ##__VA_ARGS__)
-
-#define drm_err(drm, fmt, ...)					\
-	__drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
-
-
-#define drm_info_once(drm, fmt, ...)				\
-	__drm_printk((drm), info, _once, fmt, ##__VA_ARGS__)
-
-#define drm_notice_once(drm, fmt, ...)				\
-	__drm_printk((drm), notice, _once, fmt, ##__VA_ARGS__)
-
-#define drm_warn_once(drm, fmt, ...)				\
-	__drm_printk((drm), warn, _once, fmt, ##__VA_ARGS__)
-
-#define drm_err_once(drm, fmt, ...)				\
-	__drm_printk((drm), err, _once, "*ERROR* " fmt, ##__VA_ARGS__)
-
-
-#define drm_err_ratelimited(drm, fmt, ...)				\
-	__drm_printk((drm), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__)
-
-
-#define drm_dbg_core(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
-#define drm_dbg(drm, fmt, ...)						\
-	drm_dev_dbg((drm)->dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
-#define drm_dbg_kms(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
-#define drm_dbg_prime(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
-#define drm_dbg_atomic(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
-#define drm_dbg_vbl(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
-#define drm_dbg_state(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_STATE, fmt, ##__VA_ARGS__)
-#define drm_dbg_lease(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
-#define drm_dbg_dp(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_DP, fmt, ##__VA_ARGS__)
-
-
 /*
  * printk based logging
  *
-- 
2.20.1

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

[PATCH v1 3/8] drm/print: add new logging helper for drm logging

[Sam Ravnborg]

Add new helper so logging can use the standard logging
functions without an extra helper function.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_print.h | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index d2b9ac6a6e18..c1d333bb7534 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -403,10 +403,15 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
  * Prefer drm_device based logging over device or printk based logging.
  */
 
-/* Helper for struct drm_device based logging. */
+/* Helpers for struct drm_device based logging. */
 #define __drm_printk(drm, level, type, fmt, ...)			\
 	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
 
+#define __drm_cat_printk(drm, cat, fmt, ...)				\
+({									\
+	if (drm_debug_enabled(cat))					\
+		dev_dbg((drm)->dev, "[drm] " fmt, ##__VA_ARGS__);	\
+})
 
 #define drm_info(drm, fmt, ...)						\
 	__drm_printk((drm), info,, fmt, ##__VA_ARGS__)
@@ -430,24 +435,23 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
 	__drm_printk((drm), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__)
 
 #define drm_dbg_core(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_CORE, fmt, ##__VA_ARGS__)
 #define drm_dbg(drm, fmt, ...)						\
-	drm_dev_dbg((drm)->dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
 #define drm_dbg_kms(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_KMS, fmt, ##__VA_ARGS__)
 #define drm_dbg_prime(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_PRIME, fmt, ##__VA_ARGS__)
 #define drm_dbg_atomic(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
 #define drm_dbg_vbl(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_VBL, fmt, ##__VA_ARGS__)
 #define drm_dbg_state(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_STATE, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_STATE, fmt, ##__VA_ARGS__)
 #define drm_dbg_lease(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
+	__drm_cat_printk((drm), DRM_UT_LEASE, fmt, ##__VA_ARGS__)
 #define drm_dbg_dp(drm, fmt, ...)					\
-	drm_dev_dbg((drm)->dev, DRM_UT_DP, fmt, ##__VA_ARGS__)
-
+	__drm_cat_printk((drm), DRM_UT_DP, fmt, ##__VA_ARGS__)
 
 /*
  * LEGACY logging support - do not use in new code
-- 
2.20.1

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

[PATCH v1 4/8] drm/print: add kernel-doc for drm_debug_enabled

[Sam Ravnborg]

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_print.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index c1d333bb7534..c9fa06b517cc 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -392,6 +392,21 @@ enum drm_debug_category {
 	DRM_UT_DP		= 0x100,
 };
 
+/**
+ * drm_debug_enabled - check if debug logging is enabled for the
+ * specified category
+ * @category: the category that is checked
+ *
+ * The category specified can be either one &drm_debug_category value or several
+ * &drm_debug_category values ORed together.
+ * drm_debug_enabled() can be used to avoid expensive logging functionality
+ * when logging is not enabled.
+ * Use with care.
+ *
+ * RETURNS:
+ * true if debug logging for the specified category is enabled.
+ * false otherwise
+ */
 static inline bool drm_debug_enabled(enum drm_debug_category category)
 {
 	return unlikely(__drm_debug & category);
-- 
2.20.1

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

[PATCH v1 5/8] drm/print: rename drm_dev_dbg

[Sam Ravnborg]

Rename drm_dev_dbg to the internal name __drm_dev_dbg.
This will make room for a new logging macro with the same name.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_print.c |  6 +++---
 include/drm/drm_print.h     | 20 ++++++++++----------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 111b932cf2a9..fd70205f8c5c 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -256,8 +256,8 @@ void drm_dev_printk(const struct device *dev, const char *level,
 }
 EXPORT_SYMBOL(drm_dev_printk);
 
-void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
-		 const char *format, ...)
+void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
+		   const char *format, ...)
 {
 	struct va_format vaf;
 	va_list args;
@@ -278,7 +278,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 
 	va_end(args);
 }
-EXPORT_SYMBOL(drm_dev_dbg);
+EXPORT_SYMBOL(__drm_dev_dbg);
 
 void __drm_dbg(enum drm_debug_category category, const char *format, ...)
 {
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index c9fa06b517cc..7c0b93e6cb80 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -482,8 +482,8 @@ __printf(3, 4)
 void drm_dev_printk(const struct device *dev, const char *level,
 		    const char *format, ...);
 __printf(3, 4)
-void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
-		 const char *format, ...);
+void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
+		   const char *format, ...);
 
 /**
  * Error output.
@@ -529,19 +529,19 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
  * @fmt: printf() like format string.
  */
 #define DRM_DEV_DEBUG(dev, fmt, ...)					\
-	drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
+	__drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)				\
-	drm_dev_dbg(dev, DRM_UT_DRIVER,	fmt, ##__VA_ARGS__)
+	__drm_dev_dbg(dev, DRM_UT_DRIVER,	fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_KMS(dev, fmt, ...)				\
-	drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	__drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_PRIME(dev, fmt, ...)				\
-	drm_dev_dbg(dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+	__drm_dev_dbg(dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)				\
-	drm_dev_dbg(dev, DRM_UT_ATOMIC,	fmt, ##__VA_ARGS__)
+	__drm_dev_dbg(dev, DRM_UT_ATOMIC,	fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_VBL(dev, fmt, ...)				\
-	drm_dev_dbg(dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
+	__drm_dev_dbg(dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
 #define	DRM_DEV_DEBUG_DP(dev, fmt, ...)					\
-	drm_dev_dbg(dev, DRM_UT_DP, fmt, ## __VA_ARGS__)
+	__drm_dev_dbg(dev, DRM_UT_DP, fmt, ## __VA_ARGS__)
 
 #define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, category, fmt, ...)	\
 ({									\
@@ -549,7 +549,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 				      DEFAULT_RATELIMIT_INTERVAL,	\
 				      DEFAULT_RATELIMIT_BURST);		\
 	if (__ratelimit(&_rs))						\
-		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
+		__drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
 })
 
 /**
-- 
2.20.1

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

[PATCH v1 6/8] drm/print: add drm_dev_* logging functions

[Sam Ravnborg]

There are a lot of cases where we have a device * but no drm_device *.
Add drm_dev_* variants of the logging functions to cover these cases.

Include brief documentation.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_print.h | 99 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 7c0b93e6cb80..b2e5d0209010 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -337,7 +337,50 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
  *
  * Logging when a &device * is available, but no &drm_device *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- * TODO
+ *
+ * Adding a device pointer (if no &drm_device * is available) is always a good
+ * idea as it add more information in the logging message thus making it easier
+ * to determine the source of the logging.
+ *
+ * All logging functions in this block share the same prototype:
+ *
+ * .. code-block:: c
+ *
+ *   void drm_dev_xxx(struct device *, char * fmt, ...)
+ *
+ * The following functions are available:
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   drm_dev_dbg()
+ *   drm_dev_info()
+ *   drm_dev_notice()
+ *   drm_dev_warn()
+ *   drm_dev_err()
+ *
+ *   # Log only once
+ *   drm_dev_info_once()
+ *   drm_dev_notice_once()
+ *   drm_dev_warn_once()
+ *   drm_dev_err_once()
+ *
+ *   # Ratelimited - do not flood the logs
+ *   drm_dev_err_ratelimited()
+ *   drm_dev_dbg_ratelimited()
+ *   drm_dev_dbg_kms_ratelimited()
+ *
+ *   # Logging with a specific category
+ *   drm_dev_dbg_core()
+ *   drm_dev_dbg()		# Uses the DRIVER category
+ *   drm_dev_dbg_kms()
+ *   drm_dev_dbg_prime()
+ *   drm_dev_dbg_atomic()
+ *   drm_dev_dbg_vbl()
+ *   drm_dev_dbg_state()
+ *   drm_dev_dbg_lease()
+ *   drm_dev_dbg_dp()
+ *
  *
  * Logging when no &device * nor &drm_device * is available
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -468,6 +511,60 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
 #define drm_dbg_dp(drm, fmt, ...)					\
 	__drm_cat_printk((drm), DRM_UT_DP, fmt, ##__VA_ARGS__)
 
+/* struct device based logging. */
+#define __drm_dev_printk(dev, level, type, fmt, ...)			\
+	dev_##level##type(dev, "[drm] " fmt, ##__VA_ARGS__)
+
+#define __drm_dev_cat_printk(dev, cat, type, fmt, ...)			\
+({									\
+	if (drm_debug_enabled(cat))					\
+		dev_dbg##type((dev), "[drm] " fmt, ##__VA_ARGS__);	\
+})
+
+#define drm_dev_info(dev, fmt, ...)					\
+	__drm_dev_printk((dev), info,, fmt, ##__VA_ARGS__)
+#define drm_dev_notice(dev, fmt, ...)					\
+	__drm_dev_printk((dev), notice,, fmt, ##__VA_ARGS__)
+#define drm_dev_warn(dev, fmt, ...)					\
+	__drm_dev_printk((dev), warn,, fmt, ##__VA_ARGS__)
+#define drm_dev_err(dev, fmt, ...)					\
+	__drm_dev_printk((dev), err,, "*ERROR* " fmt, ##__VA_ARGS__)
+
+#define drm_dev_info_once(dev, fmt, ...)				\
+	__drm_dev_printk((dev), info, _once, fmt, ##__VA_ARGS__)
+#define drm_dev_notice_once(dev, fmt, ...)				\
+	__drm_dev_printk((dev), notice, _once, fmt, ##__VA_ARGS__)
+#define drm_dev_warn_once(dev, fmt, ...)				\
+	__drm_dev_printk((dev), warn, _once, fmt, ##__VA_ARGS__)
+#define drm_dev_err_once(dev, fmt, ...)					\
+	__drm_dev_printk((dev), err, _once, "*ERROR* " fmt, ##__VA_ARGS__)
+
+#define drm_dev_err_ratelimited(dev, fmt, ...)				\
+	__drm_dev_printk((dev), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_ratelimited(dev, fmt, ...)				\
+	__drm_dev_cat_printk((dev), DRM_UT_DRIVER,_ratelimited, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_kms_ratelimited(dev, fmt, ...)			\
+	__drm_dev_cat_printk((dev), DRM_UT_KMS,_ratelimited, fmt, ##__VA_ARGS__)
+
+#define drm_dev_dbg_core(dev, fmt, ...)					\
+	__drm_dev_cat_printk((dev), DRM_UT_CORE,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg(dev, fmt, ...)					\
+	__drm_dev_cat_printk((dev), DRM_UT_DRIVER,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_kms(dev, fmt, ...)					\
+	__drm_dev_cat_printk((dev), DRM_UT_KMS,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_prime(dev, fmt, ...)				\
+	__drm_dev_cat_printk((dev), DRM_UT_PRIME,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_atomic(dev, fmt, ...)				\
+	__drm_dev_cat_printk((dev), DRM_UT_ATOMIC,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_vbl(dev, fmt, ...)					\
+	__drm_dev_cat_printk((dev), DRM_UT_VBL,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_state(dev, fmt, ...)				\
+	__drm_dev_cat_printk((dev), DRM_UT_STATE,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_lease(dev, fmt, ...)				\
+	__drm_dev_cat_printk((dev), DRM_UT_LEASE,, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg_dp(dev, fmt, ...)					\
+	__drm_dev_cat_printk((dev), DRM_UT_DP,, fmt, ##__VA_ARGS__)
+
 /*
  * LEGACY logging support - do not use in new code
  */
-- 
2.20.1

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

[PATCH v1 7/8] drm/print: add drm_pr_ logging

[Sam Ravnborg]

Add standard logging functions that can be used when
no struct device *, nor struct drm_device * is available.

Include brief documentation.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 include/drm/drm_print.h | 88 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index b2e5d0209010..0b0468340573 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -384,7 +384,46 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
  *
  * Logging when no &device * nor &drm_device * is available
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- * TODO
+ *
+ * When there is neither a struct &drm_device *, nor a struct device * use
+ * the logging variants that does not take a pointer.
+ * It is important to note that the logging core cannot provide much extra
+ * info but as the logging has a fixed *[drm]* prefix then the logging continue
+ * to be easy to spot.
+ *
+ * All logging functions in this block share the same prototype:
+ *
+ * .. code-block:: c
+ *
+ *   void drm_pr_xxx(char * fmt, ...)
+ *
+ * The following functions are available:
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   drm_pr_dbg()
+ *   drm_pr_info()
+ *   drm_pr_notice()
+ *   drm_pr_warn()
+ *   drm_pr_err()
+ *
+ *   # Log only once
+ *   drm_pr_info_once()
+ *   drm_pr_notice_once()
+ *   drm_pr_warn_once()
+ *   drm_pr_err_once()
+ *
+ *   # Logging with a specific category
+ *   drm_pr_dbg_core()
+ *   drm_pr_dbg()		# Uses the DRIVER category
+ *   drm_pr_dbg_kms()
+ *   drm_pr_dbg_prime()
+ *   drm_pr_dbg_atomic()
+ *   drm_pr_dbg_vbl()
+ *   drm_pr_dbg_state()
+ *   drm_pr_dbg_lease()
+ *   drm_pr_dbg_dp()
  *
  * Obsoleted logging functions
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -565,6 +604,53 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
 #define drm_dev_dbg_dp(dev, fmt, ...)					\
 	__drm_dev_cat_printk((dev), DRM_UT_DP,, fmt, ##__VA_ARGS__)
 
+/* logging with no struct device * and no struct drm_device * */
+#define __drm_pr_printk(level, type, fmt, ...)				\
+	pr_##level##type("[drm] " fmt, ##__VA_ARGS__)
+
+#define __drm_pr_cat_printk(cat, type, fmt, ...)			\
+({									\
+	if (drm_debug_enabled(cat))					\
+		pr_debug##type("[drm] " fmt, ##__VA_ARGS__);		\
+})
+
+#define drm_pr_info(fmt, ...)						\
+	__drm_pr_printk(info,, fmt, ##__VA_ARGS__)
+#define drm_pr_notice(fmt, ...)						\
+	__drm_pr_printk(notice,, fmt, ##__VA_ARGS__)
+#define drm_pr_warn(fmt, ...)						\
+	__drm_pr_printk(warn,, fmt, ##__VA_ARGS__)
+#define drm_pr_err(fmt, ...)						\
+	__drm_pr_printk(err,, "*ERROR* " fmt, ##__VA_ARGS__)
+
+#define drm_pr_info_once(fmt, ...)					\
+	__drm_pr_printk(info, _once, fmt, ##__VA_ARGS__)
+#define drm_pr_notice_once(fmt, ...)					\
+	__drm_pr_printk(notice, _once, fmt, ##__VA_ARGS__)
+#define drm_pr_warn_once(fmt, ...)					\
+	__drm_pr_printk(warn, _once, fmt, ##__VA_ARGS__)
+#define drm_pr_err_once(fmt, ...)					\
+	__drm_pr_printk(err, _once, "*ERROR* " fmt, ##__VA_ARGS__)
+
+#define drm_pr_dbg_core(fmt, ...)					\
+	__drm_pr_cat_printk(DRM_UT_CORE,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg(fmt, ...)						\
+	__drm_pr_cat_printk(DRM_UT_DRIVER,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg_kms(fmt, ...)					\
+	__drm_pr_cat_printk(DRM_UT_KMS,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg_prime(fmt, ...)					\
+	__drm_pr_cat_printk(DRM_UT_PRIME,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg_atomic(fmt, ...)					\
+	__drm_pr_cat_printk(DRM_UT_ATOMIC,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg_vbl(fmt, ...)					\
+	__drm_pr_cat_printk(DRM_UT_VBL,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg_state(fmt, ...)					\
+	__drm_pr_cat_printk(DRM_UT_STATE,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg_lease(fmt, ...)					\
+	__drm_pr_cat_printk(DRM_UT_LEASE,, fmt, ##__VA_ARGS__)
+#define drm_pr_dbg_dp(fmt, ...)						\
+	__drm_pr_cat_printk(DRM_UT_DP,, fmt, ##__VA_ARGS__)
+
 /*
  * LEGACY logging support - do not use in new code
  */
-- 
2.20.1

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

[PATCH v1 8/8] drm/print: let legacy logging use new style functions

[Sam Ravnborg]

Update the legacy logging functions to use the new style logging
functions.
This will help people when transition to the new style as they can just
go and see what new style logging function to use.

This also makes logging look a bit more consistent as the same base is
now used for logging.

While converting, drop the unused ratelimited variants.

Delete the now unused logging support functions in drm_print.c

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Joe Perches <joe@perches.com>
---
 drivers/gpu/drm/drm_print.c |  80 ----------------
 include/drm/drm_print.h     | 184 ++++++------------------------------
 2 files changed, 31 insertions(+), 233 deletions(-)

diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index fd70205f8c5c..e5a3d2576d6f 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -235,86 +235,6 @@ void drm_print_bits(struct drm_printer *p, unsigned long value,
 }
 EXPORT_SYMBOL(drm_print_bits);
 
-void drm_dev_printk(const struct device *dev, const char *level,
-		    const char *format, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	va_start(args, format);
-	vaf.fmt = format;
-	vaf.va = &args;
-
-	if (dev)
-		dev_printk(level, dev, "[" DRM_NAME ":%ps] %pV",
-			   __builtin_return_address(0), &vaf);
-	else
-		printk("%s" "[" DRM_NAME ":%ps] %pV",
-		       level, __builtin_return_address(0), &vaf);
-
-	va_end(args);
-}
-EXPORT_SYMBOL(drm_dev_printk);
-
-void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
-		   const char *format, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	if (!drm_debug_enabled(category))
-		return;
-
-	va_start(args, format);
-	vaf.fmt = format;
-	vaf.va = &args;
-
-	if (dev)
-		dev_printk(KERN_DEBUG, dev, "[" DRM_NAME ":%ps] %pV",
-			   __builtin_return_address(0), &vaf);
-	else
-		printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
-		       __builtin_return_address(0), &vaf);
-
-	va_end(args);
-}
-EXPORT_SYMBOL(__drm_dev_dbg);
-
-void __drm_dbg(enum drm_debug_category category, const char *format, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	if (!drm_debug_enabled(category))
-		return;
-
-	va_start(args, format);
-	vaf.fmt = format;
-	vaf.va = &args;
-
-	printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
-	       __builtin_return_address(0), &vaf);
-
-	va_end(args);
-}
-EXPORT_SYMBOL(__drm_dbg);
-
-void __drm_err(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);
-
 /**
  * drm_print_regset32 - print the contents of registers to a
  * &drm_printer stream.
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 0b0468340573..f305454544ee 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -655,176 +655,54 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
  * LEGACY logging support - do not use in new code
  */
 
-/*
- * struct device based logging
- *
- * Prefer drm_device based logging over device or prink based logging.
- */
-
-__printf(3, 4)
-void drm_dev_printk(const struct device *dev, const char *level,
-		    const char *format, ...);
-__printf(3, 4)
-void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
-		   const char *format, ...);
+#define DRM_DEV_ERROR(dev, fmt, ...)		\
+	drm_dev_err((dev), fmt, ##__VA_ARGS__)
+#define DRM_DEV_INFO(dev, fmt, ...)		\
+	drm_dev_info((dev), fmt, ##__VA_ARGS__)
+#define DRM_DEV_INFO_ONCE(dev, fmt, ...)	\
+	drm_dev_info_once((dev), fmt, ##__VA_ARGS__)
 
-/**
- * Error output.
- *
- * @dev: device pointer
- * @fmt: printf() like format string.
- */
-#define DRM_DEV_ERROR(dev, fmt, ...)					\
-	drm_dev_printk(dev, KERN_ERR, "*ERROR* " fmt, ##__VA_ARGS__)
-
-/**
- * Rate limited error output.  Like DRM_ERROR() but won't flood the log.
- *
- * @dev: device pointer
- * @fmt: printf() like format string.
- */
-#define DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)			\
-({									\
-	static DEFINE_RATELIMIT_STATE(_rs,				\
-				      DEFAULT_RATELIMIT_INTERVAL,	\
-				      DEFAULT_RATELIMIT_BURST);		\
-									\
-	if (__ratelimit(&_rs))						\
-		DRM_DEV_ERROR(dev, fmt, ##__VA_ARGS__);			\
-})
-
-#define DRM_DEV_INFO(dev, fmt, ...)				\
-	drm_dev_printk(dev, KERN_INFO, 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__);			\
-	}								\
-})
-
-/**
- * Debug output.
- *
- * @dev: device pointer
- * @fmt: printf() like format string.
- */
 #define DRM_DEV_DEBUG(dev, fmt, ...)					\
-	__drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
+	drm_dev_dbg_core((dev), fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)				\
-	__drm_dev_dbg(dev, DRM_UT_DRIVER,	fmt, ##__VA_ARGS__)
+	drm_dev_dbg((dev), fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_KMS(dev, fmt, ...)				\
-	__drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	drm_dev_dbg_kms((dev), fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_PRIME(dev, fmt, ...)				\
-	__drm_dev_dbg(dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+	drm_dev_dbg_prime((dev), fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)				\
-	__drm_dev_dbg(dev, DRM_UT_ATOMIC,	fmt, ##__VA_ARGS__)
+	drm_dev_dbg_atomic((dev), fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_VBL(dev, fmt, ...)				\
-	__drm_dev_dbg(dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
+	drm_dev_dbg_vbl((dev), fmt, ##__VA_ARGS__)
 #define	DRM_DEV_DEBUG_DP(dev, fmt, ...)					\
-	__drm_dev_dbg(dev, DRM_UT_DP, fmt, ## __VA_ARGS__)
-
-#define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, category, fmt, ...)	\
-({									\
-	static DEFINE_RATELIMIT_STATE(_rs,				\
-				      DEFAULT_RATELIMIT_INTERVAL,	\
-				      DEFAULT_RATELIMIT_BURST);		\
-	if (__ratelimit(&_rs))						\
-		__drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
-})
+	drm_dev_dbg_dp((dev), fmt, ## __VA_ARGS__)
 
-/**
- * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
- *
- * @dev: device pointer
- * @fmt: printf() like format string.
- */
-#define DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)			\
-	_DEV_DRM_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_CORE,		\
-					  fmt, ##__VA_ARGS__)
-#define DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)			\
-	_DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_DRIVER,		\
-					  fmt, ##__VA_ARGS__)
+#define DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)			\
+	drm_dev_err_ratelimited((dev), fmt, ##__VA_ARGS__)
 #define DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)			\
-	_DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_KMS,		\
-					  fmt, ##__VA_ARGS__)
-#define DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)			\
-	_DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_PRIME,		\
-					  fmt, ##__VA_ARGS__)
-
-/*
- * printk based logging
- *
- * Prefer drm_device based logging over device or prink based logging.
- */
-
-__printf(2, 3)
-void __drm_dbg(enum drm_debug_category category, const char *format, ...);
-__printf(1, 2)
-void __drm_err(const char *format, ...);
-
-/* Macros to make printk easier */
-
-#define _DRM_PRINTK(once, level, fmt, ...)				\
-	printk##once(KERN_##level "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
-
-#define DRM_INFO(fmt, ...)						\
-	_DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE(fmt, ...)						\
-	_DRM_PRINTK(, NOTICE, fmt, ##__VA_ARGS__)
-#define DRM_WARN(fmt, ...)						\
-	_DRM_PRINTK(, WARNING, fmt, ##__VA_ARGS__)
+	drm_dev_dbg_kms_ratelimited((dev), fmt, ##__VA_ARGS__)
 
-#define DRM_INFO_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, INFO, fmt, ##__VA_ARGS__)
-#define DRM_NOTE_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, NOTICE, fmt, ##__VA_ARGS__)
-#define DRM_WARN_ONCE(fmt, ...)						\
-	_DRM_PRINTK(_once, WARNING, fmt, ##__VA_ARGS__)
+#define DRM_INFO(fmt, ...) drm_pr_info(fmt, ##__VA_ARGS__)
+#define DRM_NOTE(fmt, ...) drm_pr_notice(fmt, ##__VA_ARGS__)
+#define DRM_WARN(fmt, ...) drm_pr_warn(fmt, ##__VA_ARGS__)
 
-#define DRM_ERROR(fmt, ...)						\
-	__drm_err(fmt, ##__VA_ARGS__)
+#define DRM_INFO_ONCE(fmt, ...) drm_pr_info_once(fmt, ##__VA_ARGS__)
+#define DRM_NOTE_ONCE(fmt, ...) drm_pr_notice_once(fmt, ##__VA_ARGS__)
+#define DRM_WARN_ONCE(fmt, ...) drm_pr_warn_once(fmt, ##__VA_ARGS__)
+#define DRM_ERROR(fmt, ...)     drm_pr_err(fmt, ##__VA_ARGS__)
 
 #define DRM_ERROR_RATELIMITED(fmt, ...)					\
 	DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG(fmt, ...)						\
-	__drm_dbg(DRM_UT_CORE, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_DRIVER(fmt, ...)					\
-	__drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_KMS(fmt, ...)						\
-	__drm_dbg(DRM_UT_KMS, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_PRIME(fmt, ...)					\
-	__drm_dbg(DRM_UT_PRIME, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_ATOMIC(fmt, ...)					\
-	__drm_dbg(DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_VBL(fmt, ...)						\
-	__drm_dbg(DRM_UT_VBL, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_LEASE(fmt, ...)					\
-	__drm_dbg(DRM_UT_LEASE, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_DP(fmt, ...)						\
-	__drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
-
-
-#define DRM_DEBUG_RATELIMITED(fmt, ...)					\
-	DRM_DEV_DEBUG_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
-
-#define DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)				\
-	DRM_DEV_DEBUG_DRIVER_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
-
 #define DRM_DEBUG_KMS_RATELIMITED(fmt, ...)				\
 	DRM_DEV_DEBUG_KMS_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
 
-#define DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)				\
-	DRM_DEV_DEBUG_PRIME_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
+#define DRM_DEBUG(fmt, ...)        drm_pr_dbg_core(fmt, ##__VA_ARGS__)
+#define DRM_DEBUG_DRIVER(fmt, ...) drm_pr_dbg(fmt, ##__VA_ARGS__)
+#define DRM_DEBUG_KMS(fmt, ...)    drm_pr_dbg_kms(fmt, ##__VA_ARGS__)
+#define DRM_DEBUG_PRIME(fmt, ...)  drm_pr_dbg_prime(fmt, ##__VA_ARGS__)
+#define DRM_DEBUG_ATOMIC(fmt, ...) drm_pr_dbg_atomic(fmt, ##__VA_ARGS__)
+#define DRM_DEBUG_VBL(fmt, ...)    drm_pr_dbg_vbl(fmt, ##__VA_ARGS__)
+#define DRM_DEBUG_LEASE(fmt, ...)  drm_pr_dbg_lease(fmt, ##__VA_ARGS__)
+#define DRM_DEBUG_DP(fmt, ...)     drm_pr_dbg_dp(fmt, ##__VA_ARGS__)
 
 #endif /* DRM_PRINT_H_ */
-- 
2.20.1

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

Re: [PATCH v1 6/8] drm/print: add drm_dev_* logging functions

[Joe Perches]

On Sat, 2019-12-21 at 10:55 +0100, Sam Ravnborg wrote:
> There are a lot of cases where we have a device * but no drm_device *.
> Add drm_dev_* variants of the logging functions to cover these cases.
[]
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
[]
> @@ -468,6 +511,60 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
>  #define drm_dbg_dp(drm, fmt, ...)					\
>  	__drm_cat_printk((drm), DRM_UT_DP, fmt, ##__VA_ARGS__)
>  
> +/* struct device based logging. */
> +#define __drm_dev_printk(dev, level, type, fmt, ...)			\
> +	dev_##level##type(dev, "[drm] " fmt, ##__VA_ARGS__)
> +
> +#define __drm_dev_cat_printk(dev, cat, type, fmt, ...)			\
> +({									\
> +	if (drm_debug_enabled(cat))					\
> +		dev_dbg##type((dev), "[drm] " fmt, ##__VA_ARGS__);	\

trivia:  The parentheses around dev aren't necessary.

> +})
> +
> +#define drm_dev_info(dev, fmt, ...)					\
> +	__drm_dev_printk((dev), info,, fmt, ##__VA_ARGS__)

etc...


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

Re: [PATCH v1 6/8] drm/print: add drm_dev_* logging functions

[Sam Ravnborg]

Hi Joe.

> > +#define __drm_dev_cat_printk(dev, cat, type, fmt, ...)			\
> > +({									\
> > +	if (drm_debug_enabled(cat))					\
> > +		dev_dbg##type((dev), "[drm] " fmt, ##__VA_ARGS__);	\
> 
> trivia:  The parentheses around dev aren't necessary.
> 
> > +})
> > +
> > +#define drm_dev_info(dev, fmt, ...)					\
> > +	__drm_dev_printk((dev), info,, fmt, ##__VA_ARGS__)
> 
> etc...

I was not really sure so I just added them.
Will remove in v2 in all relevent patches - thanks!

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

Re: [PATCH v1 3/8] drm/print: add new logging helper for drm logging

[Jani Nikula]

On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> Add new helper so logging can use the standard logging
> functions without an extra helper function.

The main functional change here is that this will no longer print the
function names in the debug logs. I am not sure if we want to make that
change.

BR,
Jani.

>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/drm_print.h | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index d2b9ac6a6e18..c1d333bb7534 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -403,10 +403,15 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
>   * Prefer drm_device based logging over device or printk based logging.
>   */
>  
> -/* Helper for struct drm_device based logging. */
> +/* Helpers for struct drm_device based logging. */
>  #define __drm_printk(drm, level, type, fmt, ...)			\
>  	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
>  
> +#define __drm_cat_printk(drm, cat, fmt, ...)				\
> +({									\
> +	if (drm_debug_enabled(cat))					\
> +		dev_dbg((drm)->dev, "[drm] " fmt, ##__VA_ARGS__);	\
> +})
>  
>  #define drm_info(drm, fmt, ...)						\
>  	__drm_printk((drm), info,, fmt, ##__VA_ARGS__)
> @@ -430,24 +435,23 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
>  	__drm_printk((drm), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__)
>  
>  #define drm_dbg_core(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_CORE, fmt, ##__VA_ARGS__)
>  #define drm_dbg(drm, fmt, ...)						\
> -	drm_dev_dbg((drm)->dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
>  #define drm_dbg_kms(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_KMS, fmt, ##__VA_ARGS__)
>  #define drm_dbg_prime(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_PRIME, fmt, ##__VA_ARGS__)
>  #define drm_dbg_atomic(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
>  #define drm_dbg_vbl(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_VBL, fmt, ##__VA_ARGS__)
>  #define drm_dbg_state(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_STATE, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_STATE, fmt, ##__VA_ARGS__)
>  #define drm_dbg_lease(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
> +	__drm_cat_printk((drm), DRM_UT_LEASE, fmt, ##__VA_ARGS__)
>  #define drm_dbg_dp(drm, fmt, ...)					\
> -	drm_dev_dbg((drm)->dev, DRM_UT_DP, fmt, ##__VA_ARGS__)
> -
> +	__drm_cat_printk((drm), DRM_UT_DP, fmt, ##__VA_ARGS__)
>  
>  /*
>   * LEGACY logging support - do not use in new code

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 3/8] drm/print: add new logging helper for drm logging

[Joe Perches]

On Mon, 2019-12-23 at 13:16 +0200, Jani Nikula wrote:
> On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> > Add new helper so logging can use the standard logging
> > functions without an extra helper function.
> 
> The main functional change here is that this will no longer print the
> function names in the debug logs. I am not sure if we want to make that
> change.

It will also increase overall code size


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

Re: [PATCH v1 4/8] drm/print: add kernel-doc for drm_debug_enabled

[Jani Nikula]

On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/drm_print.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index c1d333bb7534..c9fa06b517cc 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -392,6 +392,21 @@ enum drm_debug_category {
>  	DRM_UT_DP		= 0x100,
>  };
>  
> +/**
> + * drm_debug_enabled - check if debug logging is enabled for the
> + * specified category
> + * @category: the category that is checked
> + *
> + * The category specified can be either one &drm_debug_category value or several
> + * &drm_debug_category values ORed together.

It can implementation wise, but I'd prefer to advertize it as a plain
enum for just one category in the interface.

BR,
Jani.

> + * drm_debug_enabled() can be used to avoid expensive logging functionality
> + * when logging is not enabled.
> + * Use with care.
> + *
> + * RETURNS:
> + * true if debug logging for the specified category is enabled.
> + * false otherwise
> + */
>  static inline bool drm_debug_enabled(enum drm_debug_category category)
>  {
>  	return unlikely(__drm_debug & category);

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 6/8] drm/print: add drm_dev_* logging functions

[Jani Nikula]

On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Joe.
>
>> > +#define __drm_dev_cat_printk(dev, cat, type, fmt, ...)			\
>> > +({									\
>> > +	if (drm_debug_enabled(cat))					\
>> > +		dev_dbg##type((dev), "[drm] " fmt, ##__VA_ARGS__);	\
>> 
>> trivia:  The parentheses around dev aren't necessary.
>> 
>> > +})
>> > +
>> > +#define drm_dev_info(dev, fmt, ...)					\
>> > +	__drm_dev_printk((dev), info,, fmt, ##__VA_ARGS__)
>> 
>> etc...
>
> I was not really sure so I just added them.
> Will remove in v2 in all relevent patches - thanks!

FWIW, they are necessary in the drm_device variants due to the macros
doing the dereferencing.

BR,
Jani.


>
> 	Sam

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 1/8] drm/print: document logging functions

[Jani Nikula]

On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> This is the documentation I have missed when I looked for help
> how to do proper logging. Hopefully it can help others.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  Documentation/gpu/drm-internals.rst |  6 ++
>  include/drm/drm_print.h             | 91 ++++++++++++++++++++++++++---
>  2 files changed, 90 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index a73320576ca9..c2093611999c 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -164,6 +164,12 @@ File Operations
>  Misc Utilities
>  ==============
>  
> +Logging
> +-------
> +
> +.. kernel-doc:: include/drm/drm_print.h
> +   :doc: logging
> +
>  Printer
>  -------
>  
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 8f99d389792d..e9e31ace0afa 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -250,22 +250,42 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
>  }
>  
>  /**
> - * enum drm_debug_category - The DRM debug categories
> + * DOC: logging
> + *
> + * There is a set of functions/macros available used for logging
> + * in the DRM subsystem.
> + * Using the drm logging function enables that the logging is consistently
> + * prefixed with *[drm]* thus the logging is easy to recognize.
> + *
> + * Example of logging with *[drm]* prefix::
>   *
> - * Each of the DRM debug logging macros use a specific category, and the logging
> - * is filtered by the drm.debug module parameter. This enum specifies the values
> - * for the interface.
> + *   [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> + *   [drm] Driver supports precise vblank timestamp query.
>   *
> - * Each DRM_DEBUG_<CATEGORY> macro logs to DRM_UT_<CATEGORY> category, except
> - * DRM_DEBUG() logs to DRM_UT_CORE.
> + *
> + * Each of the debug logging macros use a specific category, and the logging
> + * is filtered by the drm.debug module parameter. The &drm_debug_category enum
> + * specifies the values for the interface.
> + *
> + * Each drm_dbg_<category> macro logs to a DRM_UT_<category> category,
> + * except drm_dbg() that logs to DRM_UT_DRIVER.
>   *
>   * Enabling verbose debug messages is done through the drm.debug parameter, each
>   * category being enabled by a bit:
>   *
>   *  - drm.debug=0x1 will enable CORE messages
>   *  - drm.debug=0x2 will enable DRIVER messages
> + *  - drm.debug=0x4 will enable KMS messages
> + *  - drm.debug=0x8 will enable PRIME messages
> + *  - drm.debug=0x10 will enable ATOMIC messages
> + *  - drm.debug=0x20 will enable VBL messages
> + *  - drm.debug=0x40 will enable STATE messages
> + *  - drm.debug=0x80 will enable LEASE messages
> + *  - drm.debug=0x100 will enable DP messages

Maybe document this stuff in enum drm_debug_category where they're
defined instead?

BR,
Jani.

> + *
> + * To enable more than one category OR the values - examples:
> + *
>   *  - drm.debug=0x3 will enable CORE and DRIVER messages
> - *  - ...
>   *  - drm.debug=0x1ff will enable all messages
>   *
>   * An interesting feature is that it's possible to enable verbose logging at
> @@ -273,6 +293,63 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
>   *
>   *   # echo 0xf > /sys/module/drm/parameters/debug
>   *
> + *
> + * When a &drm_device * is available use one of the following logging functions.
> + * The same prototype is shared by all the logging functions
> + * that take a &drm_device * as first argument:
> + *
> + * .. code-block:: c
> + *
> + *   void drm_xxx(struct drm_device *, char * fmt, ...)
> + *
> + * Drivers can use the following functions for logging.
> + *
> + * .. code-block:: none
> + *
> + *   # Plain logging
> + *   drm_dbg()
> + *   drm_info()
> + *   drm_notice()
> + *   drm_warn()
> + *   drm_err()
> + *
> + *   # Log only once
> + *   drm_info_once()
> + *   drm_notice_once()
> + *   drm_warn_once()
> + *   drm_err_once()
> + *
> + *   # Ratelimited - do not flood the logs
> + *   drm_err_ratelimited()
> + *
> + *   # Logging with a specific category
> + *   drm_dbg_core()
> + *   drm_dbg()		# Uses the DRIVER category
> + *   drm_dbg_kms()
> + *   drm_dbg_prime()
> + *   drm_dbg_atomic()
> + *   drm_dbg_vbl()
> + *   drm_dbg_state()
> + *   drm_dbg_lease()
> + *   drm_dbg_dp()
> + *
> + * See enum &drm_debug_category for a description of the categories.
> + *
> + * Logging when a &device * is available, but no &drm_device *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + * TODO
> + *
> + * Logging when no &device * nor &drm_device * is available
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + * TODO
> + *
> + * Obsoleted logging functions
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + * The DRM_*() logging functions are deprecated - do not use them in new code.
> + */
> +
> +/**
> + * enum drm_debug_category - The DRM debug categories
>   */
>  enum drm_debug_category {
>  	/**

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 6/8] drm/print: add drm_dev_* logging functions

[Jani Nikula]

On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> There are a lot of cases where we have a device * but no drm_device *.
> Add drm_dev_* variants of the logging functions to cover these cases.

So I know there are some valid cases where we only have struct device *,
and instead of passing struct drm_device * will need the distinction
between multiple struct device *.

Not all current uses of DRM_DEV_* meet that criteria, however. I think
I'd like to have those converted over to the drm_device based logging
first, and then see what's left. Because I fear adding these will just
lead to mass conversion from DRM_DEV_* to drm_dev_*, and the ball gets
dropped there.

I feel a bit similar about the drm_pr_* logging functions. I want to
promote switching to drm_device based logging, not switching to the same
old thing with just new names.

BR,
Jani.


>
> Include brief documentation.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  include/drm/drm_print.h | 99 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 7c0b93e6cb80..b2e5d0209010 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -337,7 +337,50 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
>   *
>   * Logging when a &device * is available, but no &drm_device *
>   * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> - * TODO
> + *
> + * Adding a device pointer (if no &drm_device * is available) is always a good
> + * idea as it add more information in the logging message thus making it easier
> + * to determine the source of the logging.
> + *
> + * All logging functions in this block share the same prototype:
> + *
> + * .. code-block:: c
> + *
> + *   void drm_dev_xxx(struct device *, char * fmt, ...)
> + *
> + * The following functions are available:
> + *
> + * .. code-block:: none
> + *
> + *   # Plain logging
> + *   drm_dev_dbg()
> + *   drm_dev_info()
> + *   drm_dev_notice()
> + *   drm_dev_warn()
> + *   drm_dev_err()
> + *
> + *   # Log only once
> + *   drm_dev_info_once()
> + *   drm_dev_notice_once()
> + *   drm_dev_warn_once()
> + *   drm_dev_err_once()
> + *
> + *   # Ratelimited - do not flood the logs
> + *   drm_dev_err_ratelimited()
> + *   drm_dev_dbg_ratelimited()
> + *   drm_dev_dbg_kms_ratelimited()
> + *
> + *   # Logging with a specific category
> + *   drm_dev_dbg_core()
> + *   drm_dev_dbg()		# Uses the DRIVER category
> + *   drm_dev_dbg_kms()
> + *   drm_dev_dbg_prime()
> + *   drm_dev_dbg_atomic()
> + *   drm_dev_dbg_vbl()
> + *   drm_dev_dbg_state()
> + *   drm_dev_dbg_lease()
> + *   drm_dev_dbg_dp()
> + *
>   *
>   * Logging when no &device * nor &drm_device * is available
>   * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> @@ -468,6 +511,60 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
>  #define drm_dbg_dp(drm, fmt, ...)					\
>  	__drm_cat_printk((drm), DRM_UT_DP, fmt, ##__VA_ARGS__)
>  
> +/* struct device based logging. */
> +#define __drm_dev_printk(dev, level, type, fmt, ...)			\
> +	dev_##level##type(dev, "[drm] " fmt, ##__VA_ARGS__)
> +
> +#define __drm_dev_cat_printk(dev, cat, type, fmt, ...)			\
> +({									\
> +	if (drm_debug_enabled(cat))					\
> +		dev_dbg##type((dev), "[drm] " fmt, ##__VA_ARGS__);	\
> +})
> +
> +#define drm_dev_info(dev, fmt, ...)					\
> +	__drm_dev_printk((dev), info,, fmt, ##__VA_ARGS__)
> +#define drm_dev_notice(dev, fmt, ...)					\
> +	__drm_dev_printk((dev), notice,, fmt, ##__VA_ARGS__)
> +#define drm_dev_warn(dev, fmt, ...)					\
> +	__drm_dev_printk((dev), warn,, fmt, ##__VA_ARGS__)
> +#define drm_dev_err(dev, fmt, ...)					\
> +	__drm_dev_printk((dev), err,, "*ERROR* " fmt, ##__VA_ARGS__)
> +
> +#define drm_dev_info_once(dev, fmt, ...)				\
> +	__drm_dev_printk((dev), info, _once, fmt, ##__VA_ARGS__)
> +#define drm_dev_notice_once(dev, fmt, ...)				\
> +	__drm_dev_printk((dev), notice, _once, fmt, ##__VA_ARGS__)
> +#define drm_dev_warn_once(dev, fmt, ...)				\
> +	__drm_dev_printk((dev), warn, _once, fmt, ##__VA_ARGS__)
> +#define drm_dev_err_once(dev, fmt, ...)					\
> +	__drm_dev_printk((dev), err, _once, "*ERROR* " fmt, ##__VA_ARGS__)
> +
> +#define drm_dev_err_ratelimited(dev, fmt, ...)				\
> +	__drm_dev_printk((dev), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_ratelimited(dev, fmt, ...)				\
> +	__drm_dev_cat_printk((dev), DRM_UT_DRIVER,_ratelimited, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_kms_ratelimited(dev, fmt, ...)			\
> +	__drm_dev_cat_printk((dev), DRM_UT_KMS,_ratelimited, fmt, ##__VA_ARGS__)
> +
> +#define drm_dev_dbg_core(dev, fmt, ...)					\
> +	__drm_dev_cat_printk((dev), DRM_UT_CORE,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg(dev, fmt, ...)					\
> +	__drm_dev_cat_printk((dev), DRM_UT_DRIVER,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_kms(dev, fmt, ...)					\
> +	__drm_dev_cat_printk((dev), DRM_UT_KMS,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_prime(dev, fmt, ...)				\
> +	__drm_dev_cat_printk((dev), DRM_UT_PRIME,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_atomic(dev, fmt, ...)				\
> +	__drm_dev_cat_printk((dev), DRM_UT_ATOMIC,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_vbl(dev, fmt, ...)					\
> +	__drm_dev_cat_printk((dev), DRM_UT_VBL,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_state(dev, fmt, ...)				\
> +	__drm_dev_cat_printk((dev), DRM_UT_STATE,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_lease(dev, fmt, ...)				\
> +	__drm_dev_cat_printk((dev), DRM_UT_LEASE,, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg_dp(dev, fmt, ...)					\
> +	__drm_dev_cat_printk((dev), DRM_UT_DP,, fmt, ##__VA_ARGS__)
> +
>  /*
>   * LEGACY logging support - do not use in new code
>   */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 1/8] drm/print: document logging functions

[Sam Ravnborg]

Hi Jani.

> > + *
> > + * Each of the debug logging macros use a specific category, and the logging
> > + * is filtered by the drm.debug module parameter. The &drm_debug_category enum
> > + * specifies the values for the interface.
> > + *
> > + * Each drm_dbg_<category> macro logs to a DRM_UT_<category> category,
> > + * except drm_dbg() that logs to DRM_UT_DRIVER.
> >   *
> >   * Enabling verbose debug messages is done through the drm.debug parameter, each
> >   * category being enabled by a bit:
> >   *
> >   *  - drm.debug=0x1 will enable CORE messages
> >   *  - drm.debug=0x2 will enable DRIVER messages
> > + *  - drm.debug=0x4 will enable KMS messages
> > + *  - drm.debug=0x8 will enable PRIME messages
> > + *  - drm.debug=0x10 will enable ATOMIC messages
> > + *  - drm.debug=0x20 will enable VBL messages
> > + *  - drm.debug=0x40 will enable STATE messages
> > + *  - drm.debug=0x80 will enable LEASE messages
> > + *  - drm.debug=0x100 will enable DP messages
> 
> Maybe document this stuff in enum drm_debug_category where they're
> defined instead?

For the logging user it is much more convinient to have the logging
filtering explained in one place.
The enum already tell part of the story but then the reader needs to
hunt for the information.

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

Re: [PATCH v1 3/8] drm/print: add new logging helper for drm logging

[Sam Ravnborg]

Hi Jani.

On Mon, Dec 23, 2019 at 01:16:01PM +0200, Jani Nikula wrote:
> On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> > Add new helper so logging can use the standard logging
> > functions without an extra helper function.
> 
> The main functional change here is that this will no longer print the
> function names in the debug logs. I am not sure if we want to make that
> change.
When I typed the patch I did not think about this functional change.
And I agree, we want to keep it.

I will fix this in v2 if we agree to add more logging functions.

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

Re: [PATCH v1 6/8] drm/print: add drm_dev_* logging functions

[Sam Ravnborg]

Hi Jani.

On Mon, Dec 23, 2019 at 01:29:19PM +0200, Jani Nikula wrote:
> On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> > There are a lot of cases where we have a device * but no drm_device *.
> > Add drm_dev_* variants of the logging functions to cover these cases.
> 
> So I know there are some valid cases where we only have struct device *,
> and instead of passing struct drm_device * will need the distinction
> between multiple struct device *.
> 
> Not all current uses of DRM_DEV_* meet that criteria, however. I think
> I'd like to have those converted over to the drm_device based logging
> first, and then see what's left. Because I fear adding these will just
> lead to mass conversion from DRM_DEV_* to drm_dev_*, and the ball gets
> dropped there.

Hmm...
$ git grep -E '(DRM_DEV_ERROR|DRM_DEV_INFO|DRM_DEV_WARN|DRM_DEV_DEBUG)'
953
There are 4 hits in drm/* - the rest is in drivers (no suprise).


$ git grep -E '(DRM_ERROR|DRM_INFO|DRM_WARN|DRM_DEBUG)' | wc -l
8380
There are 626 hits in drm/* - the rest in drivers.


So moving over all DRM_DEV looks doable with a lot of effort. It touches
all drivers.
But the non-DEV variants - thats just too much.
This is a lot of effort required before we can offer new drivers
a simple a logical logging solution.

On top of this - there is today no gain using drm_device * versus device *.
The output is exactly the same.

We should discuss what is required before we can offer the full solution
for new drivers. And how much the existing usage should hold this back.

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

Re: [PATCH v1 6/8] drm/print: add drm_dev_* logging functions

[Jani Nikula]

On Mon, 23 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Jani.
>
> On Mon, Dec 23, 2019 at 01:29:19PM +0200, Jani Nikula wrote:
>> On Sat, 21 Dec 2019, Sam Ravnborg <sam@ravnborg.org> wrote:
>> > There are a lot of cases where we have a device * but no drm_device *.
>> > Add drm_dev_* variants of the logging functions to cover these cases.
>> 
>> So I know there are some valid cases where we only have struct device *,
>> and instead of passing struct drm_device * will need the distinction
>> between multiple struct device *.
>> 
>> Not all current uses of DRM_DEV_* meet that criteria, however. I think
>> I'd like to have those converted over to the drm_device based logging
>> first, and then see what's left. Because I fear adding these will just
>> lead to mass conversion from DRM_DEV_* to drm_dev_*, and the ball gets
>> dropped there.
>
> Hmm...
> $ git grep -E '(DRM_DEV_ERROR|DRM_DEV_INFO|DRM_DEV_WARN|DRM_DEV_DEBUG)'
> 953
> There are 4 hits in drm/* - the rest is in drivers (no suprise).
>
>
> $ git grep -E '(DRM_ERROR|DRM_INFO|DRM_WARN|DRM_DEBUG)' | wc -l
> 8380
> There are 626 hits in drm/* - the rest in drivers.
>
>
> So moving over all DRM_DEV looks doable with a lot of effort. It touches
> all drivers.
> But the non-DEV variants - thats just too much.
> This is a lot of effort required before we can offer new drivers
> a simple a logical logging solution.

I guess that's part of the point. Do we even want to offer new non-dev
based alternatives for (DRM_ERROR|DRM_INFO|DRM_WARN|DRM_DEBUG)? We'll
end up carrying the alternatives for years. And face tons of churn for
no real benefit. Why not just stick to the old ones when you're not
using a drm device based alternative?

Switching from non-dev based logging to drm device based logging, OTOH,
is worth the churn.

> On top of this - there is today no gain using drm_device * versus device *.
> The output is exactly the same.

For me and i915 the gain is in not having to do the dereference
everywhere. Having drm device available is the more common case.

If you go through the current DRM_DEV_* uses, a significant portion of
them use drm->dev or have drm device readily available. Again, I'd
prefer converting them over to drm device instead of just changing the
call to another struct device based version. And then see how big the
demand really is for struct device based logging before adding all
possible variations of it. Could they do with less?

Using drm device gives us the benefit that we can also add drm device
based debug control if we want, for example to enable debug only for a
certain device in a multi-GPU system. That option is not easily
available with struct device based logging.

> We should discuss what is required before we can offer the full solution
> for new drivers. And how much the existing usage should hold this back.

I guess I'm more concerned about existing drivers and the conversion
than new drivers...


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel