Skip to content
Permalink
Browse files
drm_print: add choice to use dynamic debug in drm-debug
drm's debug system writes 10 distinct categories of messages to syslog
using a small API[1]: drm_dbg*(10 names), DRM_DEV_DEBUG*(3 names),
DRM_DEBUG*(8 names).  There are thousands of these callsites, each
categorized in this systematized way.

These callsites can be enabled at runtime by their category, each
controlled by a bit in drm.debug (/sys/modules/drm/parameter/debug).
In the current "basic" implementation, drm_debug_enabled() tests these
bits in __drm_debug each time an API[1] call is executed; while cheap
individually, the costs accumulate with uptime.

This patch uses dynamic-debug with (required) jump-label to patch
enabled calls onto their respective NOOP slots, avoiding all runtime
bit-checks of __drm_debug by drm_debug_enabled().

Dynamic debug has no concept of category, but we can emulate one by
replacing enum categories with a set of prefix-strings; "drm:core:",
"drm:kms:" "drm:driver:" etc, and prepend them (at compile time) to
the given formats.

Then we can use:
  `echo module drm format "^drm:core: " +p > control`

to enable the whole category with one query.

This conversion yields many new prdbg callsites:

  dyndbg: 207 debug prints in module drm_kms_helper
  dyndbg: 376 debug prints in module drm
  dyndbg: 1811 debug prints in module i915
  dyndbg: 3917 debug prints in module amdgpu

Each site costs 56 bytes of .data, which is a big increase for
drm modules, so CONFIG_DRM_USE_DYNAMIC_DEBUG makes it optional.

CONFIG_JUMP_LABEL is also required, to get the promised optimizations.

The "basic" -> "dyndbg" switchover is layered into the macro scheme

A. A "prefix" version of DRM_UT_<CATs> map, named DRM_DBG_CAT_<CATs>

"basic":  DRM_DBG_CAT_<CATs>  <===  DRM_UT_<CATs>.  Identity map.
"dyndbg":
   #define DRM_DBG_CAT_KMS    "drm:kms: "
   #define DRM_DBG_CAT_PRIME  "drm:prime: "
   #define DRM_DBG_CAT_ATOMIC "drm:atomic: "

DRM_UT_* are preserved, since theyre used elsewhere.  Since the
callback maintains its state in __drm_debug, drm_debug_enabled() will
stay synchronized, and continue to work.  We can address them
separately if they are called enough to be worth fixing.

B. drm_dev_dbg() & drm_debug() are interposed with macros

basic:	  forward to renamed fn, with args preserved
enabled:  redirect to pr_debug, dev_dbg, with CATEGORY format catenated

This is where drm_debug_enabled() is avoided.  The prefix is prepended
at compile-time, no category at runtime.

C. API[1] uses DRM_DBG_CAT_<CAT>s

The API already uses B, now it uses A too, instead of DRM_UT_<CAT>, to
get the correct token type for "basic" and "dyndbg" configs.

D. use DEFINE_DYNAMIC_DEBUG_CATEGORIES()

This defines the map using DRM_CAT_<CAT>s, and creates the /sysfs
bitmap to control those categories.

CONFIG_DRM_USE_DYNAMIC_DEBUG is also used to adjust amdgpu, i915
makefiles to add -DDYNAMIC_DEBUG_MODULE; it includes the current
CONFIG_DYNAMIC_DEBUG_CORE and is enabled by the user.

LIMITATIONS:

dev_dbg(etal) effectively prepends twice, category then driver-name,
yielding format strings like so:

bash-5.1# grep amdgpu: /proc/dynamic_debug/control | grep drm: | cut -d= -f2-
_ "amdgpu: drm:core: fence driver on ring %s use gpu addr 0x%016llx\012"
_ "amdgpu: drm:kms: Cannot create framebuffer from imported dma_buf\012"

This means we cannot use anchored "^drm:kms: " to specify the
category, a small loss of precision.

Note that searching on "format ^amdgpu: " works, but this is less
valuable, because the same can be done with "module amdgpu".

NOTES:

Because the dyndbg callback is keeping state in __drm_debug, it
synchronizes with drm_debug_enabled() and its remaining users; the
switchover should be transparent.

Code Review is expected to catch the lack of correspondence between
bit=>prefix definitions (the selector) and the prefixes used in the
API[1] layer above pr_debug()

I've coded the categories with trailing spaces.  This excludes any
sub-categories which might get added later.  This convention protects
any "drm:atomic:fail:" callsites from getting stomped on by `echo 0 >
debug`.  Other categories could differ, but we need some default.

Dyndbg requires that the prefix be in the compiled-in format string;
run-time prefixing evades callsite selection by category.

	pr_debug("%s: ...", __func__, ...) // not ideal

Unfortunately __func__ is not a macro, and cannot be catenated at
preprocess/compile time.

If you want that, you might consider +mfl flags instead;)

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
v5:
. use DEFINE_DYNAMIC_DEBUG_CATEGORIES in drm_print.c
. s/DRM_DBG_CLASS_/DRM_DBG_CAT_/ - dont need another term
. default=y in Kconfig entry - per @danvet
. move some commit-log prose to dyndbg commit
. add-prototyes to (param_get/set)_dyndbg
. more wrinkles found by <lkp@intel.com>
. relocate ratelimit chunk from elsewhere
v6:
. add kernel doc
. fix cpp paste, drop '#'
v7:
. change __drm_debug to long, to fit with DEFINE_DYNAMIC_DEBUG_CATEGORIES
. add -DDYNAMIC_DEBUG_MODULE to ccflags if DRM_USE_DYNAMIC_DEBUG
v8:
. adapt to altered ^ insertion
. add mem cost numbers to kconfig
. kdoc improvements (I hope)
  • Loading branch information
jimc committed Oct 16, 2021
1 parent fa51ea3 commit 13595be14ee22859c794542d847670ab9bf69897
Show file tree
Hide file tree
Showing 6 changed files with 209 additions and 53 deletions.
@@ -62,6 +62,32 @@ config DRM_DEBUG_MM

If in doubt, say "N".

config DRM_USE_DYNAMIC_DEBUG
bool "use dynamic debug to implement drm.debug"
default y
depends on DRM
depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE
depends on JUMP_LABEL
help
The "basic" drm.debug facility does a lot of unlikely
bit-field tests at runtime; while cheap individually, the
cost accumulates. DYNAMIC_DEBUG can patch pr_debug()s into
NOOP slots, in a running kernel, so avoids those bit-test
overheads, and is therefore recommended by default.

DRM_USE_DYNAMIC_DEBUG converts "basic" to "dyndbg", this
creates many new dyndbg callsites (56 bytes each), which
significantly increases drm* module .data, so is optional.
On an x86-64 kernel, with a config derived from fedora, that
price is:
#prdbgs KiB #with DRM_USE_DYNAMIC_DEBUG=y
kernel 3079 166k
drm 1 .06k 376 21k
drm_kms_helper 207 12k
i915 167 9.3k 1811 101k
amdgpu 2339 130k 3917 220k
nouveau 3 .17k 105 ~60k

config DRM_DEBUG_SELFTEST
tristate "kselftests for DRM"
depends on DRM
@@ -20,6 +20,9 @@ drm-y := drm_aperture.o drm_auth.o drm_cache.o \
drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o \
drm_managed.o drm_vblank_work.o

#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
ccflags-y += -DDYNAMIC_DEBUG_MODULE
#endif
drm-$(CONFIG_DRM_LEGACY) += drm_agpsupport.o drm_bufs.o drm_context.o drm_dma.o \
drm_legacy_misc.o drm_lock.o drm_memory.o drm_scatter.o \
drm_vm.o
@@ -38,7 +38,7 @@ ccflags-y := -I$(FULL_AMD_PATH)/include/asic_reg \
-I$(FULL_AMD_DISPLAY_PATH)/amdgpu_dm \
-I$(FULL_AMD_PATH)/amdkfd

ccflags-$(CONFIG_DYNAMIC_DEBUG_CORE) += -DYNAMIC_DEBUG_MODULE
ccflags-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DYNAMIC_DEBUG_MODULE

amdgpu-y := amdgpu_drv.o

@@ -28,6 +28,7 @@
#include <linux/stdarg.h>

#include <linux/io.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
@@ -40,19 +41,38 @@
* __drm_debug: Enable debug output.
* Bitmask of DRM_UT_x. See include/drm/drm_print.h for details.
*/
unsigned int __drm_debug;
unsigned long __drm_debug;
EXPORT_SYMBOL(__drm_debug);

MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
"\t\tBit 0 (0x01) will enable CORE messages (drm core code)\n"
"\t\tBit 1 (0x02) will enable DRIVER messages (drm controller code)\n"
"\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n"
"\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n"
"\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n"
"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n"
"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)\n"
"\t\tBit 8 (0x100) will enable DP messages (displayport code)");
module_param_named(debug, __drm_debug, int, 0600);
#define DRM_DEBUG_DESC \
"Enable debug output, where each bit enables a debug category.\n" \
"\t\tBit 0 (0x01) will enable CORE messages (drm core code)\n" \
"\t\tBit 1 (0x02) will enable DRIVER messages (drm controller code)\n" \
"\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n" \
"\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n" \
"\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n" \
"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n" \
"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)\n" \
"\t\tBit 8 (0x100) will enable DP messages (displayport code)."

#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
MODULE_PARM_DESC(debug, DRM_DEBUG_DESC);
module_param_named(debug, __drm_debug, ulong, 0600);
#else
#include <linux/dynamic_debug.h>
DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
DRM_DEBUG_DESC,
[0] = { DRM_DBG_CAT_CORE },
[1] = { DRM_DBG_CAT_DRIVER },
[2] = { DRM_DBG_CAT_KMS },
[3] = { DRM_DBG_CAT_PRIME },
[4] = { DRM_DBG_CAT_ATOMIC },
[5] = { DRM_DBG_CAT_VBL },
[6] = { DRM_DBG_CAT_STATE },
[7] = { DRM_DBG_CAT_LEASE },
[8] = { DRM_DBG_CAT_DP },
[9] = { DRM_DBG_CAT_DRMRES });
#endif

void __drm_puts_coredump(struct drm_printer *p, const char *str)
{
@@ -256,8 +276,8 @@ void drm_dev_printk(const struct device *dev, const char *level,
}
EXPORT_SYMBOL(drm_dev_printk);

void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
const char *format, ...)
void __drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
const char *format, ...)
{
struct va_format vaf;
va_list args;
@@ -278,9 +298,9 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,

va_end(args);
}
EXPORT_SYMBOL(drm_dev_dbg);
EXPORT_SYMBOL(__drm_dev_dbg);

void __drm_dbg(enum drm_debug_category category, const char *format, ...)
void ___drm_dbg(enum drm_debug_category category, const char *format, ...)
{
struct va_format vaf;
va_list args;
@@ -297,7 +317,7 @@ void __drm_dbg(enum drm_debug_category category, const char *format, ...)

va_end(args);
}
EXPORT_SYMBOL(__drm_dbg);
EXPORT_SYMBOL(___drm_dbg);

void __drm_err(const char *format, ...)
{
@@ -301,7 +301,7 @@ i915-y += intel_gvt.o
include $(src)/gvt/Makefile
endif

ccflags-$(CONFIG_DYNAMIC_DEBUG_CORE) += -DDYNAMIC_DEBUG_MODULE
ccflags-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DDYNAMIC_DEBUG_MODULE

obj-$(CONFIG_DRM_I915) += i915.o
obj-$(CONFIG_DRM_I915_GVT_KVMGT) += gvt/kvmgt.o

0 comments on commit 13595be

Please sign in to comment.