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

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