Re: [PATCH v2 07/20] drm/print: Fixup DRM_DEBUG_KMS_RATELIMITED()

From: Robert Foss
Date: Tue Mar 30 2021 - 06:44:09 EST


Hey Lyude,

I'm seeing no issues with this patch and the reasoning behind the
patch is sound to me.

Reviewed-by: Robert Foss <robert.foss@xxxxxxxxxx>

On Fri, 26 Mar 2021 at 21:39, Lyude Paul <lyude@xxxxxxxxxx> wrote:
>
> Since we're about to move drm_dp_helper.c over to drm_dbg_*(), we'll want
> to make sure that we can also add ratelimited versions of these macros in
> order to retain some of the previous debugging output behavior we had.
>
> However, as I was preparing to do this I noticed that the current
> rate limited macros we have are kind of bogus. It looks like when I wrote
> these, I didn't notice that we'd always be calling __ratelimit() even if
> the debugging message we'd be printing would normally be filtered out due
> to the relevant DRM debugging category being disabled.
>
> So, let's fix this by making sure to check drm_debug_enabled() in our
> ratelimited macros before calling __ratelimit(), and start using
> drm_dev_printk() in order to print debugging messages since that will save
> us from doing a redundant drm_debug_enabled() check. And while we're at it,
> let's move the code for this into another macro that we can reuse for
> defining new ratelimited DRM debug macros more easily.
>
> v2:
> * Make sure to use tabs where possible in __DRM_DEFINE_DBG_RATELIMITED()
>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: Robert Foss <robert.foss@xxxxxxxxxx>
> ---
> include/drm/drm_print.h | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index f32d179e139d..a3c58c941bdc 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -524,16 +524,20 @@ void __drm_err(const char *format, ...);
> #define DRM_DEBUG_DP(fmt, ...) \
> __drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
>
> -
> -#define DRM_DEBUG_KMS_RATELIMITED(fmt, ...) \
> -({ \
> - static DEFINE_RATELIMIT_STATE(_rs, \
> - DEFAULT_RATELIMIT_INTERVAL, \
> - DEFAULT_RATELIMIT_BURST); \
> - if (__ratelimit(&_rs)) \
> - drm_dev_dbg(NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__); \
> +#define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...) \
> +({ \
> + static DEFINE_RATELIMIT_STATE(rs_, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST);\
> + const struct drm_device *drm_ = (drm); \
> + \
> + if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(&rs_)) \
> + drm_dev_printk(drm_ ? drm_->dev : NULL, KERN_DEBUG, fmt, ## __VA_ARGS__); \
> })
>
> +#define drm_dbg_kms_ratelimited(drm, fmt, ...) \
> + __DRM_DEFINE_DBG_RATELIMITED(KMS, drm, fmt, ## __VA_ARGS__)
> +
> +#define DRM_DEBUG_KMS_RATELIMITED(fmt, ...) drm_dbg_kms_ratelimited(NULL, fmt, ## __VA_ARGS__)
> +
> /*
> * struct drm_device based WARNs
> *
> --
> 2.30.2
>