[Intel-gfx] [RFC v2] drm: Enable dynamic debug for DRM_[DEV]_DEBUG*

[Daniel Vetter]

On Thu, Dec 08, 2016 at 11:57:25AM +0000, Robert Bragg wrote:
> On Thu, Dec 8, 2016 at 12:17 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> >
> > On Wed, Dec 07, 2016 at 06:35:29PM +0000, Robert Bragg wrote:
> > > This is still missing corresponding documentation changes, and I haven't
> > > moved anything to drm_print.h yet, as suggested.
> > >
> > > Sending out with a few functional improvements first to get agreement
> > > before documenting anything (changes summarised in v2: section below)
> > >
> > > In particular, affecting the output format, I stole an idea from Tvrtko
> > > Ursulin to have the prefix for messages be based on the driver name,
> > > such as "[i915]" instead of always being "[drm]".
> > >
> > > Depending on peoples thoughts on compatibility, we could consider
> > > removing the prefix given that the dynamic debug control interface has a
> > > way of specifying that messages should include a module name, function
> > > or line info like:
> > >
> > > echo "module i915 +mfp" > dynamic_debug/control
> > >
> > > That would enable all i915 debug messages with a module and function
> > > prefix.
> > >
> > > A trade-off would be that anyone only using the drm.drm_debug interface
> > > to control messages would loose some information. If we really wanted we
> > > could have the best of both by adding a utility printing api that can
> > > recognise when printing due to a dynamic debug control query vs
> > > drm.drm_debug to conditionally add the prefix.
> > >
> > > --- >8 --- (git am --scissors)
> > >
> > > Dynamic debug messages (ref: Documentation/dynamic-debug-howto.txt)
> > > allow fine grained control over which debug messages are enabled with
> > > runtime control through /sysfs/kernel/debug/dynamic_debug/control
> > >
> > > This provides more control than the current drm.drm_debug parameter
> > > which for some use cases is impractical to use given how chatty
> > > some drm debug categories are.
> > >
> > > For example all debug messages in i915_drm.c can be enabled with:
> > > echo "file i915_perf.c +p" > dynamic_debug/control
> > >
> > > This doesn't strictly maintain format compatibility with the previous
> > > debug messages since the category is now added as part of the prefix
> > > like "[drm][kms] No FB found". Adding the categories with a consistent
> > > format makes it possible to enable categories with a dynamic debug
> > > query like: echo "format [kms] +p" > dynamic_debug/control
> > >
> > > This maintains support for enabling debug messages using the drm_debug
> > > parameter. If dynamic debug is not enabled via CONFIG_DYNAMIC_DEBUG the
> > > debug messages essentially work as before, except with the inclusion of
> > > categories in the format strings as described above.
> > >
> > > This removes the drm_[dev_]printk wrappers considering that the dynamic
> > > debug macros are only useful if they can track the __FILE__, __func__
> > > and __LINE__ where they are called. The wrapper didn't seem necessary in
> > > the DRM_UT_NONE case with no category flag.
> > >
> > > The non _DEV macros are no longer defined in terms of passing NULL to a
> > > _DEV variant to avoid have the core.c dev_printk implementation adding
> > > "(NULL device *)". The previous drm_[dev_]prink function used to handle
> > > this as a special case.
> > >
> > > Instead of using DRM_NAME to add [drm] to the start of every message,
> > > the prefix is now based on module_name(THIS_MODULE) so it will be [drm]
> > > or e.g. [i915] for the Intel driver. Later we might consider removing
> > > the prefix altogether considering that the dynamic debug control
> > > interface has a way of optionally adding the module, function or line to
> > > the formatting of messages.
> > >
> > > v2:
> > >     Add categories to format like "[drm][kms] No FB found"
> > >     Only single conditional call per message (macros expand to less code)
> > >     Uses __dynamic_pr_debug/dev_dbg for dynamic formatting features
> > >     Use module name for msg prefix like [drm] or [i915]
> > >
> > > Signed-off-by: Robert Bragg <robert at sixbynine.org>
> > > Cc: dri-devel at lists.freedesktop.org
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >
> > So assuming I understand it correctly - I like this 3way cascade of
> > dynamic debug, then printk and no_printk fallback if CONFIG_DEBUG=n for
> > the space concious. But I guess we do need to add a DRM Kconfig knob to
> > set DEBUG, at least I'm not entirely sure how that's supposed to work. Or
> > we might need to have our own #ifdef maze for this. Maybe we need to keep
> > the old drm*printk stuff for that?
> 
> Right, I wasn't really sure who/what's responsible for defining DEBUG.
> 
> I just found this convention documented in Documentation/CodingStyle:
> 
> > Coming up with good debugging messages can be quite a challenge; and once
> > you have them, they can be a huge help for remote troubleshooting.  However
> > debug message printing is handled differently than printing other non-debug
> > messages.  While the other pr_XXX() functions print unconditionally,
> > pr_debug() does not; it is compiled out by default, unless either DEBUG is
> > defined or CONFIG_DYNAMIC_DEBUG is set.  That is true for dev_dbg() also,
> > and a related convention uses VERBOSE_DEBUG to add dev_vdbg() messages to
> > the ones already enabled by DEBUG.
> >
> > Many subsystems have Kconfig debug options to turn on -DDEBUG in the
> > corresponding Makefile; in other cases specific files #define DEBUG.  And
> > when a debug message should be unconditionally printed, such as if it is
> > already inside a debug-related #ifdef section, printk(KERN_DEBUG ...) can be
> > used.
> 
> So I think if we want to follow something similar, we're missing a
> general kconfig option like CONFIG_DEBUG_DRM that would handle adding
> -DDDEBUG across drm core and the drivers. I did notice that a couple
> of drm drivers do have their own options for this:
> CONFIG_OMAP2_DSS_DEBUG and CONFIG_DRM_TEGRA_DEBUG.

Yeah, CONFIG_DEBUG_DRM to set -DDEBUG (and default y for backwards compat)
is probably what we need. I think we can leave drivers as-is for now,
maintainers can decide for themselves whether they want to chain to the
overall drm debug (should be much easier with dyndebug since then they can
define their own components for filtering) or leave their separate debug
Kconfig knob.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch