[PATCH v2 0/2] drm: document logging functions

[PATCH v2 0/2] drm: document logging functions

[Sam Ravnborg]

Add kernel-doc for the drm_ and DRM_ logging
functions.
This is the documentation that I missed when I started to use
the logging functions.

Version 1 of this patchset included drm_ variants of the existing
logging functions - but they are left out for now.
The idea is that we should try to use drm_ logging in favour
of the the variants that take a device *.

This patchset document the existing logging functions with
no functional changes.
And the documentation is properly wired up in drm-internals.rst

	Sam

Sam Ravnborg (2):
      drm/print: document drm_ logging functions
      drm/print: document DRM_ logging functions

 Documentation/gpu/drm-internals.rst |   6 ++
 include/drm/drm_print.h             | 164 +++++++++++++++++++++++++++++++++---
 2 files changed, 159 insertions(+), 11 deletions(-)


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

[PATCH v2 1/2] drm/print: document drm_ 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.

v2:
  - Add parameters to the logging functions in the doc
  - Drop notes on other types of logging

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             | 80 ++++++++++++++++++++++++++---
 2 files changed, 79 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..89e75eea65d2 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,52 @@ 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, ...)
+ *
+ * DRM/Drivers can use the following functions for logging.
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   drm_dbg(drm, fmt, ...)
+ *   drm_info(drm, fmt, ...)
+ *   drm_notice(drm, fmt, ...)
+ *   drm_warn(drm, fmt, ...)
+ *   drm_err(drm, fmt, ...)
+ *
+ *   # Log only once
+ *   drm_info_once(drm, fmt, ...)
+ *   drm_notice_once(drm, fmt, ...)
+ *   drm_warn_once(drm, fmt, ...)
+ *   drm_err_once(drm, fmt, ...)
+ *
+ *   # Ratelimited - do not flood the logs
+ *   drm_err_ratelimited(drm, fmt, ...)
+ *
+ *   # Logging with a specific category
+ *   drm_dbg_core(drm, fmt, ...)
+ *   drm_dbg(drm, fmt, ...)		# Uses the DRIVER category
+ *   drm_dbg_kms(drm, fmt, ...)
+ *   drm_dbg_prime(drm, fmt, ...)
+ *   drm_dbg_atomic(drm, fmt, ...)
+ *   drm_dbg_vbl(drm, fmt, ...)
+ *   drm_dbg_state(drm, fmt, ...)
+ *   drm_dbg_lease(drm, fmt, ...)
+ *   drm_dbg_dp(drm, fmt, ...)
+ *
+ * See enum &drm_debug_category for a description of the categories.
+ *
+ */
+
+/**
+ * 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

Re: [PATCH v2 1/2] drm/print: document drm_ logging functions

[Daniel Vetter]

On Thu, Jan 02, 2020 at 11:15:18PM +0100, Sam Ravnborg wrote:
> This is the documentation I have missed when I looked for help
> how to do proper logging. Hopefully it can help others.
> 
> v2:
>   - Add parameters to the logging functions in the doc
>   - Drop notes on other types of logging
> 
> 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             | 80 ++++++++++++++++++++++++++---
>  2 files changed, 79 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..89e75eea65d2 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,52 @@ 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, ...)
> + *
> + * DRM/Drivers can use the following functions for logging.
> + *
> + * .. code-block:: none
> + *
> + *   # Plain logging
> + *   drm_dbg(drm, fmt, ...)
> + *   drm_info(drm, fmt, ...)
> + *   drm_notice(drm, fmt, ...)
> + *   drm_warn(drm, fmt, ...)
> + *   drm_err(drm, fmt, ...)
> + *
> + *   # Log only once
> + *   drm_info_once(drm, fmt, ...)
> + *   drm_notice_once(drm, fmt, ...)
> + *   drm_warn_once(drm, fmt, ...)
> + *   drm_err_once(drm, fmt, ...)
> + *
> + *   # Ratelimited - do not flood the logs
> + *   drm_err_ratelimited(drm, fmt, ...)
> + *
> + *   # Logging with a specific category
> + *   drm_dbg_core(drm, fmt, ...)
> + *   drm_dbg(drm, fmt, ...)		# Uses the DRIVER category
> + *   drm_dbg_kms(drm, fmt, ...)
> + *   drm_dbg_prime(drm, fmt, ...)
> + *   drm_dbg_atomic(drm, fmt, ...)
> + *   drm_dbg_vbl(drm, fmt, ...)
> + *   drm_dbg_state(drm, fmt, ...)
> + *   drm_dbg_lease(drm, fmt, ...)
> + *   drm_dbg_dp(drm, fmt, ...)
> + *
> + * See enum &drm_debug_category for a description of the categories.
> + *
> + */

I kinda can't decide between this and just copypasting fairly repetitive
kerneldoc over all the new functions. I think given the long-term idea is
to favour the above functions over all the screaming macros (because of
multi-gpu stuff), I'd go with full kerneldocs for these, plus comments or
a note in the overview doc that everything else is kinda deprecated.

Jani, thoughts?
-Daniel

> +
> +/**
> + * enum drm_debug_category - The DRM debug categories
>   */
>  enum drm_debug_category {
>  	/**
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm/print: document DRM_ logging functions

[Sam Ravnborg]

Document the remaining DRM_ logging functions.
As the logging functions are now all properly
listed drop the few specific kernel-doc markers
so we keep the relevant parts in the 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 | 84 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 89e75eea65d2..abe247199bf7 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -335,6 +335,82 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
  *
  * See enum &drm_debug_category for a description of the categories.
  *
+ * Logging when a &device * is available, but no &drm_device *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * DRM/Drivers can use the following functions for logging when there is a
+ * struct device * available.
+ * The logging functions share the same prototype:
+ *
+ * .. code-block:: c
+ *
+ *   void DRM_xxx(struct device *, char * fmt, ...)
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   DRM_DEV_INFO(dev, fmt, ...)
+ *   DRM_DEV_ERROR(dev, fmt, ...)
+ *
+ *   # Log only once
+ *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
+ *
+ *   # Ratelimited - do not flood the logs
+ *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
+ *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
+ *
+ *   # Logging with a specific category
+ *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
+ *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
+ *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
+ *
+ * Logging when no &device * nor &drm_device * is available
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * DRM/Drivers can use the following functions for logging when there is no
+ * extra info associated to the logging.
+ * The logging functions share the same prototype:
+ *
+ * .. code-block:: c
+ *
+ *   void DRM_xxx(char * fmt, ...)
+ *
+ * .. code-block:: none
+ *
+ *   # Plain logging
+ *   DRM_INFO(fmt, ...)
+ *   DRM_NOTE(fmt, ...)
+ *   DRM_WARN(fmt, ...)
+ *   DRM_ERROR(fmt, ...)
+ *
+ *   # Log only once
+ *   DRM_INFO_ONCE(fmt, ...)
+ *   DRM_NOTE_ONCE(fmt, ...)
+ *   DRM_WARN_ONCE(fmt, ...)
+ *
+ *   # Ratelimited - do not flood the logs
+ *   DRM_DEBUG_RATELIMITED(fmt, ...)
+ *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
+ *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
+ *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
+ *   DRM_ERROR_RATELIMITED(fmt, ...)
+ *
+ *   # Logging with a specific category
+ *   DRM_DEBUG(fmt, ...)		# Logged as CORE
+ *   DRM_DEBUG_DRIVER(fmt, ...)
+ *   DRM_DEBUG_KMS(fmt, ...)
+ *   DRM_DEBUG_PRIME(fmt, ...)
+ *   DRM_DEBUG_ATOMIC(fmt, ...)
+ *   DRM_DEBUG_VBL(fmt, ...)
+ *   DRM_DEBUG_LEASE(fmt, ...)
+ *   DRM_DEBUG_DP(fmt, ...)
  */
 
 /**
@@ -399,7 +475,7 @@ __printf(3, 4)
 void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 		 const char *format, ...);
 
-/**
+/*
  * Error output.
  *
  * @dev: device pointer
@@ -408,7 +484,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 #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
@@ -436,7 +512,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 	}								\
 })
 
-/**
+/*
  * Debug output.
  *
  * @dev: device pointer
@@ -466,7 +542,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
 })
 
-/**
+/*
  * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
  *
  * @dev: device pointer
-- 
2.20.1

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

Re: [PATCH v2 2/2] drm/print: document DRM_ logging functions

[Daniel Vetter]

On Thu, Jan 02, 2020 at 11:15:19PM +0100, Sam Ravnborg wrote:
> Document the remaining DRM_ logging functions.
> As the logging functions are now all properly
> listed drop the few specific kernel-doc markers
> so we keep the relevant parts in the 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 | 84 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 4 deletions(-)
> 
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 89e75eea65d2..abe247199bf7 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -335,6 +335,82 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
>   *
>   * See enum &drm_debug_category for a description of the categories.
>   *
> + * Logging when a &device * is available, but no &drm_device *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * DRM/Drivers can use the following functions for logging when there is a
> + * struct device * available.
> + * The logging functions share the same prototype:

I'd replace the entire block with a "This stuff is deprecated" warning. We
have at least a corresponding todo.rst entry.
-Daniel

> + *
> + * .. code-block:: c
> + *
> + *   void DRM_xxx(struct device *, char * fmt, ...)
> + *
> + * .. code-block:: none
> + *
> + *   # Plain logging
> + *   DRM_DEV_INFO(dev, fmt, ...)
> + *   DRM_DEV_ERROR(dev, fmt, ...)
> + *
> + *   # Log only once
> + *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
> + *
> + *   # Ratelimited - do not flood the logs
> + *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
> + *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
> + *
> + *   # Logging with a specific category
> + *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
> + *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
> + *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
> + *
> + * Logging when no &device * nor &drm_device * is available
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * DRM/Drivers can use the following functions for logging when there is no
> + * extra info associated to the logging.
> + * The logging functions share the same prototype:
> + *
> + * .. code-block:: c
> + *
> + *   void DRM_xxx(char * fmt, ...)
> + *
> + * .. code-block:: none
> + *
> + *   # Plain logging
> + *   DRM_INFO(fmt, ...)
> + *   DRM_NOTE(fmt, ...)
> + *   DRM_WARN(fmt, ...)
> + *   DRM_ERROR(fmt, ...)
> + *
> + *   # Log only once
> + *   DRM_INFO_ONCE(fmt, ...)
> + *   DRM_NOTE_ONCE(fmt, ...)
> + *   DRM_WARN_ONCE(fmt, ...)
> + *
> + *   # Ratelimited - do not flood the logs
> + *   DRM_DEBUG_RATELIMITED(fmt, ...)
> + *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
> + *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
> + *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
> + *   DRM_ERROR_RATELIMITED(fmt, ...)
> + *
> + *   # Logging with a specific category
> + *   DRM_DEBUG(fmt, ...)		# Logged as CORE
> + *   DRM_DEBUG_DRIVER(fmt, ...)
> + *   DRM_DEBUG_KMS(fmt, ...)
> + *   DRM_DEBUG_PRIME(fmt, ...)
> + *   DRM_DEBUG_ATOMIC(fmt, ...)
> + *   DRM_DEBUG_VBL(fmt, ...)
> + *   DRM_DEBUG_LEASE(fmt, ...)
> + *   DRM_DEBUG_DP(fmt, ...)
>   */
>  
>  /**
> @@ -399,7 +475,7 @@ __printf(3, 4)
>  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  		 const char *format, ...);
>  
> -/**
> +/*
>   * Error output.
>   *
>   * @dev: device pointer
> @@ -408,7 +484,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  #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
> @@ -436,7 +512,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  	}								\
>  })
>  
> -/**
> +/*
>   * Debug output.
>   *
>   * @dev: device pointer
> @@ -466,7 +542,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
>  })
>  
> -/**
> +/*
>   * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
>   *
>   * @dev: device pointer
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/print: document DRM_ logging functions

[Sam Ravnborg]

Hi Daniel.

> > + * Logging when a &device * is available, but no &drm_device *
> > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + * DRM/Drivers can use the following functions for logging when there is a
> > + * struct device * available.
> > + * The logging functions share the same prototype:
> 
> I'd replace the entire block with a "This stuff is deprecated" warning. We
> have at least a corresponding todo.rst entry.

We have many situations where no drm_device is available.
At least when you a buried in drm_panel patches.

So it is either DRM_DEV_ERROR() or drv_err().
Which is why I have pushed for nicer drm_ variants of these...

The todo entry only covers the nice new macros that Jani added
where we have a drm_device.

	Sam



> -Daniel
> 
> > + *
> > + * .. code-block:: c
> > + *
> > + *   void DRM_xxx(struct device *, char * fmt, ...)
> > + *
> > + * .. code-block:: none
> > + *
> > + *   # Plain logging
> > + *   DRM_DEV_INFO(dev, fmt, ...)
> > + *   DRM_DEV_ERROR(dev, fmt, ...)
> > + *
> > + *   # Log only once
> > + *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
> > + *
> > + *   # Ratelimited - do not flood the logs
> > + *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
> > + *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
> > + *
> > + *   # Logging with a specific category
> > + *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
> > + *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
> > + *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
> > + *
> > + * Logging when no &device * nor &drm_device * is available
> > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > + *
> > + * DRM/Drivers can use the following functions for logging when there is no
> > + * extra info associated to the logging.
> > + * The logging functions share the same prototype:
> > + *
> > + * .. code-block:: c
> > + *
> > + *   void DRM_xxx(char * fmt, ...)
> > + *
> > + * .. code-block:: none
> > + *
> > + *   # Plain logging
> > + *   DRM_INFO(fmt, ...)
> > + *   DRM_NOTE(fmt, ...)
> > + *   DRM_WARN(fmt, ...)
> > + *   DRM_ERROR(fmt, ...)
> > + *
> > + *   # Log only once
> > + *   DRM_INFO_ONCE(fmt, ...)
> > + *   DRM_NOTE_ONCE(fmt, ...)
> > + *   DRM_WARN_ONCE(fmt, ...)
> > + *
> > + *   # Ratelimited - do not flood the logs
> > + *   DRM_DEBUG_RATELIMITED(fmt, ...)
> > + *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
> > + *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
> > + *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
> > + *   DRM_ERROR_RATELIMITED(fmt, ...)
> > + *
> > + *   # Logging with a specific category
> > + *   DRM_DEBUG(fmt, ...)		# Logged as CORE
> > + *   DRM_DEBUG_DRIVER(fmt, ...)
> > + *   DRM_DEBUG_KMS(fmt, ...)
> > + *   DRM_DEBUG_PRIME(fmt, ...)
> > + *   DRM_DEBUG_ATOMIC(fmt, ...)
> > + *   DRM_DEBUG_VBL(fmt, ...)
> > + *   DRM_DEBUG_LEASE(fmt, ...)
> > + *   DRM_DEBUG_DP(fmt, ...)
> >   */
> >  
> >  /**
> > @@ -399,7 +475,7 @@ __printf(3, 4)
> >  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  		 const char *format, ...);
> >  
> > -/**
> > +/*
> >   * Error output.
> >   *
> >   * @dev: device pointer
> > @@ -408,7 +484,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  #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
> > @@ -436,7 +512,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  	}								\
> >  })
> >  
> > -/**
> > +/*
> >   * Debug output.
> >   *
> >   * @dev: device pointer
> > @@ -466,7 +542,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >  		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
> >  })
> >  
> > -/**
> > +/*
> >   * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
> >   *
> >   * @dev: device pointer
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/print: document DRM_ logging functions

[Daniel Vetter]

On Tue, Jan 07, 2020 at 07:17:52PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
> 
> > > + * Logging when a &device * is available, but no &drm_device *
> > > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > + *
> > > + * DRM/Drivers can use the following functions for logging when there is a
> > > + * struct device * available.
> > > + * The logging functions share the same prototype:
> > 
> > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > have at least a corresponding todo.rst entry.
> 
> We have many situations where no drm_device is available.
> At least when you a buried in drm_panel patches.
> 
> So it is either DRM_DEV_ERROR() or drv_err().
> Which is why I have pushed for nicer drm_ variants of these...

Huh, drm_panel indeed has no drm_device. And I guess we don't have a
convenient excuse to add it ...

> The todo entry only covers the nice new macros that Jani added
> where we have a drm_device.

I wonder whether for those cases we shouldn't just directly use the
various dev_* macros?
-Daniel

> 
> 	Sam
> 
> 
> 
> > -Daniel
> > 
> > > + *
> > > + * .. code-block:: c
> > > + *
> > > + *   void DRM_xxx(struct device *, char * fmt, ...)
> > > + *
> > > + * .. code-block:: none
> > > + *
> > > + *   # Plain logging
> > > + *   DRM_DEV_INFO(dev, fmt, ...)
> > > + *   DRM_DEV_ERROR(dev, fmt, ...)
> > > + *
> > > + *   # Log only once
> > > + *   DRM_DEV_INFO_ONCE(dev, fmt, ...)
> > > + *
> > > + *   # Ratelimited - do not flood the logs
> > > + *   DRM_DEV_DEBUG_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_DRIVER_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_KMS_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_PRIME_RATELIMITED(dev, fmt, ...)
> > > + *   DRM_DEV_ERROR_RATELIMITED(dev, fmt, ...)
> > > + *
> > > + *   # Logging with a specific category
> > > + *   DRM_DEV_DEBUG(dev, fmt, ...)		# Logged as CORE
> > > + *   DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_KMS(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_PRIME(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_ATOMIC(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_VBL(dev, fmt, ...)
> > > + *   DRM_DEV_DEBUG_DP(dev, fmt, ...)
> > > + *
> > > + * Logging when no &device * nor &drm_device * is available
> > > + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > + *
> > > + * DRM/Drivers can use the following functions for logging when there is no
> > > + * extra info associated to the logging.
> > > + * The logging functions share the same prototype:
> > > + *
> > > + * .. code-block:: c
> > > + *
> > > + *   void DRM_xxx(char * fmt, ...)
> > > + *
> > > + * .. code-block:: none
> > > + *
> > > + *   # Plain logging
> > > + *   DRM_INFO(fmt, ...)
> > > + *   DRM_NOTE(fmt, ...)
> > > + *   DRM_WARN(fmt, ...)
> > > + *   DRM_ERROR(fmt, ...)
> > > + *
> > > + *   # Log only once
> > > + *   DRM_INFO_ONCE(fmt, ...)
> > > + *   DRM_NOTE_ONCE(fmt, ...)
> > > + *   DRM_WARN_ONCE(fmt, ...)
> > > + *
> > > + *   # Ratelimited - do not flood the logs
> > > + *   DRM_DEBUG_RATELIMITED(fmt, ...)
> > > + *   DRM_DEBUG_DRIVER_RATELIMITED(fmt, ...)
> > > + *   DRM_DEBUG_KMS_RATELIMITED(fmt, ...)
> > > + *   DRM_DEBUG_PRIME_RATELIMITED(fmt, ...)
> > > + *   DRM_ERROR_RATELIMITED(fmt, ...)
> > > + *
> > > + *   # Logging with a specific category
> > > + *   DRM_DEBUG(fmt, ...)		# Logged as CORE
> > > + *   DRM_DEBUG_DRIVER(fmt, ...)
> > > + *   DRM_DEBUG_KMS(fmt, ...)
> > > + *   DRM_DEBUG_PRIME(fmt, ...)
> > > + *   DRM_DEBUG_ATOMIC(fmt, ...)
> > > + *   DRM_DEBUG_VBL(fmt, ...)
> > > + *   DRM_DEBUG_LEASE(fmt, ...)
> > > + *   DRM_DEBUG_DP(fmt, ...)
> > >   */
> > >  
> > >  /**
> > > @@ -399,7 +475,7 @@ __printf(3, 4)
> > >  void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  		 const char *format, ...);
> > >  
> > > -/**
> > > +/*
> > >   * Error output.
> > >   *
> > >   * @dev: device pointer
> > > @@ -408,7 +484,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  #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
> > > @@ -436,7 +512,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  	}								\
> > >  })
> > >  
> > > -/**
> > > +/*
> > >   * Debug output.
> > >   *
> > >   * @dev: device pointer
> > > @@ -466,7 +542,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> > >  		drm_dev_dbg(dev, category, fmt, ##__VA_ARGS__);		\
> > >  })
> > >  
> > > -/**
> > > +/*
> > >   * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
> > >   *
> > >   * @dev: device pointer
> > > -- 
> > > 2.20.1
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/print: document DRM_ logging functions

[Sam Ravnborg]

Hi Daniel.
> > > 
> > > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > > have at least a corresponding todo.rst entry.
> > 
> > We have many situations where no drm_device is available.
> > At least when you a buried in drm_panel patches.
> > 
> > So it is either DRM_DEV_ERROR() or drv_err().
> > Which is why I have pushed for nicer drm_ variants of these...
> 
> Huh, drm_panel indeed has no drm_device. And I guess we don't have a
> convenient excuse to add it ...
> 
> > The todo entry only covers the nice new macros that Jani added
> > where we have a drm_device.
> 
> I wonder whether for those cases we shouldn't just directly use the
> various dev_* macros?

We would miss the nice [drm] marker in the logging.
So [drm] will be added by the drivers and the core - but not the panels.
That is the only drawback I see right now.

Which was enough justification for me to add the drm_dev_ variants.
Feel free to convince me that this is not justification to add these
variants.

In drm/panel/* there is no use of DRM_DEBUG* - and there is no
reason to introduce the variants we can filer with drm.debug.

There is a single DRM_DEBUG() user, which does not count here.


We could introduce only:

drm_dev_(err|warn|info|debug) - and not the more specialized variants.

Then we avoid that people make shortcuts and use drm_dev_dbg_kms() when
they are supposed to use drm_dbg_kms().
This was one of the very valid argumest against the patch that
introduced all the drm_dev_* variants.

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

Re: [PATCH v2 2/2] drm/print: document DRM_ logging functions

[Joe Perches]

On Wed, 2020-01-08 at 21:04 +0100, Sam Ravnborg wrote:
> Hi Daniel.
> > > > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > > > have at least a corresponding todo.rst entry.
> > > 
> > > We have many situations where no drm_device is available.
> > > At least when you a buried in drm_panel patches.
> > > 
> > > So it is either DRM_DEV_ERROR() or drv_err().
> > > Which is why I have pushed for nicer drm_ variants of these...
> > 
> > Huh, drm_panel indeed has no drm_device. And I guess we don't have a
> > convenient excuse to add it ...
> > 
> > > The todo entry only covers the nice new macros that Jani added
> > > where we have a drm_device.
> > 
> > I wonder whether for those cases we shouldn't just directly use the
> > various dev_* macros?
> 
> We would miss the nice [drm] marker in the logging.
> So [drm] will be added by the drivers and the core - but not the panels.
> That is the only drawback I see right now.
> 
> Which was enough justification for me to add the drm_dev_ variants.
> Feel free to convince me that this is not justification to add these
> variants.
> 
> In drm/panel/* there is no use of DRM_DEBUG* - and there is no
> reason to introduce the variants we can filer with drm.debug.
> 
> There is a single DRM_DEBUG() user, which does not count here.
> 
> 
> We could introduce only:
> 
> drm_dev_(err|warn|info|debug) - and not the more specialized variants.
> 
> Then we avoid that people make shortcuts and use drm_dev_dbg_kms() when
> they are supposed to use drm_dbg_kms().
> This was one of the very valid argumest against the patch that
> introduced all the drm_dev_* variants.

A few of these defines aren't used at all.

$ git grep -P -oh "DRM_DEV_DEBUG[A-Z_]*" | sort | uniq -c | sort -rn
     98 DRM_DEV_DEBUG_KMS
     38 DRM_DEV_DEBUG_DRIVER
     26 DRM_DEV_DEBUG
      2 DRM_DEV_DEBUG_RATELIMITED
      2 DRM_DEV_DEBUG_PRIME_RATELIMITED
      2 DRM_DEV_DEBUG_KMS_RATELIMITED
      2 DRM_DEV_DEBUG_DRIVER_RATELIMITED
      1 DRM_DEV_DEBUG_VBL
      1 DRM_DEV_DEBUG_PRIME
      1 DRM_DEV_DEBUG_DP
      1 DRM_DEV_DEBUG_ATOMIC

It might be sensible to consolidate these into just 2 calls
and add an appropriate <type> argument.

	DRM_DEV_DEBUG(dev, type, fmt, ...)
	DRM_DEV_DEBUG_RATELIMITED(dev, type, fmt, ...)

or

	drm_dev_dbg(dev, type, fmt, ...)
	drm_dev_dbg_ratelimited(dev, type, fmt, ...)

A treewide sed is trivial.


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

Re: [PATCH v2 2/2] drm/print: document DRM_ logging functions

[Daniel Vetter]

On Wed, Jan 08, 2020 at 09:04:16PM +0100, Sam Ravnborg wrote:
> Hi Daniel.
> > > > 
> > > > I'd replace the entire block with a "This stuff is deprecated" warning. We
> > > > have at least a corresponding todo.rst entry.
> > > 
> > > We have many situations where no drm_device is available.
> > > At least when you a buried in drm_panel patches.
> > > 
> > > So it is either DRM_DEV_ERROR() or drv_err().
> > > Which is why I have pushed for nicer drm_ variants of these...
> > 
> > Huh, drm_panel indeed has no drm_device. And I guess we don't have a
> > convenient excuse to add it ...
> > 
> > > The todo entry only covers the nice new macros that Jani added
> > > where we have a drm_device.
> > 
> > I wonder whether for those cases we shouldn't just directly use the
> > various dev_* macros?
> 
> We would miss the nice [drm] marker in the logging.
> So [drm] will be added by the drivers and the core - but not the panels.
> That is the only drawback I see right now.
> 
> Which was enough justification for me to add the drm_dev_ variants.
> Feel free to convince me that this is not justification to add these
> variants.

tbh I don't mind, I just don't want to have a massive proliferation of drm
debugging functions, all kinda alike but not the same. If we can settle on
some color choice for this bikeshed and actually reduce the not-longer
favoured version, I'm pretty much ok with anything.

> In drm/panel/* there is no use of DRM_DEBUG* - and there is no
> reason to introduce the variants we can filer with drm.debug.
> 
> There is a single DRM_DEBUG() user, which does not count here.
> 
> 
> We could introduce only:
> 
> drm_dev_(err|warn|info|debug) - and not the more specialized variants.
> 
> Then we avoid that people make shortcuts and use drm_dev_dbg_kms() when
> they are supposed to use drm_dbg_kms().
> This was one of the very valid argumest against the patch that
> introduced all the drm_dev_* variants.

Hm, if you want something for panels, what about a drm_panel_* set of
functions? Plus ofc patches to just roll them out everywhere (it should be
a simple sed/cocci job, drm/panel/* isn't that big). Some churn, but yay!
for consisteny at least.

If we have another set of generic drm debug/logging functions then I think
consistency is going to take a big toll, and we're already bad with this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel