Skip to content
Permalink
Browse files
drm_print: add choice to use dynamic debug in drm-debug
drm's debug system writes 10 enumerated categories of debug messages,
calling drm_debug_enabled(drm_debug_category) to determine at runtime
whether to printk.

DYNAMIC_DEBUG (with CONFIG_JUMP_LABEL) can eliminate those runtime
tests with kernel-patching, giving zero overhead for normal ops, in
trade for transient costs during rare toggles of the drm.debug bits.

dyndbg has no concept of categories, but if we can modify the exact
message format, we can prepend a prefix: "drm:core: ", "drm:kms: ",
etc to actual format.

Then we can use:

   # echo module drm format "^drm:core: " +p > /proc/dynamic_debug/control

to enable the whole category with one query.

DEFINE_DYNAMIC_DEBUG_BITGRPS implements the bitmap interface for
/sys/module/drm/parameter/debug

This is CONFIG_DRM_USE_DYNAMIC_DEBUG dependent, since there are many
drm.debug callsites, with 56 bytes per site:

  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

CONFIG_JUMP_LABEL is also required, to get the promised optimizations.

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

1. DRM_DBG_CAT_<CATs> are either: DRM_UT_<CATs>
   or a string: "drm:core: ", "drm:kms: ", "drm:atomic: ", etc.
   DRM_DBG_CAT_<CATs> is now used

2. drm_dev_dbg() & drm_debug() are interposed with macros, forwarding:

   basic:   to renamed __fns, with DRM_UT_<CAT> passed in
   dyndbg:  do cpp catenaton: DRM_DBG_CAT_<CAT> + format
   	    and forward to pr_debug, dev_dbg

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

3. all drm.debug API calls now use DRM_DBG_CAT_<CAT>: enum or string

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.

4. use DEFINE_DYNAMIC_DEBUG_CATEGORIES()

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

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.  Searching on "format ^amdgpu: "
would work, but this is less valuable, because the same can be done
with "module amdgpu".

Fixing this order of prepending is possible, given that we're adding
"drm:core:" etc anyway, so can probably arrange for its proper
placement.  This all requires buy-in anyway, due to the format change
to the debug messages.

NOTES:

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.

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)
v11:
. add,usse macro to evaluate to either enum-cat or cat-string
  • Loading branch information
jimc committed Dec 8, 2021
1 parent 3db96b0 commit 1241f739b6212e0a12f9c3d65f54468cf33e60c4
Show file tree
Hide file tree
Showing 6 changed files with 158 additions and 52 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
@@ -33,6 +33,8 @@ drm-$(CONFIG_PCI) += drm_pci.o
drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o

ccflags-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DDYNAMIC_DEBUG_MODULE

obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o

drm_vram_helper-y := drm_gem_vram_helper.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,9 +28,11 @@
#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>
#include <linux/dynamic_debug.h>

#include <drm/drm.h>
#include <drm/drm_drv.h>
@@ -40,19 +42,40 @@
* __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
static struct dyndbg_bitdesc drm_dyndbg_bitmap[] = {
[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 }
};
DEFINE_DYNAMIC_DEBUG_BITGRPS(debug, __drm_debug, DRM_DEBUG_DESC,
drm_dyndbg_bitmap);

#endif

void __drm_puts_coredump(struct drm_printer *p, const char *str)
{
@@ -256,8 +279,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 +301,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 +320,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, ...)
{
@@ -317,7 +317,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 1241f73

Please sign in to comment.