From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 08/11] drm: Pull the test on drm_debug in the logging macros Date: Tue, 25 Mar 2014 11:56:24 +0200 Message-ID: <87bnwuvbxz.fsf@intel.com> References: <1395676398-7058-1-git-send-email-damien.lespiau@intel.com> <1395676398-7058-9-git-send-email-damien.lespiau@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 242A989A44 for ; Tue, 25 Mar 2014 02:56:02 -0700 (PDT) In-Reply-To: <1395676398-7058-9-git-send-email-damien.lespiau@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Damien Lespiau , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Mon, 24 Mar 2014, Damien Lespiau 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 > --- > 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