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

[Sam Ravnborg]

Hi Jani.

On Tue, Dec 10, 2019 at 02:30:43PM +0200, Jani Nikula wrote:
> Add new struct drm_device based logging macros modeled after the core
> kernel device based logging macros. These would be preferred over the
> drm printk and struct device based macros in drm code, where possible.
> 
> We have existing drm specific struct device based logging functions, but
> they are too verbose to use for two main reasons:
> 
>  * The names are unnecessarily long, for example DRM_DEV_DEBUG_KMS().
> 
>  * The use of struct device over struct drm_device is too generic for
>    most users, leading to an extra dereference.
> 
> For example:
> 
> 	DRM_DEV_DEBUG_KMS(drm->dev, "Hello, world\n");
> 
> vs.
> 
> 	drm_dbg_kms(drm, "Hello, world\n");
> 
> It's a matter of taste, but the SHOUTING UPPERCASE has been argued to be
> less readable than lowercase.
> 
> Some names are changed from old DRM names to be based on the core kernel
> logging functions. For example, NOTE -> notice, ERROR -> err, DEBUG ->
> dbg.
> 
> Due to the conflation of DRM_DEBUG and DRM_DEBUG_DRIVER macro use
> (DRM_DEBUG is used widely in drivers though it's supposed to be a core
> debugging category), they are named as drm_dbg_core and drm_dbg,
> respectively.
> 
> The drm_err and _once/_ratelimited variants no longer include the
> function name in order to be able to use the core device based logging
> macros. Arguably this is not a significant change; error messages should
> not be so common to be only distinguishable by the function name.
> 
> Ratelimited debug logging macros are to be added later.
> 
> Cc: Sam Ravnborg <sam at ravnborg.org>
> Acked-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Acked-by: Sean Paul <sean at poorly.run>
> Signed-off-by: Jani Nikula <jani.nikula at intel.com>

To my sensitive eyes the lower case variants are much preferable.

As a follow-up it could be nice to clean up drm_print.h:
- Make it obvious that the old variants are deprecated
- Let the old variants use the new variants - to make it obvious they
  are obsolete wrappers.
- Add some intro that explains for newbies when to use what variant

And then add a todo item - so we can get some janitorials to help with the
conversion to the new varaints.


For logging we have three cases:
- We have a drm_device pointer - nicely covered by this patchset
- We have a device * - what do we do here?
- We have no pointers to device nor drm_device - what do we do here?

Would it be OK to consider drm variants for all the above - so we get
consistent prefix on logging?

Idea:

drm_<level>[_system] - example: drm_info(drm_device *, ..) or drm_info_core(drm_device *, ..)

drm_dev_<level>[_system] - example: drm_dev_info(device *, ..)

drm_pr_<level>[_system] - example: drm_pr_info(..)

level could be info, info_once, info_ratelimited and so on for dbg, err,
notice, warn

With the above I can see we can make a clean shift to drm based logging.
And we do not need to mix different ways to log stuf.

The preferred:
drm_info()
drm_dev_info()
drm_pr_info()

versus:
drm_info()
dev_info()
pr_info()

The patch is OK without the suggested change, but see this as
suggestions for improvements.
So patch as is has my:
Acked-by: Sam Ravnborg <sam at ravnborg.org>


	Sam


> 
> ---
> 
> With something like this, I think i915 could start migrating to
> drm_device based logging. I have a hard time convincing myself or anyone
> about migrating to the DRM_DEV_* variants.
> ---
>  include/drm/drm_print.h | 65 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 085a9685270c..8f99d389792d 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -322,6 +322,8 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
>  
>  /*
>   * struct device based logging
> + *
> + * Prefer drm_device based logging over device or prink based logging.
>   */
>  
>  __printf(3, 4)
> @@ -417,8 +419,71 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>  	_DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, DRM_UT_PRIME,		\
>  					  fmt, ##__VA_ARGS__)
>  
> +/*
> + * struct drm_device based logging
> + *
> + * Prefer drm_device based logging over device or prink based logging.
> + */
> +
> +/* Helper for struct drm_device based logging. */
> +#define __drm_printk(drm, level, type, fmt, ...)			\
> +	dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__)
> +
> +
> +#define drm_info(drm, fmt, ...)					\
> +	__drm_printk((drm), info,, fmt, ##__VA_ARGS__)
> +
> +#define drm_notice(drm, fmt, ...)				\
> +	__drm_printk((drm), notice,, fmt, ##__VA_ARGS__)
> +
> +#define drm_warn(drm, fmt, ...)					\
> +	__drm_printk((drm), warn,, fmt, ##__VA_ARGS__)
> +
> +#define drm_err(drm, fmt, ...)					\
> +	__drm_printk((drm), err,, "*ERROR* " fmt, ##__VA_ARGS__)
> +
> +
> +#define drm_info_once(drm, fmt, ...)				\
> +	__drm_printk((drm), info, _once, fmt, ##__VA_ARGS__)
> +
> +#define drm_notice_once(drm, fmt, ...)				\
> +	__drm_printk((drm), notice, _once, fmt, ##__VA_ARGS__)
> +
> +#define drm_warn_once(drm, fmt, ...)				\
> +	__drm_printk((drm), warn, _once, fmt, ##__VA_ARGS__)
> +
> +#define drm_err_once(drm, fmt, ...)				\
> +	__drm_printk((drm), err, _once, "*ERROR* " fmt, ##__VA_ARGS__)
> +
> +
> +#define drm_err_ratelimited(drm, fmt, ...)				\
> +	__drm_printk((drm), err, _ratelimited, "*ERROR* " fmt, ##__VA_ARGS__)
> +
> +
> +#define drm_dbg_core(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
> +#define drm_dbg(drm, fmt, ...)						\
> +	drm_dev_dbg((drm)->dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> +#define drm_dbg_kms(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
> +#define drm_dbg_prime(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
> +#define drm_dbg_atomic(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
> +#define drm_dbg_vbl(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_VBL, fmt, ##__VA_ARGS__)
> +#define drm_dbg_state(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_STATE, fmt, ##__VA_ARGS__)
> +#define drm_dbg_lease(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
> +#define drm_dbg_dp(drm, fmt, ...)					\
> +	drm_dev_dbg((drm)->dev, DRM_UT_DP, fmt, ##__VA_ARGS__)
> +
> +
>  /*
>   * printk based logging
> + *
> + * Prefer drm_device based logging over device or prink based logging.
>   */
>  
>  __printf(2, 3)
> -- 
> 2.20.1