Re: [PATCH RESEND 8/8] drm/print: introduce new struct drm_device based logging macros

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 28, 2019 at 12:38:22PM +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@ravnborg.org>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> 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.

Looks reasonable enough to me. For the series
Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

One thing that still bugs me about drm_dbg() is that the category
check is inside it. So we pay the function call overhead for every
debug statement even when debugging is disabled. So I'd also like
to see a patch moving the category check back into the 
macros/static inline (IIRC it was there originally).

> ---
>  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..e4040dea0d8c 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx