On Tue, Mar 25, 2014 at 11:56:24AM +0200, Jani Nikula wrote:
> On Mon, 24 Mar 2014, Damien Lespiau <damien.lespiau@xxxxxxxxx> 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.
I've considered this and since most users don't set debug every we should
be easily able to hit the 1:1M ratio seemingly required to make debug
work.
And once you enable debugging the actual printk overhead will wash out
anything you pay in branch mispredicts anyway.
-Daniel
>
>
> BR,
> Jani.
>
>
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
