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

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 10 Dec 2019, Jani Nikula <jani.nikula@intel.com> 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@ravnborg.org>
> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Acked-by: Sean Paul <sean@poorly.run>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Pushed this one patch, thanks for the reviews and acks. The rest could
still use review. ;)

BR,
Jani.


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

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