On Mon, 24 Mar 2014, Damien Lespiau <damien.lespiau at intel.com> wrote:
> In the logging code, we are currently checking is we need to output in
s/is/if/
> drm_ut_debug_printk(). This is too late. The problem is that when we write
> something like:
>
> DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> connector->base.id,
> drm_get_connector_name(connector),
> connector->encoder->base.id,
> drm_get_encoder_name(connector->encoder));
>
> We start by evaluating the arguments (so call drm_get_connector_name() and
> drm_get_connector_name()) before ending up in drm_ut_debug_printk() which will
> then does nothing.
s/does/do/
> This means we execute a lot of instructions (drm_get_connector_name(), in
> turn,
> calls snprintf() for example) to happily discard them in the normal case,
> drm.debug=0.
>
> So, let's put the test on drm_debug earlier, in the macros themselves.
> Sprinkle an unlikely() as well for good measure.
Bikeshed, is it likely that the unlikely matters all that much?
https://lwn.net/Articles/70473/
I won't insist on removing them, it's more the casual nature of the
"sprinkling unlikely for good measure" that bugs me.
BR,
Jani.
>
> Signed-off-by: Damien Lespiau <damien.lespiau at intel.com>
> ---
> drivers/gpu/drm/drm_stub.c | 26 ++++++++++++--------------
> include/drm/drmP.h | 27 +++++++++++++++------------
> 2 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index dc2c609..81ed832 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -97,26 +97,24 @@ int drm_err(const char *func, const char *format, ...)
> }
> EXPORT_SYMBOL(drm_err);
>
> -void drm_ut_debug_printk(unsigned int request_level,
> - const char *prefix,
> +void drm_ut_debug_printk(const char *prefix,
> const char *function_name,
> const char *format, ...)
> {
> struct va_format vaf;
> va_list args;
>
> - if (drm_debug & request_level) {
> - va_start(args, format);
> - vaf.fmt = format;
> - vaf.va = &args;
> -
> - if (function_name)
> - printk(KERN_DEBUG "[%s:%s], %pV", prefix,
> - function_name, &vaf);
> - else
> - printk(KERN_DEBUG "%pV", &vaf);
> - va_end(args);
> - }
> + va_start(args, format);
> + vaf.fmt = format;
> + vaf.va = &args;
> +
> + if (function_name)
> + printk(KERN_DEBUG "[%s:%s], %pV", prefix,
> + function_name, &vaf);
> + else
> + printk(KERN_DEBUG "%pV", &vaf);
> +
> + va_end(args);
> }
> EXPORT_SYMBOL(drm_ut_debug_printk);
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3055b36..87b558a 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -121,9 +121,8 @@ struct videomode;
> #define DRM_UT_KMS 0x04
> #define DRM_UT_PRIME 0x08
>
> -extern __printf(4, 5)
> -void drm_ut_debug_printk(unsigned int request_level,
> - const char *prefix,
> +extern __printf(3, 4)
> +void drm_ut_debug_printk(const char *prefix,
> const char *function_name,
> const char *format, ...);
> extern __printf(2, 3)
> @@ -209,24 +208,28 @@ int drm_err(const char *func, const char *format, ...);
> #if DRM_DEBUG_CODE
> #define DRM_DEBUG(fmt, args...)
> \
> do { \
> - drm_ut_debug_printk(DRM_UT_CORE, DRM_NAME, \
> - __func__, fmt, ##args); \
> + if (unlikely(drm_debug & DRM_UT_CORE)) \
> + drm_ut_debug_printk(DRM_NAME, __func__, \
> + fmt, ##args); \
> } while (0)
>
> #define DRM_DEBUG_DRIVER(fmt, args...)
> \
> do { \
> - drm_ut_debug_printk(DRM_UT_DRIVER, DRM_NAME, \
> - __func__, fmt, ##args); \
> + if (unlikely(drm_debug & DRM_UT_DRIVER)) \
> + drm_ut_debug_printk(DRM_NAME, __func__, \
> + fmt, ##args); \
> } while (0)
> -#define DRM_DEBUG_KMS(fmt, args...) \
> +#define DRM_DEBUG_KMS(fmt, args...) \
> do { \
> - drm_ut_debug_printk(DRM_UT_KMS, DRM_NAME, \
> - __func__, fmt, ##args); \
> + if (unlikely(drm_debug & DRM_UT_KMS)) \
> + drm_ut_debug_printk(DRM_NAME, __func__, \
> + fmt, ##args); \
> } while (0)
> #define DRM_DEBUG_PRIME(fmt, args...)
> \
> do { \
> - drm_ut_debug_printk(DRM_UT_PRIME, DRM_NAME, \
> - __func__, fmt, ##args); \
> + if (unlikely(drm_debug & DRM_UT_PRIME)) \
> + drm_ut_debug_printk(DRM_NAME, __func__, \
> + fmt, ##args); \
> } while (0)
> #else
> #define DRM_DEBUG_DRIVER(fmt, args...) do { } while (0)
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
