Re: [PATCH 08/11] drm: Pull the test on drm_debug in the logging macros

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

On Mon, 24 Mar 2014, Damien Lespiau <damien.lespiau@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center