linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug
@ 2021-08-31 20:21 Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 1/8] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

Hi Jason, DRM folks,

In DRM-debug currently, drm_debug_enabled() is called a lot to decide
whether or not to write debug messages.  Each test is cheap, but costs
continue with uptime.  DYNAMIC_DEBUG "dyndbg", when built with
JUMP_LABEL, replaces each of those tests with a patchable NOOP, for
"zero cost when off" debugs.

this is v7:
- drops RFC distractions -JBaron
- drops patch-1: kp->data addition in moduleparams.h not needed -JBaron
- rework dyndbg callbacks to use kp->arg instead -JBaron
- fixes for problem configs reported -lkp 
- others noted per patch below ---

Broadly, this patchset does 3 things:

Adds DEFINE_DYNAMIC_DEBUG_CATEGORIES, which defines a mapping from
bits to "categorized" pr_debugs, a sysfs interface, and callbacks to
implement the bits as dynamic_debug >controls.  This is modelled after
DRM's debug interface.

Uses the new macro in amdgpu & i915 to control existing pr_debugs
according to their ad-hoc categorizations.

Adapts the drm-debug API (~20 macros) to configurably "replace"
drm_dbg & drm_dev_dbg with pr_debug & dev_dbg, which avoids
drm_debug_enabled() overheads.  DEFINE_DYNAMIC_DEBUG_CATEGORIES is
used to create the controlling bitmap, which is wired to __drm_debug
var so remaining drm_debug_enabled() users stay in sync.

on a virtual box:
bash-5.1# for m in i915 amdgpu nouveau; do modprobe $m; done
[43833.332326] dyndbg: 384 debug prints in module drm
[43833.609577] dyndbg: 211 debug prints in module drm_kms_helper
[43833.707124] dyndbg:   2 debug prints in module ttm
[43837.471166] dyndbg: 1727 debug prints in module i915
[43838.030774] AMD-Vi: AMD IOMMUv2 driver by Joerg Roedel <jroedel@suse.de>
[43838.031905] AMD-Vi: AMD IOMMUv2 functionality not available on this system
[43846.209583] dyndbg: 3852 debug prints in module amdgpu
[43846.261391] [drm] amdgpu kernel modesetting enabled.
[43846.262512] amdgpu: CRAT table not found
[43846.263264] amdgpu: Virtual CRAT table created for CPU
[43846.264293] amdgpu: Topology: Add CPU node
[43846.743781] dyndbg:   3 debug prints in module wmi
[43852.517497] dyndbg:  92 debug prints in module nouveau

There are a few multi-second delays there, just before dyndbg
initializes the large blocks of debug prints.


Jim Cromie (8):
  dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  dyndbg: remove spaces in pr_debug "gvt: core:" etc prefixes
  i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:"
    etc categories
  amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES
  drm_print: add choice to use dynamic debug in drm-debug
  drm_print: instrument drm_debug_enabled
  amdgpu_ucode: reduce number of pr_debug calls
  nouveau: fold multiple DRM_DEBUG_DRIVERs together

 drivers/gpu/drm/Kconfig                       |  13 +
 drivers/gpu/drm/Makefile                      |   3 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c     | 293 ++++++++++--------
 .../gpu/drm/amd/display/dc/core/dc_debug.c    |  44 ++-
 drivers/gpu/drm/drm_print.c                   |  53 +++-
 drivers/gpu/drm/i915/Makefile                 |   4 +
 drivers/gpu/drm/i915/gvt/debug.h              |  18 +-
 drivers/gpu/drm/i915/i915_params.c            |  35 +++
 drivers/gpu/drm/nouveau/nouveau_drm.c         |  36 ++-
 include/drm/drm_print.h                       | 150 +++++++--
 include/linux/dynamic_debug.h                 |  60 ++++
 lib/dynamic_debug.c                           |  79 ++++-
 12 files changed, 582 insertions(+), 206 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 1/8] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 2/8] dyndbg: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

DEFINE_DYNAMIC_DEBUG_CATEGORIES(name, var, bitmap_desc, @bit_descs)
allows users to define a drm.debug style (bitmap) sysfs interface, and
to specify the desired mapping from bits[0-N] to the format-prefix'd
pr_debug()s to be controlled.

DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
	"i915/gvt bitmap desc",
	/**
	 * search-prefixes, passed to dd-exec_queries
	 * defines bits 0-N in order.
	 * leading ^ is tacitly inserted (by callback currently)
	 * trailing space used here excludes subcats.
	 * helper macro needs more work
	 * macro to autogen ++$i, 0x%x$i ?
	 */
	_DD_cat_("gvt:cmd: "),
	_DD_cat_("gvt:core: "),
	_DD_cat_("gvt:dpy: "),
	_DD_cat_("gvt:el: "),
	_DD_cat_("gvt:irq: "),
	_DD_cat_("gvt:mm: "),
	_DD_cat_("gvt:mmio: "),
	_DD_cat_("gvt:render: "),
	_DD_cat_("gvt:sched: "));

dynamic_debug.c: add 2 new elements:

 - int param_set_dyndbg()
 - int param_get_dyndbg()
 - struct kernel_param_ops param_ops_dyndbg

Following the model of kernel/params.c STANDARD_PARAM_DEFS, these are
non-static and exported.

get/set use an augmented kernel_param; the arg refs a new struct
dyndbg_bitmap_param containing:

- the map (array, indexed by bitpos) of format-prefix strings, which
  define the set/category of prdbgs to be changed per each bit.

- pointer to the user's ulong holding the bits/state.
  by sharing state, we coordinate with code that still uses it directly

This will allow drm-debug to be converted incrementally, while still
using __drm_debug & drm_debug_enabled() in other parts.

param_set_dyndbg() compares new vs old bits, and only updates prdbgs
on changes.  This maximally preserves the underlying state, which may
have been customized via later `echo $cmd >control`.  So if a user
really wants to know that all prdbgs are set precisely, they must
pre-clear then set.

TLDR: this also doesn't affect the decorator flags "mflt" set per prdbg.

dynamic_debug.h:

Add DEFINE_DYNAMIC_DEBUG_CATEGORIES() described above, and a stub
throwing a BUILD_BUG (RFC) when used without DYNAMIC_DEBUG support.
Add structs dyndbg_bitdesc, dyndbg_bitmap_param to support the macro.

Note that it also calls MODULE_PARM_DESC for the user, but expects the
user to catenate all the bit-descriptions together (as is done in
drm.debug), and in the following uses in amdgpu, i915.

The intent is to regenerate this output from per-bit help given in
VA_ARGS, including a bit_label(); but this can wait.

Also externs the struct kernel_param param_ops_dyndbg symbol, as is
done in moduleparams.h for all the STANDARD params.

USAGE NOTES:

Using dyndbg to query on "format ^$prefix" requires that the prefix be
present in the compiled-in format string; where run-time prefixing is
used, that format would be "%s...", which is not usefully selectable.

Using DEFINE_DYNAMIC_DEBUG_CATEGORIES wo support gets a BUILD_BUG.
ISTM there is already action at a declarative distance, nobody needs
mystery as to why the /sysfs thingy didn't appear.

Dyndbg is completely agnostic wrt the categorization scheme used, in
order to play well with any prefix convention already in use in the
codebase.  Ad-hoc categories and sub-categories are implicitly
allowed, author discipline and review is expected.

Here are some examples:

"1","2","3"		2 doesn't imply 1.
   			otherwize, sorta like printk levels
"1:","2:","3:"		are better, avoiding [1-9]\d+ ambiguity
"hi:","mid:","low:"	are reasonable, and imply independence
"todo:","rfc:","fixme:" might be handy
"A:".."Z:"		uhm, yeah

Hierarchical classes/categories are natural:

"drm:<CAT>:"		is used in a later commit
"drm:<CAT>:<SUB>:"	is a natural extension.
"drm:atomic:fail:"	has been proposed, sounds directly useful

NB: in a real sense we abandon enum strictures here, and lose some
compiler help, on spelling errs for example.  Obviously "drm:" != "DRM:".

Some properties of a hierarchical category deserve explication:

Trailing spaces matter !

With 1..3-space ("drm: ", "drm:atomic: ", "drm:atomic:fail: "), the
":" doesn't terminate the search-space, the trailing space does.  So a
"drm:" search spec will match all DRM categories & subcategories, and
will not be useful in an interface where all categories are already
controlled together.  That said, "drm:atomic:" & "drm:atomic: " are
different, and both are useful in cases.

Ad-Hoc sub-categories:

These have a caveat wrt wrapper macros adding prefixes like
"drm:atomic: "; the trailing space in the prefix means that
drm_dbg_atomic("fail: ...") pastes as "drm:atomic: fail: ", which
obviously isn't ideal wrt clear and simple bitmaps.

A possible solution is to have a FOO_() version of every FOO() macro
which (anti-mnemonically) elides the trailing space, which is normally
inserted by a newer FOO().

IE: drm_dbg_atomic_("fail: ..."); // trailing _ means ad-hoc subcat

Summarizing:

 - "drm:kms: " & "drm:kms:" are different
 - "drm:kms"		also different - includes drm:kms2:
 - "drm:kms:\t"		also different - could be troublesome
 - "drm:kms:*"		doesn't work, no wildcard on format atm.

Order matters in DEFINE_DYNAMIC_DEBUG_CATEGORIES(... @bit_descs)

@bit_descs (array) position determines the bit mapping to the prefix,
so to keep a stable map, new categories or 3rd level categories must
be added to the end.

Since bits are/will-stay applied 0-N, the later bits can countermand
the earlier ones, but it's tricky - consider;

    DD_CATs(... "drm:atomic:", "drm:atomic:fail:" ) // misleading

The 1st search-term is misleading, because it includes (modifies)
subcategories, but then 2nd overrides it.  So don't do that.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
v5:
. rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
. in set_dyndbg, replace hardcoded "i915" w kp->mod->name
. static inline the stubs
. const *str in structs, const array. - Emil
. dyndbg: add do-nothing DEFINE_DYNAMIC_DEBUG_CATEGORIES if !DD_CORE
. call MOD_PARM_DESC(name, "$desc") for users
. simplify callback, remove bit-change detection
. config errs reported by <lkp@intel.com>

v6:
. return rc, bitmap->, snprintf, ws - Andy Shevchenko
. s/chgct/matches/ - old varname is misleading
. move code off file bottom to a "better" place
. change ##fsname to ##var for less generic varname (ie: not "debug")
. add KP_MOD_NAME workaround for !CONFIG_MODULES
. move forward decl down to where its needed

v7:
. use kp->arg, dont need kp->data or previous patch-1 - jbaron
. use client's ulong for bits, share state
. throw BUILD_BUG if DEFINE_DYNAMIC_DEBUG_CATEGORIES used wo support
---
 include/linux/dynamic_debug.h | 60 ++++++++++++++++++++++++++
 lib/dynamic_debug.c           | 79 ++++++++++++++++++++++++++++++++++-
 2 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index dce631e678dd..f51b738668a0 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -181,6 +181,10 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 				   KERN_DEBUG, prefix_str, prefix_type,	\
 				   rowsize, groupsize, buf, len, ascii)
 
+struct kernel_param;
+int param_set_dyndbg(const char *instr, const struct kernel_param *kp);
+int param_get_dyndbg(char *buffer, const struct kernel_param *kp);
+
 #else /* !CONFIG_DYNAMIC_DEBUG_CORE */
 
 #include <linux/string.h>
@@ -227,6 +231,62 @@ static inline int dynamic_debug_exec_queries(const char *query, const char *modn
 	return 0;
 }
 
+struct kernel_param;
+static inline int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
+{ return 0; }
+static inline int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
+{ return 0; }
+
 #endif /* !CONFIG_DYNAMIC_DEBUG_CORE */
 
+struct dyndbg_bitdesc {
+	/* bitpos is inferred from index in containing array */
+	const char *prefix;
+	const char *help;
+};
+
+struct dyndbg_bitmap_param {
+	unsigned long *bits;
+	struct dyndbg_bitdesc map[];
+};
+
+#if defined(CONFIG_DYNAMIC_DEBUG) || \
+	(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
+/**
+ * DEFINE_DYNAMIC_DEBUG_CATEGORIES() - bitmap control of categorized prdbgs
+ * @fsname: parameter basename under /sys
+ * @var:    C-identifier holding bitmap
+ * @_desc:  string summarizing the controls provided
+ * @...:    list of struct dyndbg_bitdesc initializations
+ *
+ * Intended for modules with substantial use of "categorized" prdbgs
+ * (those with some systematic prefix in the format string), this lets
+ * modules (using dyndbg) control those prdbg groups according to
+ * their prefixes, and map them to bits 0-N of a sysfs control point.
+ * The @bits... identifies the prefixes to be used by dyndbg to
+ * select and alter those categorized prdbgs, order defines bitpos.
+ */
+#define DEFINE_DYNAMIC_DEBUG_CATEGORIES(fsname, _var, _desc, ...)	\
+	MODULE_PARM_DESC(fsname, _desc);				\
+	static struct dyndbg_bitmap_param ddcats_##_var =		\
+	{ .bits = &_var, .map = { __VA_ARGS__, { .prefix = NULL }}};	\
+	module_param_cb(fsname, &param_ops_dyndbg, &ddcats_##_var, 0644)
+
+#define _DD_cat_(pfx)		{ .prefix = pfx, .help = "help for " pfx }
+#define _DD_cat_help_(pfx)	"\t   " pfx "\t- help for " pfx "\n"
+
+extern const struct kernel_param_ops param_ops_dyndbg;
+
+#elif (defined(CONFIG_DYNAMIC_DEBUG_CORE) && !defined(DYNAMIC_DEBUG_MODULE))
+
+#define DEFINE_DYNAMIC_DEBUG_CATEGORIES(fsname, var, bitmap_desc, ...)	\
+	BUILD_BUG_ON_MSG(1, "you need -DDYNAMIC_DEBUG_MODULE in compile")
+
+#else
+#define DEFINE_DYNAMIC_DEBUG_CATEGORIES(fsname, var, bitmap_desc, ...) \
+	BUILD_BUG_ON_MSG(1, "DYNAMIC_DEBUG support required to use this macro: " #var)
+#define _DD_cat_(pfx)
+#define _DD_cat_help_(pfx)
+#endif
+
 #endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index cb5abb42c16a..9f9e70023aa4 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -529,7 +529,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
 		if (!query || !*query || *query == '#')
 			continue;
 
-		vpr_info("query %d: \"%s\"\n", i, query);
+		vpr_info("query %d: \"%s\" mod:%s\n", i, query, modname ?: "*");
 
 		rc = ddebug_exec_query(query, modname);
 		if (rc < 0) {
@@ -577,6 +577,83 @@ int dynamic_debug_exec_queries(const char *query, const char *modname)
 }
 EXPORT_SYMBOL_GPL(dynamic_debug_exec_queries);
 
+#ifdef CONFIG_MODULES
+#define KP_MOD_NAME kp->mod->name
+#else
+#define KP_MOD_NAME NULL /* wildcard */
+#endif
+#define FMT_QUERY_SIZE 128 /* typically need <40 */
+/**
+ * param_set_dyndbg - bits => categories >control setter
+ * @instr: string echo>d to sysfs
+ * @kp:    kp->arg has state: bits, map
+ *
+ * Enable/disable prdbgs by their "category", as defined in args to
+ * DEFINE_DYNAMIC_DEBUG_CATEGORIES
+ * Returns: 0 or <0 if error.
+ */
+int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
+{
+	unsigned long inbits;
+	int rc, i, matches = 0, totct = 0;
+	char query[FMT_QUERY_SIZE];
+	const struct dyndbg_bitmap_param *p = kp->arg;
+	const struct dyndbg_bitdesc *map = p->map;
+
+	if (!map) {
+		pr_err("set_dyndbg: no bits=>queries map\n");
+		return -EINVAL;
+	}
+	rc = kstrtoul(instr, 0, &inbits);
+	if (rc) {
+		pr_err("set_dyndbg: failed\n");
+		return rc;
+	}
+	vpr_info("set_dyndbg: new 0x%lx old 0x%lx\n", inbits, *p->bits);
+
+	for (i = 0; map->prefix && i < BITS_PER_LONG; map++, i++) {
+
+		if (test_bit(i, &inbits) == test_bit(i, p->bits))
+			continue;
+		snprintf(query, FMT_QUERY_SIZE, "format '^%s' %cp", map->prefix,
+			 test_bit(i, &inbits) ? '+' : '-');
+
+		matches = ddebug_exec_queries(query, KP_MOD_NAME);
+
+		v2pr_info("bit-%d: %d matches on format '^%s'\n", i,
+			  matches, map->prefix);
+		totct += matches;
+	}
+	*p->bits = inbits;
+	vpr_info("total matches: %d\n", totct);
+	return 0;
+}
+EXPORT_SYMBOL(param_set_dyndbg);
+
+/**
+ * param_get_dyndbg - bitmap reader
+ * @buffer: receives string rep of bitmap
+ * @kp:    kp->arg has state: bits, map
+ *
+ * Reads last written bits, underlying prdbg state may have changed since.
+ * Returns: #chars written
+ */
+int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
+{
+	const struct dyndbg_bitmap_param *p = kp->arg;
+	unsigned long val = *p->bits;
+
+	return scnprintf(buffer, PAGE_SIZE, "0x%lx\n", val);
+}
+EXPORT_SYMBOL(param_get_dyndbg);
+
+const struct kernel_param_ops param_ops_dyndbg = {
+	.set = param_set_dyndbg,
+	.get = param_get_dyndbg,
+};
+/* support DEFINE_DYNAMIC_DEBUG_CATEGORIES users */
+EXPORT_SYMBOL(param_ops_dyndbg);
+
 #define PREFIX_SIZE 64
 
 static int remaining(int wrote)
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 2/8] dyndbg: remove spaces in pr_debug "gvt: core:" etc prefixes
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 1/8] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

Taking embedded spaces out of existing prefixes makes them better
class-prefixes; simplifying the extra quoting needed otherwise:

  $> echo format "^gvt: core:" +p >control

Dropping the internal spaces means any trailing space in a query will
more clearly terminate the prefix being searched for.

Consider a generic drm-debug example:

  # turn off ATOMIC reports
  echo format "^drm:atomic: " -p > control

  # turn off all ATOMIC:* reports, including any sub-categories
  echo format "^drm:atomic:" -p > control

  # turn on ATOMIC:FAIL: reports
  echo format "^drm:atomic:fail: " +p > control

Removing embedded spaces in the class-prefixes simplifies the
corresponding match-prefix.  This means that "quoted" match-prefixes
are only needed when the trailing space is desired, in order to
exclude explicitly sub-categorized pr-debugs; in this example,
"drm:atomic:fail:".

RFC: maybe the prefix catenation should paste in the " " class-prefix
terminator explicitly.  A pr_debug_() flavor could exclude the " ",
allowing ad-hoc sub-categorization by appending for example, "fail:"
to "drm:atomic:" without the default " " insertion.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/i915/gvt/debug.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/debug.h b/drivers/gpu/drm/i915/gvt/debug.h
index c6027125c1ec..b4021f41c546 100644
--- a/drivers/gpu/drm/i915/gvt/debug.h
+++ b/drivers/gpu/drm/i915/gvt/debug.h
@@ -36,30 +36,30 @@ do {									\
 } while (0)
 
 #define gvt_dbg_core(fmt, args...) \
-	pr_debug("gvt: core: "fmt, ##args)
+	pr_debug("gvt:core: "fmt, ##args)
 
 #define gvt_dbg_irq(fmt, args...) \
-	pr_debug("gvt: irq: "fmt, ##args)
+	pr_debug("gvt:irq: "fmt, ##args)
 
 #define gvt_dbg_mm(fmt, args...) \
-	pr_debug("gvt: mm: "fmt, ##args)
+	pr_debug("gvt:mm: "fmt, ##args)
 
 #define gvt_dbg_mmio(fmt, args...) \
-	pr_debug("gvt: mmio: "fmt, ##args)
+	pr_debug("gvt:mmio: "fmt, ##args)
 
 #define gvt_dbg_dpy(fmt, args...) \
-	pr_debug("gvt: dpy: "fmt, ##args)
+	pr_debug("gvt:dpy: "fmt, ##args)
 
 #define gvt_dbg_el(fmt, args...) \
-	pr_debug("gvt: el: "fmt, ##args)
+	pr_debug("gvt:el: "fmt, ##args)
 
 #define gvt_dbg_sched(fmt, args...) \
-	pr_debug("gvt: sched: "fmt, ##args)
+	pr_debug("gvt:sched: "fmt, ##args)
 
 #define gvt_dbg_render(fmt, args...) \
-	pr_debug("gvt: render: "fmt, ##args)
+	pr_debug("gvt:render: "fmt, ##args)
 
 #define gvt_dbg_cmd(fmt, args...) \
-	pr_debug("gvt: cmd: "fmt, ##args)
+	pr_debug("gvt:cmd: "fmt, ##args)
 
 #endif
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 1/8] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 2/8] dyndbg: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  2021-09-03 11:07   ` [Intel-gfx] " Tvrtko Ursulin
  2021-08-31 20:21 ` [PATCH v7 4/8] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES Jim Cromie
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

The gvt component of this driver has ~120 pr_debugs, in 9 categories
quite similar to those in DRM.  Following the interface model of
drm.debug, add a parameter to map bits to these categorizations.

DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
	"dyndbg bitmap desc",
	{ "gvt:cmd: ",  "command processing" },
	{ "gvt:core: ", "core help" },
	{ "gvt:dpy: ",  "display help" },
	{ "gvt:el: ",   "help" },
	{ "gvt:irq: ",  "help" },
	{ "gvt:mm: ",   "help" },
	{ "gvt:mmio: ", "help" },
	{ "gvt:render: ", "help" },
	{ "gvt:sched: " "help" });

The actual patch has a few details different, cmd_help() macro emits
the initialization construct.

if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added
cflags, by gvt/Makefile.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
v5:
. static decl of vector of bit->class descriptors - Emil.V
. relocate gvt-makefile chunk from elsewhere
v7:
. move ccflags addition up to i915/Makefile from i915/gvt
---
 drivers/gpu/drm/i915/Makefile      |  4 ++++
 drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 4f22cac1c49b..5a4e371a3ec2 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init)
 
 subdir-ccflags-y += -I$(srctree)/$(src)
 
+#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
+ccflags-y += -DDYNAMIC_DEBUG_MODULE
+#endif
+
 # Please keep these build lists sorted!
 
 # core driver code
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index e07f4cfea63a..e645e149485e 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params)
 	I915_PARAMS_FOR_EACH(FREE);
 #undef FREE
 }
+
+#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
+/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */
+
+unsigned long __gvt_debug;
+EXPORT_SYMBOL(__gvt_debug);
+
+#define _help(key)	"\t    \"" key "\"\t: help for " key "\n"
+
+#define I915_GVT_CATEGORIES(name) \
+	" Enable debug output via /sys/module/i915/parameters/" #name	\
+	", where each bit enables a debug category.\n"			\
+	_help("gvt:cmd:")						\
+	_help("gvt:core:")						\
+	_help("gvt:dpy:")						\
+	_help("gvt:el:")						\
+	_help("gvt:irq:")						\
+	_help("gvt:mm:")						\
+	_help("gvt:mmio:")						\
+	_help("gvt:render:")						\
+	_help("gvt:sched:")
+
+DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
+				I915_GVT_CATEGORIES(debug_gvt),
+				_DD_cat_("gvt:cmd:"),
+				_DD_cat_("gvt:core:"),
+				_DD_cat_("gvt:dpy:"),
+				_DD_cat_("gvt:el:"),
+				_DD_cat_("gvt:irq:"),
+				_DD_cat_("gvt:mm:"),
+				_DD_cat_("gvt:mmio:"),
+				_DD_cat_("gvt:render:"),
+				_DD_cat_("gvt:sched:"));
+
+#endif
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 4/8] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
                   ` (2 preceding siblings ...)
  2021-08-31 20:21 ` [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

logger_types.h defines many DC_LOG_*() categorized debug wrappers.
Most of these use DRM debug API, so are controllable using drm.debug,
but others use bare pr_debug("$prefix: .."), each with a different
class-prefix matching "^\[\w+\]:"

Use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create a /sys debug_dc
parameter, modinfos, and to specify a map from bits -> categorized
pr_debugs to be controlled.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 .../gpu/drm/amd/display/dc/core/dc_debug.c    | 44 ++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_debug.c b/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
index 21be2a684393..9fd43254db88 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_debug.c
@@ -36,8 +36,50 @@
 
 #include "resource.h"
 
-#define DC_LOGGER_INIT(logger)
+#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
+/* define a drm.debug style dyndbg pr-debug control point */
+#include <linux/dynamic_debug.h>
+
+unsigned long __debug_dc;
+EXPORT_SYMBOL(__debug_dc);
+
+#define _help_(key)	"\t   " key "\t- help for " key "\n"
+
+/* Id like to do these inside DEFINE_DYNAMIC_DEBUG_CATEGORIES, if possible */
+#define DC_DYNDBG_BITMAP_DESC(name)					\
+	"Control pr_debugs via /sys/module/amdgpu/parameters/" #name	\
+	", where each bit controls a debug category.\n"			\
+	_help_("[SURFACE]:")						\
+	_help_("[CURSOR]:")						\
+	_help_("[PFLIP]:")						\
+	_help_("[VBLANK]:")						\
+	_help_("[HW_LINK_TRAINING]:")					\
+	_help_("[HW_AUDIO]:")						\
+	_help_("[SCALER]:")						\
+	_help_("[BIOS]:")						\
+	_help_("[BANDWIDTH_CALCS]:")					\
+	_help_("[DML]:")						\
+	_help_("[IF_TRACE]:")						\
+	_help_("[GAMMA]:")						\
+	_help_("[SMU_MSG]:")
+
+DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_dc, __debug_dc,
+	DC_DYNDBG_BITMAP_DESC(debug_dc),
+	_DD_cat_("[CURSOR]:"),
+	_DD_cat_("[PFLIP]:"),
+	_DD_cat_("[VBLANK]:"),
+	_DD_cat_("[HW_LINK_TRAINING]:"),
+	_DD_cat_("[HW_AUDIO]:"),
+	_DD_cat_("[SCALER]:"),
+	_DD_cat_("[BIOS]:"),
+	_DD_cat_("[BANDWIDTH_CALCS]:"),
+	_DD_cat_("[DML]:"),
+	_DD_cat_("[IF_TRACE]:"),
+	_DD_cat_("[GAMMA]:"),
+	_DD_cat_("[SMU_MSG]:"));
+#endif
 
+#define DC_LOGGER_INIT(logger)
 
 #define SURFACE_TRACE(...) do {\
 		if (dc->debug.surface_trace) \
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
                   ` (3 preceding siblings ...)
  2021-08-31 20:21 ` [PATCH v7 4/8] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  2021-09-03 11:15   ` [Intel-gfx] " Tvrtko Ursulin
  2021-08-31 20:21 ` [PATCH v7 6/8] drm_print: instrument drm_debug_enabled Jim Cromie
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

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 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: 195 debug prints in module drm_kms_helper
  dyndbg: 298 debug prints in module drm
  dyndbg: 1630 debug prints in module i915
  dyndbg: ~3500 debug prints in module amdgpu

CONFIG_DRM_USE_DYNAMIC_DEBUG enables this, and is available if
CONFIG_DYNAMIC_DEBUG or CONFIG_DYNAMIC_DEBUG_CORE is chosen, and if
CONFIG_JUMP_LABEL is enabled; this because its 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: "

In v3, had older name, DRM_DBG_CLASS_<CATs> was countered, I had
agreed, but this seems better still; CATEGORY is already DRM's
term-of-art, and adding a near-synonym 'CLASS' only adds ambiguity.

DRM_UT_* are preserved, since theyre used elsewhere.  We can probably
reduce their use further, but thats a separate thing.

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

these already use (B), now they use (A) too, 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.

NOTES:

Because the dyndbg callback is watching __drm_debug, it is coherent
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 search-prefixes/categories with a trailing space, which
excludes any sub-categories 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

With "lineno X" in a query, its possible to enable single callsites,
but it is tedious, and useless in a category context.

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

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 KBuild 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
---
 drivers/gpu/drm/Kconfig     |  13 ++++
 drivers/gpu/drm/Makefile    |   3 +
 drivers/gpu/drm/drm_print.c |  53 +++++++++----
 include/drm/drm_print.h     | 144 ++++++++++++++++++++++++++++--------
 4 files changed, 166 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 7ff89690a976..97e38d86fd27 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -57,6 +57,19 @@ 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 patches pr_debug()s in/out
+	  of the running kernel, so avoids those bit-test overheads,
+	  and is therefore recommended.
+
 config DRM_DEBUG_SELFTEST
 	tristate "kselftests for DRM"
 	depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index a118692a6df7..1809329654b3 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -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
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 111b932cf2a9..df2e10754c41 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -28,6 +28,7 @@
 #include <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,39 @@
  * __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)."
+
+#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
+#include <linux/dynamic_debug.h>
+DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
+	DRM_DEBUG_DESC,
+	_DD_cat_(DRM_DBG_CAT_CORE),
+	_DD_cat_(DRM_DBG_CAT_DRIVER),
+	_DD_cat_(DRM_DBG_CAT_KMS),
+	_DD_cat_(DRM_DBG_CAT_PRIME),
+	_DD_cat_(DRM_DBG_CAT_ATOMIC),
+	_DD_cat_(DRM_DBG_CAT_VBL),
+	_DD_cat_(DRM_DBG_CAT_STATE),
+	_DD_cat_(DRM_DBG_CAT_LEASE),
+	_DD_cat_(DRM_DBG_CAT_DP),
+	_DD_cat_(DRM_DBG_CAT_DRMRES));
+
+#else
+MODULE_PARM_DESC(debug, DRM_DEBUG_DESC);
+module_param_named(debug, __drm_debug, ulong, 0600);
+#endif
 
 void __drm_puts_coredump(struct drm_printer *p, const char *str)
 {
@@ -256,8 +277,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 +299,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 +318,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, ...)
 {
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 9b66be54dd16..973443040561 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -35,7 +35,7 @@
 #include <drm/drm.h>
 
 /* Do *not* use outside of drm_print.[ch]! */
-extern unsigned int __drm_debug;
+extern unsigned long __drm_debug;
 
 /**
  * DOC: print
@@ -252,15 +252,15 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
 /**
  * enum drm_debug_category - The DRM debug categories
  *
- * Each of the DRM debug logging macros use a specific category, and the logging
- * is filtered by the drm.debug module parameter. This enum specifies the values
- * for the interface.
+ * The drm.debug logging API[1] has 10 enumerated categories of
+ * messages, issued by 3 families of macros: 10 drm_dbg_<CATs>, 8
+ * DRM_DEBUG_<CATs>, and 3 DRM_DEV_DEBUG_<CATs>.
  *
  * Each DRM_DEBUG_<CATEGORY> macro logs to DRM_UT_<CATEGORY> category, except
  * DRM_DEBUG() logs to DRM_UT_CORE.
  *
- * Enabling verbose debug messages is done through the drm.debug parameter, each
- * category being enabled by a bit:
+ * Enabling categories of debug messages is done through the drm.debug
+ * parameter, each category being enabled by a bit:
  *
  *  - drm.debug=0x1 will enable CORE messages
  *  - drm.debug=0x2 will enable DRIVER messages
@@ -319,6 +319,86 @@ enum drm_debug_category {
 	DRM_UT_DRMRES		= 0x200,
 };
 
+/**
+ * DOC: DRM_USE_DYNAMIC_DEBUG - using dyndbg in drm.debug
+ *
+ * In the "basic" drm.debug implementation outlined above, each time a
+ * drm-debug API[1] call is executed, drm_debug_enabled(cat) tests
+ * drm.debug vs cat before printing.
+ *
+ * DYNAMIC_DEBUG (aka: dyndbg) patches pr_debug()s in^out of the
+ * running kernel, so it can avoid drm_debug_enabled() and skip lots
+ * of unlikely bit tests.
+ *
+ * dyndbg has no concept of category, but we can prepend a
+ * class-prefix string: "drm:core: ", "drm:kms: ", "drm:driver: " etc,
+ * to pr_debug's format (at compile time).
+ *
+ * Then control the category
+ *    `echo module drm format "^drm:core: " +p > control`
+ *    `dynamic_debug_exec_queries("format '^drm:core: ' +p", "drm");`
+ *
+ * To do this for "basic" | "dyndbg", adaptation adds some macro indirection:
+ *
+ * 0. use dyndbg support to define the bits => prefixes map, attach callback.
+ *
+ *    DYNDBG_BITMAP_DESC(debug, __drm_debug,
+ *			 "drm.debug - overview",
+ *			 { "drm:core:", "enable CORE debug messages" },
+ *			 { "drm:kms:", "enable KMS debug messages" }, ...);
+ *
+ * 1. DRM_DBG_CAT_<CAT>
+ *
+ * This set of symbols replaces DRM_UT_<CAT> in the drm-debug API; it
+ * is either a copy of DRM_UT_<CAT>, or the class-prefix strings.
+ *
+ * 2. drm_dev_dbg & drm_debug are called by drm.debug API
+ *
+ * These are now macros, either forwarding to renamed functions, or
+ * prepending the class string to the format, and invoking pr_debug
+ * directly.  Since the API is all macros, dyndbg remembers
+ * per-pr_debug: module,file,func,line,format and uses that to find
+ * and enable them.
+ */
+#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+
+#define __drm_dbg(cls, fmt, ...)			\
+	___drm_dbg(cls, fmt, ##__VA_ARGS__)
+#define drm_dev_dbg(dev, cls, fmt, ...)			\
+	__drm_dev_dbg(dev, cls, fmt, ##__VA_ARGS__)
+
+#define DRM_DBG_CAT_CORE	DRM_UT_CORE
+#define DRM_DBG_CAT_DRIVER	DRM_UT_DRIVER
+#define DRM_DBG_CAT_KMS		DRM_UT_KMS
+#define DRM_DBG_CAT_PRIME	DRM_UT_PRIME
+#define DRM_DBG_CAT_ATOMIC	DRM_UT_ATOMIC
+#define DRM_DBG_CAT_VBL		DRM_UT_VBL
+#define DRM_DBG_CAT_STATE	DRM_UT_STATE
+#define DRM_DBG_CAT_LEASE	DRM_UT_LEASE
+#define DRM_DBG_CAT_DP		DRM_UT_DP
+#define DRM_DBG_CAT_DRMRES	DRM_UT_DRMRES
+
+#else /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
+
+/* join prefix+format in cpp so dyndbg can see it */
+#define __drm_dbg(cls, fmt, ...)		\
+	pr_debug(cls fmt, ##__VA_ARGS__)
+#define drm_dev_dbg(dev, cls, fmt, ...)		\
+	dev_dbg(dev, cls fmt, ##__VA_ARGS__)
+
+#define DRM_DBG_CAT_CORE	"drm:core: "
+#define DRM_DBG_CAT_DRIVER	"drm:drvr: "
+#define DRM_DBG_CAT_KMS		"drm:kms: "
+#define DRM_DBG_CAT_PRIME	"drm:prime: "
+#define DRM_DBG_CAT_ATOMIC	"drm:atomic: "
+#define DRM_DBG_CAT_VBL		"drm:vbl: "
+#define DRM_DBG_CAT_STATE	"drm:state: "
+#define DRM_DBG_CAT_LEASE	"drm:lease: "
+#define DRM_DBG_CAT_DP		"drm:dp: "
+#define DRM_DBG_CAT_DRMRES	"drm:res: " /* not in MODULE_PARM_DESC */
+
+#endif /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
+
 static inline bool drm_debug_enabled(enum drm_debug_category category)
 {
 	return unlikely(__drm_debug & category);
@@ -334,8 +414,8 @@ __printf(3, 4)
 void drm_dev_printk(const struct device *dev, const char *level,
 		    const char *format, ...);
 __printf(3, 4)
-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, ...);
 
 /**
  * DRM_DEV_ERROR() - Error output.
@@ -383,7 +463,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
  * @fmt: printf() like format string.
  */
 #define DRM_DEV_DEBUG(dev, fmt, ...)					\
-	drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(dev, DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
 /**
  * DRM_DEV_DEBUG_DRIVER() - Debug output for vendor specific part of the driver
  *
@@ -391,7 +471,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
  * @fmt: printf() like format string.
  */
 #define DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)				\
-	drm_dev_dbg(dev, DRM_UT_DRIVER,	fmt, ##__VA_ARGS__)
+	drm_dev_dbg(dev, DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
 /**
  * DRM_DEV_DEBUG_KMS() - Debug output for modesetting code
  *
@@ -399,7 +479,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
  * @fmt: printf() like format string.
  */
 #define DRM_DEV_DEBUG_KMS(dev, fmt, ...)				\
-	drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	drm_dev_dbg(dev, DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
 
 /*
  * struct drm_device based logging
@@ -443,25 +523,25 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
 
 
 #define drm_dbg_core(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_CORE, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
 #define drm_dbg(drm, fmt, ...)						\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
 #define drm_dbg_kms(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
 #define drm_dbg_prime(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_PRIME, fmt, ##__VA_ARGS__)
 #define drm_dbg_atomic(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_ATOMIC, fmt, ##__VA_ARGS__)
 #define drm_dbg_vbl(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_VBL, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_VBL, fmt, ##__VA_ARGS__)
 #define drm_dbg_state(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_STATE, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_STATE, fmt, ##__VA_ARGS__)
 #define drm_dbg_lease(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_LEASE, fmt, ##__VA_ARGS__)
 #define drm_dbg_dp(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DP, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DP, fmt, ##__VA_ARGS__)
 #define drm_dbg_drmres(drm, fmt, ...)					\
-	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
+	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DRMRES, fmt, ##__VA_ARGS__)
 
 
 /*
@@ -471,7 +551,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
  */
 
 __printf(2, 3)
-void __drm_dbg(enum drm_debug_category category, const char *format, ...);
+void ___drm_dbg(enum drm_debug_category category, const char *format, ...);
 __printf(1, 2)
 void __drm_err(const char *format, ...);
 
@@ -500,29 +580,30 @@ void __drm_err(const char *format, ...);
 #define DRM_ERROR_RATELIMITED(fmt, ...)					\
 	DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
 
+
 #define DRM_DEBUG(fmt, ...)						\
-	__drm_dbg(DRM_UT_CORE, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_DRIVER(fmt, ...)					\
-	__drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_KMS(fmt, ...)						\
-	__drm_dbg(DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_PRIME(fmt, ...)					\
-	__drm_dbg(DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_PRIME, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_ATOMIC(fmt, ...)					\
-	__drm_dbg(DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_ATOMIC, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_VBL(fmt, ...)						\
-	__drm_dbg(DRM_UT_VBL, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_VBL, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_LEASE(fmt, ...)					\
-	__drm_dbg(DRM_UT_LEASE, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_LEASE, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_DP(fmt, ...)						\
-	__drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
+	__drm_dbg(DRM_DBG_CAT_DP, fmt, ## __VA_ARGS__)
 
 #define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)					\
 ({												\
@@ -530,7 +611,8 @@ void __drm_err(const char *format, ...);
 	const struct drm_device *drm_ = (drm);							\
 												\
 	if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(&rs_))			\
-		drm_dev_printk(drm_ ? drm_->dev : NULL, KERN_DEBUG, fmt, ## __VA_ARGS__);	\
+		drm_dev_dbg((drm_) ? (drm_)->dev : NULL,					\
+			    DRM_DBG_CAT_ ## category, fmt, ##__VA_ARGS__);			\
 })
 
 #define drm_dbg_kms_ratelimited(drm, fmt, ...) \
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 6/8] drm_print: instrument drm_debug_enabled
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
                   ` (4 preceding siblings ...)
  2021-08-31 20:21 ` [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  2021-09-05 18:50   ` jim.cromie
  2021-08-31 20:21 ` [PATCH v7 7/8] amdgpu_ucode: reduce number of pr_debug calls Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 8/8] nouveau: fold multiple DRM_DEBUG_DRIVERs together Jim Cromie
  7 siblings, 1 reply; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

Duplicate drm_debug_enabled() code into both "basic" and "dyndbg"
ifdef branches.  Then add a pr_debug("todo: ...") into the "dyndbg"
branch.

Then convert the "dyndbg" branch's code to a macro, so that its
pr_debug() get its callsite info from the invoking function, instead
of from drm_debug_enabled() itself.

This gives us unique callsite info for the 8 remaining users of
drm_debug_enabled(), and lets us enable them individually to see how
much logging traffic they generate.  The oft-visited callsites can
then be reviewed for runtime cost and possible optimizations.

Heres what we get:

bash-5.1# modprobe drm
dyndbg: 384 debug prints in module drm
bash-5.1# grep todo: /proc/dynamic_debug/control
drivers/gpu/drm/drm_edid.c:1843 [drm]connector_bad_edid =_ "todo: maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_print.c:309 [drm]___drm_dbg =p "todo: maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_print.c:286 [drm]__drm_dev_dbg =p "todo: maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_vblank.c:1491 [drm]drm_vblank_restore =_ "todo: maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_vblank.c:787 [drm]drm_crtc_vblank_helper_get_vblank_timestamp_internal =_ "todo: maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_vblank.c:410 [drm]drm_crtc_accurate_vblank_count =_ "todo: maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_atomic_uapi.c:1457 [drm]drm_mode_atomic_ioctl =_ "todo: maybe avoid via dyndbg\012"
drivers/gpu/drm/drm_edid_load.c:178 [drm]edid_load =_ "todo: maybe avoid via dyndbg\012"

At quick glance, edid won't qualify, drm_print might, drm_vblank is
strongest chance, maybe atomic-ioctl too.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/drm/drm_print.h | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 973443040561..03f9ae83fade 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -378,6 +378,11 @@ enum drm_debug_category {
 #define DRM_DBG_CAT_DP		DRM_UT_DP
 #define DRM_DBG_CAT_DRMRES	DRM_UT_DRMRES
 
+static inline bool drm_debug_enabled(enum drm_debug_category category)
+{
+	return unlikely(__drm_debug & category);
+}
+
 #else /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
 
 /* join prefix+format in cpp so dyndbg can see it */
@@ -397,12 +402,13 @@ enum drm_debug_category {
 #define DRM_DBG_CAT_DP		"drm:dp: "
 #define DRM_DBG_CAT_DRMRES	"drm:res: " /* not in MODULE_PARM_DESC */
 
-#endif /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
+#define drm_debug_enabled(category)			\
+	({						\
+	pr_debug("todo: maybe avoid via dyndbg\n");	\
+	unlikely(__drm_debug & category);		\
+	})
 
-static inline bool drm_debug_enabled(enum drm_debug_category category)
-{
-	return unlikely(__drm_debug & category);
-}
+#endif /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
 
 /*
  * struct device based logging
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 7/8] amdgpu_ucode: reduce number of pr_debug calls
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
                   ` (5 preceding siblings ...)
  2021-08-31 20:21 ` [PATCH v7 6/8] drm_print: instrument drm_debug_enabled Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  2021-08-31 20:21 ` [PATCH v7 8/8] nouveau: fold multiple DRM_DEBUG_DRIVERs together Jim Cromie
  7 siblings, 0 replies; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

There are blocks of DRM_DEBUG calls, consolidate their args into
single calls.  With dynamic-debug in use, each callsite consumes 56
bytes of callsite data, and this patch removes about 65 calls, so
it saves ~3.5kb.

no functional changes.

RFC: this creates multi-line log messages, does that break any syslog
conventions ?

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 293 ++++++++++++----------
 1 file changed, 158 insertions(+), 135 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
index 2834981f8c08..14a9fef1f4c6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
@@ -30,17 +30,26 @@
 
 static void amdgpu_ucode_print_common_hdr(const struct common_firmware_header *hdr)
 {
-	DRM_DEBUG("size_bytes: %u\n", le32_to_cpu(hdr->size_bytes));
-	DRM_DEBUG("header_size_bytes: %u\n", le32_to_cpu(hdr->header_size_bytes));
-	DRM_DEBUG("header_version_major: %u\n", le16_to_cpu(hdr->header_version_major));
-	DRM_DEBUG("header_version_minor: %u\n", le16_to_cpu(hdr->header_version_minor));
-	DRM_DEBUG("ip_version_major: %u\n", le16_to_cpu(hdr->ip_version_major));
-	DRM_DEBUG("ip_version_minor: %u\n", le16_to_cpu(hdr->ip_version_minor));
-	DRM_DEBUG("ucode_version: 0x%08x\n", le32_to_cpu(hdr->ucode_version));
-	DRM_DEBUG("ucode_size_bytes: %u\n", le32_to_cpu(hdr->ucode_size_bytes));
-	DRM_DEBUG("ucode_array_offset_bytes: %u\n",
-		  le32_to_cpu(hdr->ucode_array_offset_bytes));
-	DRM_DEBUG("crc32: 0x%08x\n", le32_to_cpu(hdr->crc32));
+	DRM_DEBUG("size_bytes: %u\n"
+		  "header_size_bytes: %u\n"
+		  "header_version_major: %u\n"
+		  "header_version_minor: %u\n"
+		  "ip_version_major: %u\n"
+		  "ip_version_minor: %u\n"
+		  "ucode_version: 0x%08x\n"
+		  "ucode_size_bytes: %u\n"
+		  "ucode_array_offset_bytes: %u\n"
+		  "crc32: 0x%08x\n",
+		  le32_to_cpu(hdr->size_bytes),
+		  le32_to_cpu(hdr->header_size_bytes),
+		  le16_to_cpu(hdr->header_version_major),
+		  le16_to_cpu(hdr->header_version_minor),
+		  le16_to_cpu(hdr->ip_version_major),
+		  le16_to_cpu(hdr->ip_version_minor),
+		  le32_to_cpu(hdr->ucode_version),
+		  le32_to_cpu(hdr->ucode_size_bytes),
+		  le32_to_cpu(hdr->ucode_array_offset_bytes),
+		  le32_to_cpu(hdr->crc32));
 }
 
 void amdgpu_ucode_print_mc_hdr(const struct common_firmware_header *hdr)
@@ -55,9 +64,9 @@ void amdgpu_ucode_print_mc_hdr(const struct common_firmware_header *hdr)
 		const struct mc_firmware_header_v1_0 *mc_hdr =
 			container_of(hdr, struct mc_firmware_header_v1_0, header);
 
-		DRM_DEBUG("io_debug_size_bytes: %u\n",
-			  le32_to_cpu(mc_hdr->io_debug_size_bytes));
-		DRM_DEBUG("io_debug_array_offset_bytes: %u\n",
+		DRM_DEBUG("io_debug_size_bytes: %u\n"
+			  "io_debug_array_offset_bytes: %u\n",
+			  le32_to_cpu(mc_hdr->io_debug_size_bytes),
 			  le32_to_cpu(mc_hdr->io_debug_array_offset_bytes));
 	} else {
 		DRM_ERROR("Unknown MC ucode version: %u.%u\n", version_major, version_minor);
@@ -82,13 +91,17 @@ void amdgpu_ucode_print_smc_hdr(const struct common_firmware_header *hdr)
 		switch (version_minor) {
 		case 0:
 			v2_0_hdr = container_of(hdr, struct smc_firmware_header_v2_0, v1_0.header);
-			DRM_DEBUG("ppt_offset_bytes: %u\n", le32_to_cpu(v2_0_hdr->ppt_offset_bytes));
-			DRM_DEBUG("ppt_size_bytes: %u\n", le32_to_cpu(v2_0_hdr->ppt_size_bytes));
+			DRM_DEBUG("ppt_offset_bytes: %u\n"
+				  "ppt_size_bytes: %u\n",
+				  le32_to_cpu(v2_0_hdr->ppt_offset_bytes),
+				  le32_to_cpu(v2_0_hdr->ppt_size_bytes));
 			break;
 		case 1:
 			v2_1_hdr = container_of(hdr, struct smc_firmware_header_v2_1, v1_0.header);
-			DRM_DEBUG("pptable_count: %u\n", le32_to_cpu(v2_1_hdr->pptable_count));
-			DRM_DEBUG("pptable_entry_offset: %u\n", le32_to_cpu(v2_1_hdr->pptable_entry_offset));
+			DRM_DEBUG("pptable_count: %u\n"
+				  "pptable_entry_offset: %u\n",
+				  le32_to_cpu(v2_1_hdr->pptable_count),
+				  le32_to_cpu(v2_1_hdr->pptable_entry_offset));
 			break;
 		default:
 			break;
@@ -111,10 +124,12 @@ void amdgpu_ucode_print_gfx_hdr(const struct common_firmware_header *hdr)
 		const struct gfx_firmware_header_v1_0 *gfx_hdr =
 			container_of(hdr, struct gfx_firmware_header_v1_0, header);
 
-		DRM_DEBUG("ucode_feature_version: %u\n",
-			  le32_to_cpu(gfx_hdr->ucode_feature_version));
-		DRM_DEBUG("jt_offset: %u\n", le32_to_cpu(gfx_hdr->jt_offset));
-		DRM_DEBUG("jt_size: %u\n", le32_to_cpu(gfx_hdr->jt_size));
+		DRM_DEBUG("ucode_feature_version: %u\n"
+			  "jt_offset: %u\n"
+			  "jt_size: %u\n",
+			  le32_to_cpu(gfx_hdr->ucode_feature_version),
+			  le32_to_cpu(gfx_hdr->jt_offset),
+			  le32_to_cpu(gfx_hdr->jt_size));
 	} else {
 		DRM_ERROR("Unknown GFX ucode version: %u.%u\n", version_major, version_minor);
 	}
@@ -132,82 +147,88 @@ void amdgpu_ucode_print_rlc_hdr(const struct common_firmware_header *hdr)
 		const struct rlc_firmware_header_v1_0 *rlc_hdr =
 			container_of(hdr, struct rlc_firmware_header_v1_0, header);
 
-		DRM_DEBUG("ucode_feature_version: %u\n",
-			  le32_to_cpu(rlc_hdr->ucode_feature_version));
-		DRM_DEBUG("save_and_restore_offset: %u\n",
-			  le32_to_cpu(rlc_hdr->save_and_restore_offset));
-		DRM_DEBUG("clear_state_descriptor_offset: %u\n",
-			  le32_to_cpu(rlc_hdr->clear_state_descriptor_offset));
-		DRM_DEBUG("avail_scratch_ram_locations: %u\n",
-			  le32_to_cpu(rlc_hdr->avail_scratch_ram_locations));
-		DRM_DEBUG("master_pkt_description_offset: %u\n",
+		DRM_DEBUG("ucode_feature_version: %u\n"
+			  "save_and_restore_offset: %u\n"
+			  "clear_state_descriptor_offset: %u\n"
+			  "avail_scratch_ram_locations: %u\n"
+			  "master_pkt_description_offset: %u\n",
+			  le32_to_cpu(rlc_hdr->ucode_feature_version),
+			  le32_to_cpu(rlc_hdr->save_and_restore_offset),
+			  le32_to_cpu(rlc_hdr->clear_state_descriptor_offset),
+			  le32_to_cpu(rlc_hdr->avail_scratch_ram_locations),
 			  le32_to_cpu(rlc_hdr->master_pkt_description_offset));
+
 	} else if (version_major == 2) {
 		const struct rlc_firmware_header_v2_0 *rlc_hdr =
 			container_of(hdr, struct rlc_firmware_header_v2_0, header);
 
-		DRM_DEBUG("ucode_feature_version: %u\n",
-			  le32_to_cpu(rlc_hdr->ucode_feature_version));
-		DRM_DEBUG("jt_offset: %u\n", le32_to_cpu(rlc_hdr->jt_offset));
-		DRM_DEBUG("jt_size: %u\n", le32_to_cpu(rlc_hdr->jt_size));
-		DRM_DEBUG("save_and_restore_offset: %u\n",
-			  le32_to_cpu(rlc_hdr->save_and_restore_offset));
-		DRM_DEBUG("clear_state_descriptor_offset: %u\n",
-			  le32_to_cpu(rlc_hdr->clear_state_descriptor_offset));
-		DRM_DEBUG("avail_scratch_ram_locations: %u\n",
-			  le32_to_cpu(rlc_hdr->avail_scratch_ram_locations));
-		DRM_DEBUG("reg_restore_list_size: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_restore_list_size));
-		DRM_DEBUG("reg_list_format_start: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_format_start));
-		DRM_DEBUG("reg_list_format_separate_start: %u\n",
+		DRM_DEBUG("ucode_feature_version: %u\n"
+			  "jt_offset: %u\n"
+			  "jt_size: %u\n"
+			  "save_and_restore_offset: %u\n"
+			  "clear_state_descriptor_offset: %u\n"
+			  "avail_scratch_ram_locations: %u\n"
+			  "reg_restore_list_size: %u\n"
+			  "reg_list_format_start: %u\n"
+			  "reg_list_format_separate_start: %u\n",
+			  le32_to_cpu(rlc_hdr->ucode_feature_version),
+			  le32_to_cpu(rlc_hdr->jt_offset),
+			  le32_to_cpu(rlc_hdr->jt_size),
+			  le32_to_cpu(rlc_hdr->save_and_restore_offset),
+			  le32_to_cpu(rlc_hdr->clear_state_descriptor_offset),
+			  le32_to_cpu(rlc_hdr->avail_scratch_ram_locations),
+			  le32_to_cpu(rlc_hdr->reg_restore_list_size),
+			  le32_to_cpu(rlc_hdr->reg_list_format_start),
 			  le32_to_cpu(rlc_hdr->reg_list_format_separate_start));
-		DRM_DEBUG("starting_offsets_start: %u\n",
-			  le32_to_cpu(rlc_hdr->starting_offsets_start));
-		DRM_DEBUG("reg_list_format_size_bytes: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_format_size_bytes));
-		DRM_DEBUG("reg_list_format_array_offset_bytes: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_format_array_offset_bytes));
-		DRM_DEBUG("reg_list_size_bytes: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_size_bytes));
-		DRM_DEBUG("reg_list_array_offset_bytes: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_array_offset_bytes));
-		DRM_DEBUG("reg_list_format_separate_size_bytes: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_format_separate_size_bytes));
-		DRM_DEBUG("reg_list_format_separate_array_offset_bytes: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_format_separate_array_offset_bytes));
-		DRM_DEBUG("reg_list_separate_size_bytes: %u\n",
-			  le32_to_cpu(rlc_hdr->reg_list_separate_size_bytes));
-		DRM_DEBUG("reg_list_separate_array_offset_bytes: %u\n",
+
+		DRM_DEBUG("starting_offsets_start: %u\n"
+			  "reg_list_format_size_bytes: %u\n"
+			  "reg_list_format_array_offset_bytes: %u\n"
+			  "reg_list_size_bytes: %u\n"
+			  "reg_list_array_offset_bytes: %u\n"
+			  "reg_list_format_separate_size_bytes: %u\n"
+			  "reg_list_format_separate_array_offset_bytes: %u\n"
+			  "reg_list_separate_size_bytes: %u\n"
+			  "reg_list_separate_array_offset_bytes: %u\n",
+			  le32_to_cpu(rlc_hdr->starting_offsets_start),
+			  le32_to_cpu(rlc_hdr->reg_list_format_size_bytes),
+			  le32_to_cpu(rlc_hdr->reg_list_format_array_offset_bytes),
+			  le32_to_cpu(rlc_hdr->reg_list_size_bytes),
+			  le32_to_cpu(rlc_hdr->reg_list_array_offset_bytes),
+			  le32_to_cpu(rlc_hdr->reg_list_format_separate_size_bytes),
+			  le32_to_cpu(rlc_hdr->reg_list_format_separate_array_offset_bytes),
+			  le32_to_cpu(rlc_hdr->reg_list_separate_size_bytes),
 			  le32_to_cpu(rlc_hdr->reg_list_separate_array_offset_bytes));
+
 		if (version_minor == 1) {
 			const struct rlc_firmware_header_v2_1 *v2_1 =
 				container_of(rlc_hdr, struct rlc_firmware_header_v2_1, v2_0);
-			DRM_DEBUG("reg_list_format_direct_reg_list_length: %u\n",
-				  le32_to_cpu(v2_1->reg_list_format_direct_reg_list_length));
-			DRM_DEBUG("save_restore_list_cntl_ucode_ver: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_cntl_ucode_ver));
-			DRM_DEBUG("save_restore_list_cntl_feature_ver: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_cntl_feature_ver));
-			DRM_DEBUG("save_restore_list_cntl_size_bytes %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_cntl_size_bytes));
-			DRM_DEBUG("save_restore_list_cntl_offset_bytes: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_cntl_offset_bytes));
-			DRM_DEBUG("save_restore_list_gpm_ucode_ver: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_gpm_ucode_ver));
-			DRM_DEBUG("save_restore_list_gpm_feature_ver: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_gpm_feature_ver));
-			DRM_DEBUG("save_restore_list_gpm_size_bytes %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_gpm_size_bytes));
-			DRM_DEBUG("save_restore_list_gpm_offset_bytes: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_gpm_offset_bytes));
-			DRM_DEBUG("save_restore_list_srm_ucode_ver: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_srm_ucode_ver));
-			DRM_DEBUG("save_restore_list_srm_feature_ver: %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_srm_feature_ver));
-			DRM_DEBUG("save_restore_list_srm_size_bytes %u\n",
-				  le32_to_cpu(v2_1->save_restore_list_srm_size_bytes));
-			DRM_DEBUG("save_restore_list_srm_offset_bytes: %u\n",
+
+			DRM_DEBUG("reg_list_format_direct_reg_list_length: %u\n"
+				  "save_restore_list_cntl_ucode_ver: %u\n"
+				  "save_restore_list_cntl_feature_ver: %u\n"
+				  "save_restore_list_cntl_size_bytes %u\n"
+				  "save_restore_list_cntl_offset_bytes: %u\n"
+				  "save_restore_list_gpm_ucode_ver: %u\n"
+				  "save_restore_list_gpm_feature_ver: %u\n"
+				  "save_restore_list_gpm_size_bytes %u\n"
+				  "save_restore_list_gpm_offset_bytes: %u\n"
+				  "save_restore_list_srm_ucode_ver: %u\n"
+				  "save_restore_list_srm_feature_ver: %u\n"
+				  "save_restore_list_srm_size_bytes %u\n"
+				  "save_restore_list_srm_offset_bytes: %u\n",
+				  le32_to_cpu(v2_1->reg_list_format_direct_reg_list_length),
+				  le32_to_cpu(v2_1->save_restore_list_cntl_ucode_ver),
+				  le32_to_cpu(v2_1->save_restore_list_cntl_feature_ver),
+				  le32_to_cpu(v2_1->save_restore_list_cntl_size_bytes),
+				  le32_to_cpu(v2_1->save_restore_list_cntl_offset_bytes),
+				  le32_to_cpu(v2_1->save_restore_list_gpm_ucode_ver),
+				  le32_to_cpu(v2_1->save_restore_list_gpm_feature_ver),
+				  le32_to_cpu(v2_1->save_restore_list_gpm_size_bytes),
+				  le32_to_cpu(v2_1->save_restore_list_gpm_offset_bytes),
+				  le32_to_cpu(v2_1->save_restore_list_srm_ucode_ver),
+				  le32_to_cpu(v2_1->save_restore_list_srm_feature_ver),
+				  le32_to_cpu(v2_1->save_restore_list_srm_size_bytes),
 				  le32_to_cpu(v2_1->save_restore_list_srm_offset_bytes));
 		}
 	} else {
@@ -227,12 +248,14 @@ void amdgpu_ucode_print_sdma_hdr(const struct common_firmware_header *hdr)
 		const struct sdma_firmware_header_v1_0 *sdma_hdr =
 			container_of(hdr, struct sdma_firmware_header_v1_0, header);
 
-		DRM_DEBUG("ucode_feature_version: %u\n",
-			  le32_to_cpu(sdma_hdr->ucode_feature_version));
-		DRM_DEBUG("ucode_change_version: %u\n",
-			  le32_to_cpu(sdma_hdr->ucode_change_version));
-		DRM_DEBUG("jt_offset: %u\n", le32_to_cpu(sdma_hdr->jt_offset));
-		DRM_DEBUG("jt_size: %u\n", le32_to_cpu(sdma_hdr->jt_size));
+		DRM_DEBUG("ucode_feature_version: %u\n"
+			  "ucode_change_version: %u\n"
+			  "jt_offset: %u\n"
+			  "jt_size: %u\n",
+			  le32_to_cpu(sdma_hdr->ucode_feature_version),
+			  le32_to_cpu(sdma_hdr->ucode_change_version),
+			  le32_to_cpu(sdma_hdr->jt_offset),
+			  le32_to_cpu(sdma_hdr->jt_size));
 		if (version_minor >= 1) {
 			const struct sdma_firmware_header_v1_1 *sdma_v1_1_hdr =
 				container_of(sdma_hdr, struct sdma_firmware_header_v1_1, v1_0);
@@ -256,36 +279,36 @@ void amdgpu_ucode_print_psp_hdr(const struct common_firmware_header *hdr)
 		const struct psp_firmware_header_v1_0 *psp_hdr =
 			container_of(hdr, struct psp_firmware_header_v1_0, header);
 
-		DRM_DEBUG("ucode_feature_version: %u\n",
-			  le32_to_cpu(psp_hdr->sos.fw_version));
-		DRM_DEBUG("sos_offset_bytes: %u\n",
-			  le32_to_cpu(psp_hdr->sos.offset_bytes));
-		DRM_DEBUG("sos_size_bytes: %u\n",
+		DRM_DEBUG("ucode_feature_version: %u\n"
+			  "sos_offset_bytes: %u\n"
+			  "sos_size_bytes: %u\n",
+			  le32_to_cpu(psp_hdr->sos.fw_version),
+			  le32_to_cpu(psp_hdr->sos.offset_bytes),
 			  le32_to_cpu(psp_hdr->sos.size_bytes));
 		if (version_minor == 1) {
 			const struct psp_firmware_header_v1_1 *psp_hdr_v1_1 =
 				container_of(psp_hdr, struct psp_firmware_header_v1_1, v1_0);
-			DRM_DEBUG("toc_header_version: %u\n",
-				  le32_to_cpu(psp_hdr_v1_1->toc.fw_version));
-			DRM_DEBUG("toc_offset_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_1->toc.offset_bytes));
-			DRM_DEBUG("toc_size_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_1->toc.size_bytes));
-			DRM_DEBUG("kdb_header_version: %u\n",
-				  le32_to_cpu(psp_hdr_v1_1->kdb.fw_version));
-			DRM_DEBUG("kdb_offset_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_1->kdb.offset_bytes));
-			DRM_DEBUG("kdb_size_bytes: %u\n",
+			DRM_DEBUG("toc_header_version: %u\n"
+				  "toc_offset_bytes: %u\n"
+				  "toc_size_bytes: %u\n"
+				  "kdb_header_version: %u\n"
+				  "kdb_offset_bytes: %u\n"
+				  "kdb_size_bytes: %u\n",
+				  le32_to_cpu(psp_hdr_v1_1->toc.fw_version),
+				  le32_to_cpu(psp_hdr_v1_1->toc.offset_bytes),
+				  le32_to_cpu(psp_hdr_v1_1->toc.size_bytes),
+				  le32_to_cpu(psp_hdr_v1_1->kdb.fw_version),
+				  le32_to_cpu(psp_hdr_v1_1->kdb.offset_bytes),
 				  le32_to_cpu(psp_hdr_v1_1->kdb.size_bytes));
 		}
 		if (version_minor == 2) {
 			const struct psp_firmware_header_v1_2 *psp_hdr_v1_2 =
 				container_of(psp_hdr, struct psp_firmware_header_v1_2, v1_0);
-			DRM_DEBUG("kdb_header_version: %u\n",
-				  le32_to_cpu(psp_hdr_v1_2->kdb.fw_version));
-			DRM_DEBUG("kdb_offset_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_2->kdb.offset_bytes));
-			DRM_DEBUG("kdb_size_bytes: %u\n",
+			DRM_DEBUG("kdb_header_version: %u\n"
+				  "kdb_offset_bytes: %u\n"
+				  "kdb_size_bytes: %u\n",
+				  le32_to_cpu(psp_hdr_v1_2->kdb.fw_version),
+				  le32_to_cpu(psp_hdr_v1_2->kdb.offset_bytes),
 				  le32_to_cpu(psp_hdr_v1_2->kdb.size_bytes));
 		}
 		if (version_minor == 3) {
@@ -293,23 +316,23 @@ void amdgpu_ucode_print_psp_hdr(const struct common_firmware_header *hdr)
 				container_of(psp_hdr, struct psp_firmware_header_v1_1, v1_0);
 			const struct psp_firmware_header_v1_3 *psp_hdr_v1_3 =
 				container_of(psp_hdr_v1_1, struct psp_firmware_header_v1_3, v1_1);
-			DRM_DEBUG("toc_header_version: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->v1_1.toc.fw_version));
-			DRM_DEBUG("toc_offset_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->v1_1.toc.offset_bytes));
-			DRM_DEBUG("toc_size_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->v1_1.toc.size_bytes));
-			DRM_DEBUG("kdb_header_version: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->v1_1.kdb.fw_version));
-			DRM_DEBUG("kdb_offset_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->v1_1.kdb.offset_bytes));
-			DRM_DEBUG("kdb_size_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->v1_1.kdb.size_bytes));
-			DRM_DEBUG("spl_header_version: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->spl.fw_version));
-			DRM_DEBUG("spl_offset_bytes: %u\n",
-				  le32_to_cpu(psp_hdr_v1_3->spl.offset_bytes));
-			DRM_DEBUG("spl_size_bytes: %u\n",
+			DRM_DEBUG("toc_header_version: %u\n"
+				  "toc_offset_bytes: %u\n"
+				  "toc_size_bytes: %u\n"
+				  "kdb_header_version: %u\n"
+				  "kdb_offset_bytes: %u\n"
+				  "kdb_size_bytes: %u\n"
+				  "spl_header_version: %u\n"
+				  "spl_offset_bytes: %u\n"
+				  "spl_size_bytes: %u\n",
+				  le32_to_cpu(psp_hdr_v1_3->v1_1.toc.fw_version),
+				  le32_to_cpu(psp_hdr_v1_3->v1_1.toc.offset_bytes),
+				  le32_to_cpu(psp_hdr_v1_3->v1_1.toc.size_bytes),
+				  le32_to_cpu(psp_hdr_v1_3->v1_1.kdb.fw_version),
+				  le32_to_cpu(psp_hdr_v1_3->v1_1.kdb.offset_bytes),
+				  le32_to_cpu(psp_hdr_v1_3->v1_1.kdb.size_bytes),
+				  le32_to_cpu(psp_hdr_v1_3->spl.fw_version),
+				  le32_to_cpu(psp_hdr_v1_3->spl.offset_bytes),
 				  le32_to_cpu(psp_hdr_v1_3->spl.size_bytes));
 		}
 	} else {
@@ -330,9 +353,9 @@ void amdgpu_ucode_print_gpu_info_hdr(const struct common_firmware_header *hdr)
 		const struct gpu_info_firmware_header_v1_0 *gpu_info_hdr =
 			container_of(hdr, struct gpu_info_firmware_header_v1_0, header);
 
-		DRM_DEBUG("version_major: %u\n",
-			  le16_to_cpu(gpu_info_hdr->version_major));
-		DRM_DEBUG("version_minor: %u\n",
+		DRM_DEBUG("version_major: %u\n"
+			  "version_minor: %u\n",
+			  le16_to_cpu(gpu_info_hdr->version_major),
 			  le16_to_cpu(gpu_info_hdr->version_minor));
 	} else {
 		DRM_ERROR("Unknown gpu_info ucode version: %u.%u\n", version_major, version_minor);
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 8/8] nouveau: fold multiple DRM_DEBUG_DRIVERs together
  2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
                   ` (6 preceding siblings ...)
  2021-08-31 20:21 ` [PATCH v7 7/8] amdgpu_ucode: reduce number of pr_debug calls Jim Cromie
@ 2021-08-31 20:21 ` Jim Cromie
  7 siblings, 0 replies; 19+ messages in thread
From: Jim Cromie @ 2021-08-31 20:21 UTC (permalink / raw)
  To: jbaron, gregkh, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx
  Cc: Jim Cromie

With DRM_USE_DYNAMIC_DEBUG, each callsite record requires 56 bytes.
We can combine 12 into one here and save ~620 bytes.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 36 +++++++++++++++++----------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index ba4cd5f83725..0f45399535bf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1245,19 +1245,29 @@ nouveau_drm_pci_table[] = {
 
 static void nouveau_display_options(void)
 {
-	DRM_DEBUG_DRIVER("Loading Nouveau with parameters:\n");
-
-	DRM_DEBUG_DRIVER("... tv_disable   : %d\n", nouveau_tv_disable);
-	DRM_DEBUG_DRIVER("... ignorelid    : %d\n", nouveau_ignorelid);
-	DRM_DEBUG_DRIVER("... duallink     : %d\n", nouveau_duallink);
-	DRM_DEBUG_DRIVER("... nofbaccel    : %d\n", nouveau_nofbaccel);
-	DRM_DEBUG_DRIVER("... config       : %s\n", nouveau_config);
-	DRM_DEBUG_DRIVER("... debug        : %s\n", nouveau_debug);
-	DRM_DEBUG_DRIVER("... noaccel      : %d\n", nouveau_noaccel);
-	DRM_DEBUG_DRIVER("... modeset      : %d\n", nouveau_modeset);
-	DRM_DEBUG_DRIVER("... runpm        : %d\n", nouveau_runtime_pm);
-	DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf);
-	DRM_DEBUG_DRIVER("... hdmimhz      : %d\n", nouveau_hdmimhz);
+	DRM_DEBUG_DRIVER("Loading Nouveau with parameters:\n"
+			 "... tv_disable   : %d\n"
+			 "... ignorelid    : %d\n"
+			 "... duallink     : %d\n"
+			 "... nofbaccel    : %d\n"
+			 "... config       : %s\n"
+			 "... debug        : %s\n"
+			 "... noaccel      : %d\n"
+			 "... modeset      : %d\n"
+			 "... runpm        : %d\n"
+			 "... vram_pushbuf : %d\n"
+			 "... hdmimhz      : %d\n"
+			 , nouveau_tv_disable
+			 , nouveau_ignorelid
+			 , nouveau_duallink
+			 , nouveau_nofbaccel
+			 , nouveau_config
+			 , nouveau_debug
+			 , nouveau_noaccel
+			 , nouveau_modeset
+			 , nouveau_runtime_pm
+			 , nouveau_vram_pushbuf
+			 , nouveau_hdmimhz);
 }
 
 static const struct dev_pm_ops nouveau_pm_ops = {
-- 
2.31.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
  2021-08-31 20:21 ` [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
@ 2021-09-03 11:07   ` Tvrtko Ursulin
  2021-09-03 19:22     ` jim.cromie
  0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2021-09-03 11:07 UTC (permalink / raw)
  To: Jim Cromie, jbaron, gregkh, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx


On 31/08/2021 21:21, Jim Cromie wrote:
> The gvt component of this driver has ~120 pr_debugs, in 9 categories
> quite similar to those in DRM.  Following the interface model of
> drm.debug, add a parameter to map bits to these categorizations.
> 
> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
> 	"dyndbg bitmap desc",
> 	{ "gvt:cmd: ",  "command processing" },
> 	{ "gvt:core: ", "core help" },
> 	{ "gvt:dpy: ",  "display help" },
> 	{ "gvt:el: ",   "help" },
> 	{ "gvt:irq: ",  "help" },
> 	{ "gvt:mm: ",   "help" },
> 	{ "gvt:mmio: ", "help" },
> 	{ "gvt:render: ", "help" },
> 	{ "gvt:sched: " "help" });
> 
> The actual patch has a few details different, cmd_help() macro emits
> the initialization construct.
> 
> if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added
> cflags, by gvt/Makefile.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
> v5:
> . static decl of vector of bit->class descriptors - Emil.V
> . relocate gvt-makefile chunk from elsewhere
> v7:
> . move ccflags addition up to i915/Makefile from i915/gvt
> ---
>   drivers/gpu/drm/i915/Makefile      |  4 ++++
>   drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++

Can this work if put under gvt/ or at least intel_gvt.h|c?

>   2 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 4f22cac1c49b..5a4e371a3ec2 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init)
>   
>   subdir-ccflags-y += -I$(srctree)/$(src)
>   
> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
> +ccflags-y += -DDYNAMIC_DEBUG_MODULE
> +#endif

Ignores whether CONFIG_DRM_I915_GVT is enabled or not?

> +
>   # Please keep these build lists sorted!
>   
>   # core driver code
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index e07f4cfea63a..e645e149485e 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params)
>   	I915_PARAMS_FOR_EACH(FREE);
>   #undef FREE
>   }
> +
> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
> +/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */
> +
> +unsigned long __gvt_debug;
> +EXPORT_SYMBOL(__gvt_debug);
> +
> +#define _help(key)	"\t    \"" key "\"\t: help for " key "\n"
> +
> +#define I915_GVT_CATEGORIES(name) \
> +	" Enable debug output via /sys/module/i915/parameters/" #name	\
> +	", where each bit enables a debug category.\n"			\
> +	_help("gvt:cmd:")						\
> +	_help("gvt:core:")						\
> +	_help("gvt:dpy:")						\
> +	_help("gvt:el:")						\
> +	_help("gvt:irq:")						\
> +	_help("gvt:mm:")						\
> +	_help("gvt:mmio:")						\
> +	_help("gvt:render:")						\
> +	_help("gvt:sched:")
> +
> +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
> +				I915_GVT_CATEGORIES(debug_gvt),
> +				_DD_cat_("gvt:cmd:"),
> +				_DD_cat_("gvt:core:"),
> +				_DD_cat_("gvt:dpy:"),
> +				_DD_cat_("gvt:el:"),
> +				_DD_cat_("gvt:irq:"),
> +				_DD_cat_("gvt:mm:"),
> +				_DD_cat_("gvt:mmio:"),
> +				_DD_cat_("gvt:render:"),
> +				_DD_cat_("gvt:sched:"));
> +
> +#endif

So just the foundation - no actual use sites I mean? How would these be 
used from the code?

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug
  2021-08-31 20:21 ` [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
@ 2021-09-03 11:15   ` Tvrtko Ursulin
  2021-09-03 21:57     ` jim.cromie
  0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2021-09-03 11:15 UTC (permalink / raw)
  To: Jim Cromie, jbaron, gregkh, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx


On 31/08/2021 21:21, Jim Cromie wrote:
> 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 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.

Probably stupid question - enabling stuff at boot time still works as 
described in Documentation/admin-guide/dynamic-debug-howto.rst?

Second question, which perhaps has been covered in the past so apologies 
if redundant - what is the advantage of allowing this to be 
configurable, versus perhaps always enabling it? Like what would be the 
reasons someone wouldn't just want to have CONFIG_DYNAMIC_DEBUG compiled 
in? Kernel binary size?

Regards,

Tvrtko

> 
> This conversion yields many new prdbg callsites:
> 
>    dyndbg: 195 debug prints in module drm_kms_helper
>    dyndbg: 298 debug prints in module drm
>    dyndbg: 1630 debug prints in module i915
>    dyndbg: ~3500 debug prints in module amdgpu
> 
> CONFIG_DRM_USE_DYNAMIC_DEBUG enables this, and is available if
> CONFIG_DYNAMIC_DEBUG or CONFIG_DYNAMIC_DEBUG_CORE is chosen, and if
> CONFIG_JUMP_LABEL is enabled; this because its 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: "
> 
> In v3, had older name, DRM_DBG_CLASS_<CATs> was countered, I had
> agreed, but this seems better still; CATEGORY is already DRM's
> term-of-art, and adding a near-synonym 'CLASS' only adds ambiguity.
> 
> DRM_UT_* are preserved, since theyre used elsewhere.  We can probably
> reduce their use further, but thats a separate thing.
> 
> 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
> 
> these already use (B), now they use (A) too, 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.
> 
> NOTES:
> 
> Because the dyndbg callback is watching __drm_debug, it is coherent
> 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 search-prefixes/categories with a trailing space, which
> excludes any sub-categories 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
> 
> With "lineno X" in a query, its possible to enable single callsites,
> but it is tedious, and useless in a category context.
> 
> Unfortunately __func__ is not a macro, and cannot be catenated at
> preprocess/compile time.
> 
> 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 KBuild 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
> ---
>   drivers/gpu/drm/Kconfig     |  13 ++++
>   drivers/gpu/drm/Makefile    |   3 +
>   drivers/gpu/drm/drm_print.c |  53 +++++++++----
>   include/drm/drm_print.h     | 144 ++++++++++++++++++++++++++++--------
>   4 files changed, 166 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 7ff89690a976..97e38d86fd27 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -57,6 +57,19 @@ 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 patches pr_debug()s in/out
> +	  of the running kernel, so avoids those bit-test overheads,
> +	  and is therefore recommended.
> +
>   config DRM_DEBUG_SELFTEST
>   	tristate "kselftests for DRM"
>   	depends on DRM
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index a118692a6df7..1809329654b3 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -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
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index 111b932cf2a9..df2e10754c41 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -28,6 +28,7 @@
>   #include <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,39 @@
>    * __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)."
> +
> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
> +#include <linux/dynamic_debug.h>
> +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
> +	DRM_DEBUG_DESC,
> +	_DD_cat_(DRM_DBG_CAT_CORE),
> +	_DD_cat_(DRM_DBG_CAT_DRIVER),
> +	_DD_cat_(DRM_DBG_CAT_KMS),
> +	_DD_cat_(DRM_DBG_CAT_PRIME),
> +	_DD_cat_(DRM_DBG_CAT_ATOMIC),
> +	_DD_cat_(DRM_DBG_CAT_VBL),
> +	_DD_cat_(DRM_DBG_CAT_STATE),
> +	_DD_cat_(DRM_DBG_CAT_LEASE),
> +	_DD_cat_(DRM_DBG_CAT_DP),
> +	_DD_cat_(DRM_DBG_CAT_DRMRES));
> +
> +#else
> +MODULE_PARM_DESC(debug, DRM_DEBUG_DESC);
> +module_param_named(debug, __drm_debug, ulong, 0600);
> +#endif
>   
>   void __drm_puts_coredump(struct drm_printer *p, const char *str)
>   {
> @@ -256,8 +277,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 +299,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 +318,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, ...)
>   {
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index 9b66be54dd16..973443040561 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -35,7 +35,7 @@
>   #include <drm/drm.h>
>   
>   /* Do *not* use outside of drm_print.[ch]! */
> -extern unsigned int __drm_debug;
> +extern unsigned long __drm_debug;
>   
>   /**
>    * DOC: print
> @@ -252,15 +252,15 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
>   /**
>    * enum drm_debug_category - The DRM debug categories
>    *
> - * Each of the DRM debug logging macros use a specific category, and the logging
> - * is filtered by the drm.debug module parameter. This enum specifies the values
> - * for the interface.
> + * The drm.debug logging API[1] has 10 enumerated categories of
> + * messages, issued by 3 families of macros: 10 drm_dbg_<CATs>, 8
> + * DRM_DEBUG_<CATs>, and 3 DRM_DEV_DEBUG_<CATs>.
>    *
>    * Each DRM_DEBUG_<CATEGORY> macro logs to DRM_UT_<CATEGORY> category, except
>    * DRM_DEBUG() logs to DRM_UT_CORE.
>    *
> - * Enabling verbose debug messages is done through the drm.debug parameter, each
> - * category being enabled by a bit:
> + * Enabling categories of debug messages is done through the drm.debug
> + * parameter, each category being enabled by a bit:
>    *
>    *  - drm.debug=0x1 will enable CORE messages
>    *  - drm.debug=0x2 will enable DRIVER messages
> @@ -319,6 +319,86 @@ enum drm_debug_category {
>   	DRM_UT_DRMRES		= 0x200,
>   };
>   
> +/**
> + * DOC: DRM_USE_DYNAMIC_DEBUG - using dyndbg in drm.debug
> + *
> + * In the "basic" drm.debug implementation outlined above, each time a
> + * drm-debug API[1] call is executed, drm_debug_enabled(cat) tests
> + * drm.debug vs cat before printing.
> + *
> + * DYNAMIC_DEBUG (aka: dyndbg) patches pr_debug()s in^out of the
> + * running kernel, so it can avoid drm_debug_enabled() and skip lots
> + * of unlikely bit tests.
> + *
> + * dyndbg has no concept of category, but we can prepend a
> + * class-prefix string: "drm:core: ", "drm:kms: ", "drm:driver: " etc,
> + * to pr_debug's format (at compile time).
> + *
> + * Then control the category
> + *    `echo module drm format "^drm:core: " +p > control`
> + *    `dynamic_debug_exec_queries("format '^drm:core: ' +p", "drm");`
> + *
> + * To do this for "basic" | "dyndbg", adaptation adds some macro indirection:
> + *
> + * 0. use dyndbg support to define the bits => prefixes map, attach callback.
> + *
> + *    DYNDBG_BITMAP_DESC(debug, __drm_debug,
> + *			 "drm.debug - overview",
> + *			 { "drm:core:", "enable CORE debug messages" },
> + *			 { "drm:kms:", "enable KMS debug messages" }, ...);
> + *
> + * 1. DRM_DBG_CAT_<CAT>
> + *
> + * This set of symbols replaces DRM_UT_<CAT> in the drm-debug API; it
> + * is either a copy of DRM_UT_<CAT>, or the class-prefix strings.
> + *
> + * 2. drm_dev_dbg & drm_debug are called by drm.debug API
> + *
> + * These are now macros, either forwarding to renamed functions, or
> + * prepending the class string to the format, and invoking pr_debug
> + * directly.  Since the API is all macros, dyndbg remembers
> + * per-pr_debug: module,file,func,line,format and uses that to find
> + * and enable them.
> + */
> +#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
> +
> +#define __drm_dbg(cls, fmt, ...)			\
> +	___drm_dbg(cls, fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg(dev, cls, fmt, ...)			\
> +	__drm_dev_dbg(dev, cls, fmt, ##__VA_ARGS__)
> +
> +#define DRM_DBG_CAT_CORE	DRM_UT_CORE
> +#define DRM_DBG_CAT_DRIVER	DRM_UT_DRIVER
> +#define DRM_DBG_CAT_KMS		DRM_UT_KMS
> +#define DRM_DBG_CAT_PRIME	DRM_UT_PRIME
> +#define DRM_DBG_CAT_ATOMIC	DRM_UT_ATOMIC
> +#define DRM_DBG_CAT_VBL		DRM_UT_VBL
> +#define DRM_DBG_CAT_STATE	DRM_UT_STATE
> +#define DRM_DBG_CAT_LEASE	DRM_UT_LEASE
> +#define DRM_DBG_CAT_DP		DRM_UT_DP
> +#define DRM_DBG_CAT_DRMRES	DRM_UT_DRMRES
> +
> +#else /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
> +
> +/* join prefix+format in cpp so dyndbg can see it */
> +#define __drm_dbg(cls, fmt, ...)		\
> +	pr_debug(cls fmt, ##__VA_ARGS__)
> +#define drm_dev_dbg(dev, cls, fmt, ...)		\
> +	dev_dbg(dev, cls fmt, ##__VA_ARGS__)
> +
> +#define DRM_DBG_CAT_CORE	"drm:core: "
> +#define DRM_DBG_CAT_DRIVER	"drm:drvr: "
> +#define DRM_DBG_CAT_KMS		"drm:kms: "
> +#define DRM_DBG_CAT_PRIME	"drm:prime: "
> +#define DRM_DBG_CAT_ATOMIC	"drm:atomic: "
> +#define DRM_DBG_CAT_VBL		"drm:vbl: "
> +#define DRM_DBG_CAT_STATE	"drm:state: "
> +#define DRM_DBG_CAT_LEASE	"drm:lease: "
> +#define DRM_DBG_CAT_DP		"drm:dp: "
> +#define DRM_DBG_CAT_DRMRES	"drm:res: " /* not in MODULE_PARM_DESC */
> +
> +#endif /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
> +
>   static inline bool drm_debug_enabled(enum drm_debug_category category)
>   {
>   	return unlikely(__drm_debug & category);
> @@ -334,8 +414,8 @@ __printf(3, 4)
>   void drm_dev_printk(const struct device *dev, const char *level,
>   		    const char *format, ...);
>   __printf(3, 4)
> -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, ...);
>   
>   /**
>    * DRM_DEV_ERROR() - Error output.
> @@ -383,7 +463,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>    * @fmt: printf() like format string.
>    */
>   #define DRM_DEV_DEBUG(dev, fmt, ...)					\
> -	drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg(dev, DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
>   /**
>    * DRM_DEV_DEBUG_DRIVER() - Debug output for vendor specific part of the driver
>    *
> @@ -391,7 +471,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>    * @fmt: printf() like format string.
>    */
>   #define DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)				\
> -	drm_dev_dbg(dev, DRM_UT_DRIVER,	fmt, ##__VA_ARGS__)
> +	drm_dev_dbg(dev, DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
>   /**
>    * DRM_DEV_DEBUG_KMS() - Debug output for modesetting code
>    *
> @@ -399,7 +479,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>    * @fmt: printf() like format string.
>    */
>   #define DRM_DEV_DEBUG_KMS(dev, fmt, ...)				\
> -	drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg(dev, DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
>   
>   /*
>    * struct drm_device based logging
> @@ -443,25 +523,25 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>   
>   
>   #define drm_dbg_core(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_CORE, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
>   #define drm_dbg(drm, fmt, ...)						\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
>   #define drm_dbg_kms(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
>   #define drm_dbg_prime(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_PRIME, fmt, ##__VA_ARGS__)
>   #define drm_dbg_atomic(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_ATOMIC, fmt, ##__VA_ARGS__)
>   #define drm_dbg_vbl(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_VBL, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_VBL, fmt, ##__VA_ARGS__)
>   #define drm_dbg_state(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_STATE, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_STATE, fmt, ##__VA_ARGS__)
>   #define drm_dbg_lease(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_LEASE, fmt, ##__VA_ARGS__)
>   #define drm_dbg_dp(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DP, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DP, fmt, ##__VA_ARGS__)
>   #define drm_dbg_drmres(drm, fmt, ...)					\
> -	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
> +	drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DRMRES, fmt, ##__VA_ARGS__)
>   
>   
>   /*
> @@ -471,7 +551,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
>    */
>   
>   __printf(2, 3)
> -void __drm_dbg(enum drm_debug_category category, const char *format, ...);
> +void ___drm_dbg(enum drm_debug_category category, const char *format, ...);
>   __printf(1, 2)
>   void __drm_err(const char *format, ...);
>   
> @@ -500,29 +580,30 @@ void __drm_err(const char *format, ...);
>   #define DRM_ERROR_RATELIMITED(fmt, ...)					\
>   	DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
>   
> +
>   #define DRM_DEBUG(fmt, ...)						\
> -	__drm_dbg(DRM_UT_CORE, fmt, ##__VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
>   
>   #define DRM_DEBUG_DRIVER(fmt, ...)					\
> -	__drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
>   
>   #define DRM_DEBUG_KMS(fmt, ...)						\
> -	__drm_dbg(DRM_UT_KMS, fmt, ##__VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
>   
>   #define DRM_DEBUG_PRIME(fmt, ...)					\
> -	__drm_dbg(DRM_UT_PRIME, fmt, ##__VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_PRIME, fmt, ##__VA_ARGS__)
>   
>   #define DRM_DEBUG_ATOMIC(fmt, ...)					\
> -	__drm_dbg(DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_ATOMIC, fmt, ##__VA_ARGS__)
>   
>   #define DRM_DEBUG_VBL(fmt, ...)						\
> -	__drm_dbg(DRM_UT_VBL, fmt, ##__VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_VBL, fmt, ##__VA_ARGS__)
>   
>   #define DRM_DEBUG_LEASE(fmt, ...)					\
> -	__drm_dbg(DRM_UT_LEASE, fmt, ##__VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_LEASE, fmt, ##__VA_ARGS__)
>   
>   #define DRM_DEBUG_DP(fmt, ...)						\
> -	__drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
> +	__drm_dbg(DRM_DBG_CAT_DP, fmt, ## __VA_ARGS__)
>   
>   #define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)					\
>   ({												\
> @@ -530,7 +611,8 @@ void __drm_err(const char *format, ...);
>   	const struct drm_device *drm_ = (drm);							\
>   												\
>   	if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(&rs_))			\
> -		drm_dev_printk(drm_ ? drm_->dev : NULL, KERN_DEBUG, fmt, ## __VA_ARGS__);	\
> +		drm_dev_dbg((drm_) ? (drm_)->dev : NULL,					\
> +			    DRM_DBG_CAT_ ## category, fmt, ##__VA_ARGS__);			\
>   })
>   
>   #define drm_dbg_kms_ratelimited(drm, fmt, ...) \
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
  2021-09-03 11:07   ` [Intel-gfx] " Tvrtko Ursulin
@ 2021-09-03 19:22     ` jim.cromie
  2021-09-06 12:26       ` Tvrtko Ursulin
  0 siblings, 1 reply; 19+ messages in thread
From: jim.cromie @ 2021-09-03 19:22 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Jason Baron, Greg KH, LKML, dri-devel, amd-gfx mailing list,
	intel-gvt-dev, Intel Graphics Development

On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 31/08/2021 21:21, Jim Cromie wrote:
> > The gvt component of this driver has ~120 pr_debugs, in 9 categories
> > quite similar to those in DRM.  Following the interface model of
> > drm.debug, add a parameter to map bits to these categorizations.
> >
> > DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
> >       "dyndbg bitmap desc",
> >       { "gvt:cmd: ",  "command processing" },
> >       { "gvt:core: ", "core help" },
> >       { "gvt:dpy: ",  "display help" },
> >       { "gvt:el: ",   "help" },
> >       { "gvt:irq: ",  "help" },
> >       { "gvt:mm: ",   "help" },
> >       { "gvt:mmio: ", "help" },
> >       { "gvt:render: ", "help" },
> >       { "gvt:sched: " "help" });
> >

BTW, Ive dropped the help field, its already handled, dont need to clutter.


> > The actual patch has a few details different, cmd_help() macro emits
> > the initialization construct.
> >
> > if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added
> > cflags, by gvt/Makefile.
> >
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> > ---
> > v5:
> > . static decl of vector of bit->class descriptors - Emil.V
> > . relocate gvt-makefile chunk from elsewhere
> > v7:
> > . move ccflags addition up to i915/Makefile from i915/gvt
> > ---
> >   drivers/gpu/drm/i915/Makefile      |  4 ++++
> >   drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++
>
> Can this work if put under gvt/ or at least intel_gvt.h|c?
>

I thought it belonged here more, at least according to the name of the
config.var

CONFIG_DRM_USE_DYNAMIC_DEBUG.

I suppose its not a great name, its narrow purpose is to swap
drm-debug api to use dyndbg.   drm-evrything already "uses"
dyndbg if CONFIG_DYNAMIC_DEBUG=y, those gvt/pr_debugs in particular.

Theres also CONFIG_DYNAMIC_DEBUG_CORE=y,
which drm basically ignores currently.

So with the name CONFIG_DRM_USE_DYNAMIC_DEBUG
it seemed proper to arrange for that  to be true on DD-CORE=y builds,
by adding -DDYNAMIC_DEBUG_MODULE

Does that make some sense ?
How to best resolve the frictions ?
new CONFIG names ?

> >   2 files changed, 39 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index 4f22cac1c49b..5a4e371a3ec2 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init)
> >
> >   subdir-ccflags-y += -I$(srctree)/$(src)
> >
> > +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
> > +ccflags-y += -DDYNAMIC_DEBUG_MODULE
> > +#endif
>
> Ignores whether CONFIG_DRM_I915_GVT is enabled or not?
>

not intentionally.
I think theres 2 things youre noting:

1 - make frag into gvt/Makefile
I had it there earlier, not sure why I moved it up.
maybe some confusion on proper scope of the flag.


2 - move new declaration code in i915-param.c inside the gvt ifdef

Im good with that.
I'll probably copy the ifdef wrapper down rather than move the decl up.
ie:

#if __and(IS_ENABLED(CONFIG_DRM_I915_GVT),
  IS_ENABLED(CONFIG_DRM_USE_DYNAMIC_DEBUG))

unsigned long __gvt_debug;
EXPORT_SYMBOL(__gvt_debug);


> > +
> >   # Please keep these build lists sorted!
> >
> >   # core driver code
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index e07f4cfea63a..e645e149485e 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params)
> >       I915_PARAMS_FOR_EACH(FREE);
> >   #undef FREE
> >   }
> > +
> > +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
> > +/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */
> > +
> > +unsigned long __gvt_debug;
> > +EXPORT_SYMBOL(__gvt_debug);
> > +
> > +#define _help(key)   "\t    \"" key "\"\t: help for " key "\n"
> > +
> > +#define I915_GVT_CATEGORIES(name) \
> > +     " Enable debug output via /sys/module/i915/parameters/" #name   \
> > +     ", where each bit enables a debug category.\n"                  \
> > +     _help("gvt:cmd:")                                               \
> > +     _help("gvt:core:")                                              \
> > +     _help("gvt:dpy:")                                               \
> > +     _help("gvt:el:")                                                \
> > +     _help("gvt:irq:")                                               \
> > +     _help("gvt:mm:")                                                \
> > +     _help("gvt:mmio:")                                              \
> > +     _help("gvt:render:")                                            \
> > +     _help("gvt:sched:")
> > +
> > +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
> > +                             I915_GVT_CATEGORIES(debug_gvt),
> > +                             _DD_cat_("gvt:cmd:"),
> > +                             _DD_cat_("gvt:core:"),
> > +                             _DD_cat_("gvt:dpy:"),
> > +                             _DD_cat_("gvt:el:"),
> > +                             _DD_cat_("gvt:irq:"),
> > +                             _DD_cat_("gvt:mm:"),
> > +                             _DD_cat_("gvt:mmio:"),
> > +                             _DD_cat_("gvt:render:"),
> > +                             _DD_cat_("gvt:sched:"));
> > +
> > +#endif
>
> So just the foundation - no actual use sites I mean? How would these be
> used from the code?
>

there are 120 pr_debug "users" :-)

no users per se, but anyone using drm.debug
/sys/module/drm/parameters/debug
might use this too.
its a bit easier than composing queries for >/proc/dyamic_debug/control




> Regards,
>
> Tvrtko

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug
  2021-09-03 11:15   ` [Intel-gfx] " Tvrtko Ursulin
@ 2021-09-03 21:57     ` jim.cromie
  2021-09-06 10:20       ` Tvrtko Ursulin
  0 siblings, 1 reply; 19+ messages in thread
From: jim.cromie @ 2021-09-03 21:57 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Jason Baron, Greg KH, LKML, dri-devel, amd-gfx mailing list,
	intel-gvt-dev, Intel Graphics Development

On Fri, Sep 3, 2021 at 5:15 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 31/08/2021 21:21, Jim Cromie wrote:
> > 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 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.
>
> Probably stupid question - enabling stuff at boot time still works as
> described in Documentation/admin-guide/dynamic-debug-howto.rst?
>

yes.  its turned on in earlyinit, and cmdline args are a processed then,
and when modules are added


> Second question, which perhaps has been covered in the past so apologies
> if redundant - what is the advantage of allowing this to be
> configurable, versus perhaps always enabling it? Like what would be the
> reasons someone wouldn't just want to have CONFIG_DYNAMIC_DEBUG compiled
> in? Kernel binary size?
>

Im unaware of anything on this topic, but I can opine :-)
Its been configurable since I saw it and thought "jump-labels are cool!"

code is small
[jimc@frodo local-i915m]$ size lib/dynamic_debug.o
   text    data     bss     dec     hex filename
  24016    8041      64   32121    7d79 lib/dynamic_debug.o

Its data tables are big, particularly the __dyndbg section
builtins:
dyndbg: 108 debug prints in module mptcp
dyndbg:   2 debug prints in module i386
dyndbg:   2 debug prints in module xen
dyndbg:   2 debug prints in module fixup
dyndbg:   7 debug prints in module irq
dyndbg: 3039 prdebugs in 283 modules, 11 KiB in ddebug tables, 166 kiB
in __dyndbg section

bash-5.1#
bash-5.1# for m in i915 amdgpu ; do modprobe $m dyndbg=+_ ; done
dyndbg: 384 debug prints in module drm
dyndbg: 211 debug prints in module drm_kms_helper
dyndbg:   2 debug prints in module ttm
dyndbg:   8 debug prints in module video
dyndbg: 1727 debug prints in module i915
dyndbg: processed 1 queries, with 3852 matches, 0 errs
dyndbg: 3852 debug prints in module amdgpu
[drm] amdgpu kernel modesetting enabled.
amdgpu: CRAT table disabled by module option
amdgpu: Virtual CRAT table created for CPU
amdgpu: Topology: Add CPU node
bash-5.1#

At 56 bytes / callsite, it adds up.
And teaching DRM to use it enlarges its use dramatically,
not just in drm itself, but in many drivers

amdgpu has 3852 callsite, (vs 3039 in my kernel), so it has ~240kb.
It has extra (large chunks generated by macros) to trim,
but i915 has ~1700, and drm has ~380

I have WIP to reduce the table space, by splitting it into 2 separate ones;
guts and decorations (module, function, file pointers).
The decoration recs are redundant, 45% are copies of previous
(function changes fastest)
It needs much rework, but it should get 20% overall.
decorations are 24/56 of footprint.








> Regards,
>
> Tvrtko
>
> >
> > This conversion yields many new prdbg callsites:
> >
> >    dyndbg: 195 debug prints in module drm_kms_helper
> >    dyndbg: 298 debug prints in module drm
> >    dyndbg: 1630 debug prints in module i915
> >    dyndbg: ~3500 debug prints in module amdgpu
> >
> > CONFIG_DRM_USE_DYNAMIC_DEBUG enables this, and is available if
> > CONFIG_DYNAMIC_DEBUG or CONFIG_DYNAMIC_DEBUG_CORE is chosen, and if
> > CONFIG_JUMP_LABEL is enabled; this because its 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: "
> >
> > In v3, had older name, DRM_DBG_CLASS_<CATs> was countered, I had
> > agreed, but this seems better still; CATEGORY is already DRM's
> > term-of-art, and adding a near-synonym 'CLASS' only adds ambiguity.
> >
> > DRM_UT_* are preserved, since theyre used elsewhere.  We can probably
> > reduce their use further, but thats a separate thing.
> >
> > 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
> >
> > these already use (B), now they use (A) too, 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.
> >
> > NOTES:
> >
> > Because the dyndbg callback is watching __drm_debug, it is coherent
> > 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 search-prefixes/categories with a trailing space, which
> > excludes any sub-categories 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
> >
> > With "lineno X" in a query, its possible to enable single callsites,
> > but it is tedious, and useless in a category context.
> >
> > Unfortunately __func__ is not a macro, and cannot be catenated at
> > preprocess/compile time.
> >
> > 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 KBuild 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
> > ---
> >   drivers/gpu/drm/Kconfig     |  13 ++++
> >   drivers/gpu/drm/Makefile    |   3 +
> >   drivers/gpu/drm/drm_print.c |  53 +++++++++----
> >   include/drm/drm_print.h     | 144 ++++++++++++++++++++++++++++--------
> >   4 files changed, 166 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 7ff89690a976..97e38d86fd27 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -57,6 +57,19 @@ 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 patches pr_debug()s in/out
> > +       of the running kernel, so avoids those bit-test overheads,
> > +       and is therefore recommended.
> > +
> >   config DRM_DEBUG_SELFTEST
> >       tristate "kselftests for DRM"
> >       depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index a118692a6df7..1809329654b3 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -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
> > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> > index 111b932cf2a9..df2e10754c41 100644
> > --- a/drivers/gpu/drm/drm_print.c
> > +++ b/drivers/gpu/drm/drm_print.c
> > @@ -28,6 +28,7 @@
> >   #include <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,39 @@
> >    * __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)."
> > +
> > +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
> > +#include <linux/dynamic_debug.h>
> > +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
> > +     DRM_DEBUG_DESC,
> > +     _DD_cat_(DRM_DBG_CAT_CORE),
> > +     _DD_cat_(DRM_DBG_CAT_DRIVER),
> > +     _DD_cat_(DRM_DBG_CAT_KMS),
> > +     _DD_cat_(DRM_DBG_CAT_PRIME),
> > +     _DD_cat_(DRM_DBG_CAT_ATOMIC),
> > +     _DD_cat_(DRM_DBG_CAT_VBL),
> > +     _DD_cat_(DRM_DBG_CAT_STATE),
> > +     _DD_cat_(DRM_DBG_CAT_LEASE),
> > +     _DD_cat_(DRM_DBG_CAT_DP),
> > +     _DD_cat_(DRM_DBG_CAT_DRMRES));
> > +
> > +#else
> > +MODULE_PARM_DESC(debug, DRM_DEBUG_DESC);
> > +module_param_named(debug, __drm_debug, ulong, 0600);
> > +#endif
> >
> >   void __drm_puts_coredump(struct drm_printer *p, const char *str)
> >   {
> > @@ -256,8 +277,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 +299,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 +318,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, ...)
> >   {
> > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> > index 9b66be54dd16..973443040561 100644
> > --- a/include/drm/drm_print.h
> > +++ b/include/drm/drm_print.h
> > @@ -35,7 +35,7 @@
> >   #include <drm/drm.h>
> >
> >   /* Do *not* use outside of drm_print.[ch]! */
> > -extern unsigned int __drm_debug;
> > +extern unsigned long __drm_debug;
> >
> >   /**
> >    * DOC: print
> > @@ -252,15 +252,15 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
> >   /**
> >    * enum drm_debug_category - The DRM debug categories
> >    *
> > - * Each of the DRM debug logging macros use a specific category, and the logging
> > - * is filtered by the drm.debug module parameter. This enum specifies the values
> > - * for the interface.
> > + * The drm.debug logging API[1] has 10 enumerated categories of
> > + * messages, issued by 3 families of macros: 10 drm_dbg_<CATs>, 8
> > + * DRM_DEBUG_<CATs>, and 3 DRM_DEV_DEBUG_<CATs>.
> >    *
> >    * Each DRM_DEBUG_<CATEGORY> macro logs to DRM_UT_<CATEGORY> category, except
> >    * DRM_DEBUG() logs to DRM_UT_CORE.
> >    *
> > - * Enabling verbose debug messages is done through the drm.debug parameter, each
> > - * category being enabled by a bit:
> > + * Enabling categories of debug messages is done through the drm.debug
> > + * parameter, each category being enabled by a bit:
> >    *
> >    *  - drm.debug=0x1 will enable CORE messages
> >    *  - drm.debug=0x2 will enable DRIVER messages
> > @@ -319,6 +319,86 @@ enum drm_debug_category {
> >       DRM_UT_DRMRES           = 0x200,
> >   };
> >
> > +/**
> > + * DOC: DRM_USE_DYNAMIC_DEBUG - using dyndbg in drm.debug
> > + *
> > + * In the "basic" drm.debug implementation outlined above, each time a
> > + * drm-debug API[1] call is executed, drm_debug_enabled(cat) tests
> > + * drm.debug vs cat before printing.
> > + *
> > + * DYNAMIC_DEBUG (aka: dyndbg) patches pr_debug()s in^out of the
> > + * running kernel, so it can avoid drm_debug_enabled() and skip lots
> > + * of unlikely bit tests.
> > + *
> > + * dyndbg has no concept of category, but we can prepend a
> > + * class-prefix string: "drm:core: ", "drm:kms: ", "drm:driver: " etc,
> > + * to pr_debug's format (at compile time).
> > + *
> > + * Then control the category
> > + *    `echo module drm format "^drm:core: " +p > control`
> > + *    `dynamic_debug_exec_queries("format '^drm:core: ' +p", "drm");`
> > + *
> > + * To do this for "basic" | "dyndbg", adaptation adds some macro indirection:
> > + *
> > + * 0. use dyndbg support to define the bits => prefixes map, attach callback.
> > + *
> > + *    DYNDBG_BITMAP_DESC(debug, __drm_debug,
> > + *                    "drm.debug - overview",
> > + *                    { "drm:core:", "enable CORE debug messages" },
> > + *                    { "drm:kms:", "enable KMS debug messages" }, ...);
> > + *
> > + * 1. DRM_DBG_CAT_<CAT>
> > + *
> > + * This set of symbols replaces DRM_UT_<CAT> in the drm-debug API; it
> > + * is either a copy of DRM_UT_<CAT>, or the class-prefix strings.
> > + *
> > + * 2. drm_dev_dbg & drm_debug are called by drm.debug API
> > + *
> > + * These are now macros, either forwarding to renamed functions, or
> > + * prepending the class string to the format, and invoking pr_debug
> > + * directly.  Since the API is all macros, dyndbg remembers
> > + * per-pr_debug: module,file,func,line,format and uses that to find
> > + * and enable them.
> > + */
> > +#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
> > +
> > +#define __drm_dbg(cls, fmt, ...)                     \
> > +     ___drm_dbg(cls, fmt, ##__VA_ARGS__)
> > +#define drm_dev_dbg(dev, cls, fmt, ...)                      \
> > +     __drm_dev_dbg(dev, cls, fmt, ##__VA_ARGS__)
> > +
> > +#define DRM_DBG_CAT_CORE     DRM_UT_CORE
> > +#define DRM_DBG_CAT_DRIVER   DRM_UT_DRIVER
> > +#define DRM_DBG_CAT_KMS              DRM_UT_KMS
> > +#define DRM_DBG_CAT_PRIME    DRM_UT_PRIME
> > +#define DRM_DBG_CAT_ATOMIC   DRM_UT_ATOMIC
> > +#define DRM_DBG_CAT_VBL              DRM_UT_VBL
> > +#define DRM_DBG_CAT_STATE    DRM_UT_STATE
> > +#define DRM_DBG_CAT_LEASE    DRM_UT_LEASE
> > +#define DRM_DBG_CAT_DP               DRM_UT_DP
> > +#define DRM_DBG_CAT_DRMRES   DRM_UT_DRMRES
> > +
> > +#else /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
> > +
> > +/* join prefix+format in cpp so dyndbg can see it */
> > +#define __drm_dbg(cls, fmt, ...)             \
> > +     pr_debug(cls fmt, ##__VA_ARGS__)
> > +#define drm_dev_dbg(dev, cls, fmt, ...)              \
> > +     dev_dbg(dev, cls fmt, ##__VA_ARGS__)
> > +
> > +#define DRM_DBG_CAT_CORE     "drm:core: "
> > +#define DRM_DBG_CAT_DRIVER   "drm:drvr: "
> > +#define DRM_DBG_CAT_KMS              "drm:kms: "
> > +#define DRM_DBG_CAT_PRIME    "drm:prime: "
> > +#define DRM_DBG_CAT_ATOMIC   "drm:atomic: "
> > +#define DRM_DBG_CAT_VBL              "drm:vbl: "
> > +#define DRM_DBG_CAT_STATE    "drm:state: "
> > +#define DRM_DBG_CAT_LEASE    "drm:lease: "
> > +#define DRM_DBG_CAT_DP               "drm:dp: "
> > +#define DRM_DBG_CAT_DRMRES   "drm:res: " /* not in MODULE_PARM_DESC */
> > +
> > +#endif /* CONFIG_DRM_USE_DYNAMIC_DEBUG */
> > +
> >   static inline bool drm_debug_enabled(enum drm_debug_category category)
> >   {
> >       return unlikely(__drm_debug & category);
> > @@ -334,8 +414,8 @@ __printf(3, 4)
> >   void drm_dev_printk(const struct device *dev, const char *level,
> >                   const char *format, ...);
> >   __printf(3, 4)
> > -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, ...);
> >
> >   /**
> >    * DRM_DEV_ERROR() - Error output.
> > @@ -383,7 +463,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >    * @fmt: printf() like format string.
> >    */
> >   #define DRM_DEV_DEBUG(dev, fmt, ...)                                        \
> > -     drm_dev_dbg(dev, DRM_UT_CORE, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg(dev, DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
> >   /**
> >    * DRM_DEV_DEBUG_DRIVER() - Debug output for vendor specific part of the driver
> >    *
> > @@ -391,7 +471,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >    * @fmt: printf() like format string.
> >    */
> >   #define DRM_DEV_DEBUG_DRIVER(dev, fmt, ...)                         \
> > -     drm_dev_dbg(dev, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg(dev, DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
> >   /**
> >    * DRM_DEV_DEBUG_KMS() - Debug output for modesetting code
> >    *
> > @@ -399,7 +479,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >    * @fmt: printf() like format string.
> >    */
> >   #define DRM_DEV_DEBUG_KMS(dev, fmt, ...)                            \
> > -     drm_dev_dbg(dev, DRM_UT_KMS, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg(dev, DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
> >
> >   /*
> >    * struct drm_device based logging
> > @@ -443,25 +523,25 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >
> >
> >   #define drm_dbg_core(drm, fmt, ...)                                 \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_CORE, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
> >   #define drm_dbg(drm, fmt, ...)                                              \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_kms(drm, fmt, ...)                                  \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_KMS, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_prime(drm, fmt, ...)                                        \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_PRIME, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_PRIME, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_atomic(drm, fmt, ...)                                       \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_ATOMIC, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_vbl(drm, fmt, ...)                                  \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_VBL, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_VBL, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_state(drm, fmt, ...)                                        \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_STATE, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_STATE, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_lease(drm, fmt, ...)                                        \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_LEASE, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_LEASE, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_dp(drm, fmt, ...)                                   \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DP, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DP, fmt, ##__VA_ARGS__)
> >   #define drm_dbg_drmres(drm, fmt, ...)                                       \
> > -     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_UT_DRMRES, fmt, ##__VA_ARGS__)
> > +     drm_dev_dbg((drm) ? (drm)->dev : NULL, DRM_DBG_CAT_DRMRES, fmt, ##__VA_ARGS__)
> >
> >
> >   /*
> > @@ -471,7 +551,7 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category,
> >    */
> >
> >   __printf(2, 3)
> > -void __drm_dbg(enum drm_debug_category category, const char *format, ...);
> > +void ___drm_dbg(enum drm_debug_category category, const char *format, ...);
> >   __printf(1, 2)
> >   void __drm_err(const char *format, ...);
> >
> > @@ -500,29 +580,30 @@ void __drm_err(const char *format, ...);
> >   #define DRM_ERROR_RATELIMITED(fmt, ...)                                     \
> >       DRM_DEV_ERROR_RATELIMITED(NULL, fmt, ##__VA_ARGS__)
> >
> > +
> >   #define DRM_DEBUG(fmt, ...)                                         \
> > -     __drm_dbg(DRM_UT_CORE, fmt, ##__VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_CORE, fmt, ##__VA_ARGS__)
> >
> >   #define DRM_DEBUG_DRIVER(fmt, ...)                                  \
> > -     __drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_DRIVER, fmt, ##__VA_ARGS__)
> >
> >   #define DRM_DEBUG_KMS(fmt, ...)                                             \
> > -     __drm_dbg(DRM_UT_KMS, fmt, ##__VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_KMS, fmt, ##__VA_ARGS__)
> >
> >   #define DRM_DEBUG_PRIME(fmt, ...)                                   \
> > -     __drm_dbg(DRM_UT_PRIME, fmt, ##__VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_PRIME, fmt, ##__VA_ARGS__)
> >
> >   #define DRM_DEBUG_ATOMIC(fmt, ...)                                  \
> > -     __drm_dbg(DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_ATOMIC, fmt, ##__VA_ARGS__)
> >
> >   #define DRM_DEBUG_VBL(fmt, ...)                                             \
> > -     __drm_dbg(DRM_UT_VBL, fmt, ##__VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_VBL, fmt, ##__VA_ARGS__)
> >
> >   #define DRM_DEBUG_LEASE(fmt, ...)                                   \
> > -     __drm_dbg(DRM_UT_LEASE, fmt, ##__VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_LEASE, fmt, ##__VA_ARGS__)
> >
> >   #define DRM_DEBUG_DP(fmt, ...)                                              \
> > -     __drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
> > +     __drm_dbg(DRM_DBG_CAT_DP, fmt, ## __VA_ARGS__)
> >
> >   #define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)                                       \
> >   ({                                                                                          \
> > @@ -530,7 +611,8 @@ void __drm_err(const char *format, ...);
> >       const struct drm_device *drm_ = (drm);                                                  \
> >                                                                                               \
> >       if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(&rs_))                        \
> > -             drm_dev_printk(drm_ ? drm_->dev : NULL, KERN_DEBUG, fmt, ## __VA_ARGS__);       \
> > +             drm_dev_dbg((drm_) ? (drm_)->dev : NULL,                                        \
> > +                         DRM_DBG_CAT_ ## category, fmt, ##__VA_ARGS__);                      \
> >   })
> >
> >   #define drm_dbg_kms_ratelimited(drm, fmt, ...) \
> >

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 6/8] drm_print: instrument drm_debug_enabled
  2021-08-31 20:21 ` [PATCH v7 6/8] drm_print: instrument drm_debug_enabled Jim Cromie
@ 2021-09-05 18:50   ` jim.cromie
  0 siblings, 0 replies; 19+ messages in thread
From: jim.cromie @ 2021-09-05 18:50 UTC (permalink / raw)
  To: Jason Baron, Greg KH, LKML, dri-devel, amd-gfx mailing list,
	intel-gvt-dev, Intel Graphics Development

On Tue, Aug 31, 2021 at 2:21 PM Jim Cromie <jim.cromie@gmail.com> wrote:
>
> Duplicate drm_debug_enabled() code into both "basic" and "dyndbg"
> ifdef branches.  Then add a pr_debug("todo: ...") into the "dyndbg"
> branch.
>
> Then convert the "dyndbg" branch's code to a macro, so that its
> pr_debug() get its callsite info from the invoking function, instead
> of from drm_debug_enabled() itself.
>
> This gives us unique callsite info for the 8 remaining users of
> drm_debug_enabled(), and lets us enable them individually to see how
> much logging traffic they generate.  The oft-visited callsites can
> then be reviewed for runtime cost and possible optimizations.
>
> Heres what we get:
>
> bash-5.1# modprobe drm
> dyndbg: 384 debug prints in module drm
> bash-5.1# grep todo: /proc/dynamic_debug/control
> drivers/gpu/drm/drm_edid.c:1843 [drm]connector_bad_edid =_ "todo: maybe avoid via dyndbg\012"
> drivers/gpu/drm/drm_print.c:309 [drm]___drm_dbg =p "todo: maybe avoid via dyndbg\012"
> drivers/gpu/drm/drm_print.c:286 [drm]__drm_dev_dbg =p "todo: maybe avoid via dyndbg\012"
> drivers/gpu/drm/drm_vblank.c:1491 [drm]drm_vblank_restore =_ "todo: maybe avoid via dyndbg\012"
> drivers/gpu/drm/drm_vblank.c:787 [drm]drm_crtc_vblank_helper_get_vblank_timestamp_internal =_ "todo: maybe avoid via dyndbg\012"
> drivers/gpu/drm/drm_vblank.c:410 [drm]drm_crtc_accurate_vblank_count =_ "todo: maybe avoid via dyndbg\012"
> drivers/gpu/drm/drm_atomic_uapi.c:1457 [drm]drm_mode_atomic_ioctl =_ "todo: maybe avoid via dyndbg\012"
> drivers/gpu/drm/drm_edid_load.c:178 [drm]edid_load =_ "todo: maybe avoid via dyndbg\012"
>
> At quick glance, edid won't qualify, drm_print might, drm_vblank is
> strongest chance, maybe atomic-ioctl too.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---

heres 120 seconds of enabled todos, from this patch

[jimc@frodo wk-next]$ journalctl -b0 | grep todo | grep 'frodo kernel'
[jimc@frodo wk-next]$ sudo su -c 'echo format ^todo: +pfml >
/proc/dynamic_debug/control;  sleep 120; echo format ^todo: -p >
/proc/dynamic_debug/control'
[sudo] password for jimc:
[jimc@frodo wk-next]$ journalctl -b0 | grep todo | grep 'frodo kernel'
 > todo-120-log
[jimc@frodo wk-next]$ wc todo-120-log
  228  2516 24066 todo-120-log

so overall thats not too much work, not so many bitchecks as to be
worth avoiding.

I think I'll try hitting it with my new igt-tools hammer, see what breaks :-)


[jimc@frodo wk-next]$ hsto todo-120-log
120 :  drm:drm_crtc_vblank_helper_get_vblank_timestamp_internal:787:
todo: maybe avoid via dyndbg
40 :  i915:process_csb:1904: todo: maybe avoid via dyndbg
20 :  drm:drm_vblank_restore:1491: todo: maybe avoid via dyndbg
20 :  drm:drm_crtc_accurate_vblank_count:410: todo: maybe avoid via dyndbg
20 :  i915:skl_print_wm_changes:6068: todo: maybe avoid via dyndbg
2 :  dyndbg: applied: func="" file="" module="" format="^todo:" lineno=0-0
2 :  dyndbg: parsed: func="" file="" module="" format="^todo:" lineno=0-0
1 :  dyndbg: split into words: "format" "^todo:" "-p"
1 :  dyndbg: split into words: "format" "^todo:" "+pfml"
1 :  dyndbg: query 0: "format ^todo: -p" mod:*
1 :  dyndbg: query 0: "format ^todo: +pfml" mod:*

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug
  2021-09-03 21:57     ` jim.cromie
@ 2021-09-06 10:20       ` Tvrtko Ursulin
  2021-09-06 18:24         ` jim.cromie
  0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2021-09-06 10:20 UTC (permalink / raw)
  To: jim.cromie
  Cc: Jason Baron, Greg KH, LKML, dri-devel, amd-gfx mailing list,
	intel-gvt-dev, Intel Graphics Development


On 03/09/2021 22:57, jim.cromie@gmail.com wrote:
> On Fri, Sep 3, 2021 at 5:15 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 31/08/2021 21:21, Jim Cromie wrote:
>>> 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 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.
>>
>> Probably stupid question - enabling stuff at boot time still works as
>> described in Documentation/admin-guide/dynamic-debug-howto.rst?
>>
> 
> yes.  its turned on in earlyinit, and cmdline args are a processed then,
> and when modules are added
> 
> 
>> Second question, which perhaps has been covered in the past so apologies
>> if redundant - what is the advantage of allowing this to be
>> configurable, versus perhaps always enabling it? Like what would be the
>> reasons someone wouldn't just want to have CONFIG_DYNAMIC_DEBUG compiled
>> in? Kernel binary size?
>>
> 
> Im unaware of anything on this topic, but I can opine :-)
> Its been configurable since I saw it and thought "jump-labels are cool!"
> 
> code is small
> [jimc@frodo local-i915m]$ size lib/dynamic_debug.o
>     text    data     bss     dec     hex filename
>    24016    8041      64   32121    7d79 lib/dynamic_debug.o
> 
> Its data tables are big, particularly the __dyndbg section
> builtins:
> dyndbg: 108 debug prints in module mptcp
> dyndbg:   2 debug prints in module i386
> dyndbg:   2 debug prints in module xen
> dyndbg:   2 debug prints in module fixup
> dyndbg:   7 debug prints in module irq
> dyndbg: 3039 prdebugs in 283 modules, 11 KiB in ddebug tables, 166 kiB
> in __dyndbg section
> 
> bash-5.1#
> bash-5.1# for m in i915 amdgpu ; do modprobe $m dyndbg=+_ ; done
> dyndbg: 384 debug prints in module drm
> dyndbg: 211 debug prints in module drm_kms_helper
> dyndbg:   2 debug prints in module ttm
> dyndbg:   8 debug prints in module video
> dyndbg: 1727 debug prints in module i915
> dyndbg: processed 1 queries, with 3852 matches, 0 errs
> dyndbg: 3852 debug prints in module amdgpu
> [drm] amdgpu kernel modesetting enabled.
> amdgpu: CRAT table disabled by module option
> amdgpu: Virtual CRAT table created for CPU
> amdgpu: Topology: Add CPU node
> bash-5.1#
> 
> At 56 bytes / callsite, it adds up.
> And teaching DRM to use it enlarges its use dramatically,
> not just in drm itself, but in many drivers
> 
> amdgpu has 3852 callsite, (vs 3039 in my kernel), so it has ~240kb.
> It has extra (large chunks generated by macros) to trim,
> but i915 has ~1700, and drm has ~380
> 
> I have WIP to reduce the table space, by splitting it into 2 separate ones;
> guts and decorations (module, function, file pointers).
> The decoration recs are redundant, 45% are copies of previous
> (function changes fastest)
> It needs much rework, but it should get 20% overall.
> decorations are 24/56 of footprint.

I'll try to extract the "executive summary" from this, you tell me if I 
got it right.

So using or not using dynamic debug for DRM debug ends up being about 
shifting the cost between kernel binary size (data section grows by each 
pr_debug call site) and runtime conditionals?

Since the table sizes you mention seem significant enough, I think that 
justifies existence of DRM_USE_DYNAMIC_DEBUG. It would probably be a 
good idea to put some commentary on that there. Ideally including some 
rough estimates both including space cost per call site and space cost 
for a typical distro kernel build?

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
  2021-09-03 19:22     ` jim.cromie
@ 2021-09-06 12:26       ` Tvrtko Ursulin
       [not found]         ` <CAJfuBxymoFx79kQzGw_Gxv1vk7kVaTN-V0Hn694C6kT=kP7u2A@mail.gmail.com>
  0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2021-09-06 12:26 UTC (permalink / raw)
  To: jim.cromie
  Cc: Jason Baron, Greg KH, LKML, dri-devel, amd-gfx mailing list,
	intel-gvt-dev, Intel Graphics Development


On 03/09/2021 20:22, jim.cromie@gmail.com wrote:
> On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 31/08/2021 21:21, Jim Cromie wrote:
>>> The gvt component of this driver has ~120 pr_debugs, in 9 categories
>>> quite similar to those in DRM.  Following the interface model of
>>> drm.debug, add a parameter to map bits to these categorizations.
>>>
>>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
>>>        "dyndbg bitmap desc",
>>>        { "gvt:cmd: ",  "command processing" },
>>>        { "gvt:core: ", "core help" },
>>>        { "gvt:dpy: ",  "display help" },
>>>        { "gvt:el: ",   "help" },
>>>        { "gvt:irq: ",  "help" },
>>>        { "gvt:mm: ",   "help" },
>>>        { "gvt:mmio: ", "help" },
>>>        { "gvt:render: ", "help" },
>>>        { "gvt:sched: " "help" });
>>>
> 
> BTW, Ive dropped the help field, its already handled, dont need to clutter.
> 
> 
>>> The actual patch has a few details different, cmd_help() macro emits
>>> the initialization construct.
>>>
>>> if CONFIG_DRM_USE_DYNAMIC_DEBUG, then -DDYNAMIC_DEBUG_MODULE is added
>>> cflags, by gvt/Makefile.
>>>
>>> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
>>> ---
>>> v5:
>>> . static decl of vector of bit->class descriptors - Emil.V
>>> . relocate gvt-makefile chunk from elsewhere
>>> v7:
>>> . move ccflags addition up to i915/Makefile from i915/gvt
>>> ---
>>>    drivers/gpu/drm/i915/Makefile      |  4 ++++
>>>    drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++
>>
>> Can this work if put under gvt/ or at least intel_gvt.h|c?
>>
> 
> I thought it belonged here more, at least according to the name of the
> config.var

Hmm bear with me please - the categories this patch creates are intended 
to be used explicitly from the GVT "sub-module", or they somehow even 
get automatically used with no further intervention to callers required?

> CONFIG_DRM_USE_DYNAMIC_DEBUG.
> 
> I suppose its not a great name, its narrow purpose is to swap
> drm-debug api to use dyndbg.   drm-evrything already "uses"
> dyndbg if CONFIG_DYNAMIC_DEBUG=y, those gvt/pr_debugs in particular.
> 
> Theres also CONFIG_DYNAMIC_DEBUG_CORE=y,
> which drm basically ignores currently.
> 
> So with the name CONFIG_DRM_USE_DYNAMIC_DEBUG
> it seemed proper to arrange for that  to be true on DD-CORE=y builds,
> by adding -DDYNAMIC_DEBUG_MODULE
> 
> Does that make some sense ?
> How to best resolve the frictions ?
> new CONFIG names ?
> 
>>>    2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>>> index 4f22cac1c49b..5a4e371a3ec2 100644
>>> --- a/drivers/gpu/drm/i915/Makefile
>>> +++ b/drivers/gpu/drm/i915/Makefile
>>> @@ -30,6 +30,10 @@ CFLAGS_display/intel_fbdev.o = $(call cc-disable-warning, override-init)
>>>
>>>    subdir-ccflags-y += -I$(srctree)/$(src)
>>>
>>> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
>>> +ccflags-y += -DDYNAMIC_DEBUG_MODULE
>>> +#endif
>>
>> Ignores whether CONFIG_DRM_I915_GVT is enabled or not?
>>
> 
> not intentionally.
> I think theres 2 things youre noting:
> 
> 1 - make frag into gvt/Makefile
> I had it there earlier, not sure why I moved it up.
> maybe some confusion on proper scope of the flag.
> 
> 
> 2 - move new declaration code in i915-param.c inside the gvt ifdef
> 
> Im good with that.
> I'll probably copy the ifdef wrapper down rather than move the decl up.
> ie:
> 
> #if __and(IS_ENABLED(CONFIG_DRM_I915_GVT),
>    IS_ENABLED(CONFIG_DRM_USE_DYNAMIC_DEBUG))
> 
> unsigned long __gvt_debug;
> EXPORT_SYMBOL(__gvt_debug);
> 
> 
>>> +
>>>    # Please keep these build lists sorted!
>>>
>>>    # core driver code
>>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>>> index e07f4cfea63a..e645e149485e 100644
>>> --- a/drivers/gpu/drm/i915/i915_params.c
>>> +++ b/drivers/gpu/drm/i915/i915_params.c
>>> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params *params)
>>>        I915_PARAMS_FOR_EACH(FREE);
>>>    #undef FREE
>>>    }
>>> +
>>> +#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
>>> +/* todo: needs DYNAMIC_DEBUG_MODULE in some cases */
>>> +
>>> +unsigned long __gvt_debug;
>>> +EXPORT_SYMBOL(__gvt_debug);
>>> +
>>> +#define _help(key)   "\t    \"" key "\"\t: help for " key "\n"
>>> +
>>> +#define I915_GVT_CATEGORIES(name) \
>>> +     " Enable debug output via /sys/module/i915/parameters/" #name   \
>>> +     ", where each bit enables a debug category.\n"                  \
>>> +     _help("gvt:cmd:")                                               \
>>> +     _help("gvt:core:")                                              \
>>> +     _help("gvt:dpy:")                                               \
>>> +     _help("gvt:el:")                                                \
>>> +     _help("gvt:irq:")                                               \
>>> +     _help("gvt:mm:")                                                \
>>> +     _help("gvt:mmio:")                                              \
>>> +     _help("gvt:render:")                                            \
>>> +     _help("gvt:sched:")
>>> +
>>> +DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
>>> +                             I915_GVT_CATEGORIES(debug_gvt),
>>> +                             _DD_cat_("gvt:cmd:"),
>>> +                             _DD_cat_("gvt:core:"),
>>> +                             _DD_cat_("gvt:dpy:"),
>>> +                             _DD_cat_("gvt:el:"),
>>> +                             _DD_cat_("gvt:irq:"),
>>> +                             _DD_cat_("gvt:mm:"),
>>> +                             _DD_cat_("gvt:mmio:"),
>>> +                             _DD_cat_("gvt:render:"),
>>> +                             _DD_cat_("gvt:sched:"));
>>> +
>>> +#endif
>>
>> So just the foundation - no actual use sites I mean? How would these be
>> used from the code?
>>
> 
> there are 120 pr_debug "users" :-)
> 
> no users per se, but anyone using drm.debug
> /sys/module/drm/parameters/debug
> might use this too.
> its a bit easier than composing queries for >/proc/dyamic_debug/control

Same as my previous question, perhaps I am not up to speed with this 
yet.. Even if pr_debug is used inside GVT - are the categories and 
debug_gvt global as of this patch (or series)?

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug
  2021-09-06 10:20       ` Tvrtko Ursulin
@ 2021-09-06 18:24         ` jim.cromie
  0 siblings, 0 replies; 19+ messages in thread
From: jim.cromie @ 2021-09-06 18:24 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Jason Baron, Greg KH, LKML, dri-devel, amd-gfx mailing list,
	intel-gvt-dev, Intel Graphics Development

> I'll try to extract the "executive summary" from this, you tell me if I
> got it right.
>
> So using or not using dynamic debug for DRM debug ends up being about
> shifting the cost between kernel binary size (data section grows by each
> pr_debug call site) and runtime conditionals?

Yes.

> Since the table sizes you mention seem significant enough, I think that
> justifies existence of DRM_USE_DYNAMIC_DEBUG. It would probably be a
> good idea to put some commentary on that there. Ideally including some
> rough estimates both including space cost per call site and space cost
> for a typical distro kernel build?

yeah, agreed.  I presume you mean in Kconfig entry,
since commits have some size info now - I have i915, amdgpu, nouveau;
I can see some prose improvements for 5/8




> Regards,
>
> Tvrtko

thanks
Jim

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
       [not found]         ` <CAJfuBxymoFx79kQzGw_Gxv1vk7kVaTN-V0Hn694C6kT=kP7u2A@mail.gmail.com>
@ 2021-09-07 15:14           ` Tvrtko Ursulin
  2021-09-07 17:26             ` jim.cromie
  0 siblings, 1 reply; 19+ messages in thread
From: Tvrtko Ursulin @ 2021-09-07 15:14 UTC (permalink / raw)
  To: jim.cromie, Sean Paul, Jason Baron, Daniel Vetter
  Cc: Greg KH, LKML, dri-devel, amd-gfx mailing list, intel-gvt-dev,
	Intel Graphics Development


On 06/09/2021 18:41, jim.cromie@gmail.com wrote:
> On Mon, Sep 6, 2021 at 6:26 AM Tvrtko Ursulin 
> <tvrtko.ursulin@linux.intel.com <mailto:tvrtko.ursulin@linux.intel.com>> 
> wrote:
>  >
>  >
>  > On 03/09/2021 20:22, jim.cromie@gmail.com 
> <mailto:jim.cromie@gmail.com> wrote:
>  > > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin
>  > > <tvrtko.ursulin@linux.intel.com 
> <mailto:tvrtko.ursulin@linux.intel.com>> wrote:
>  > >>
>  > >>
>  > >> On 31/08/2021 21:21, Jim Cromie wrote:
>  > >>> The gvt component of this driver has ~120 pr_debugs, in 9 categories
>  > >>> quite similar to those in DRM.  Following the interface model of
>  > >>> drm.debug, add a parameter to map bits to these categorizations.
>  > >>>
>  > >>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
>  > >>>        "dyndbg bitmap desc",
>  > >>>        { "gvt:cmd: ",  "command processing" },
> 
>  > >>> v7:
>  > >>> . move ccflags addition up to i915/Makefile from i915/gvt
>  > >>> ---
>  > >>>    drivers/gpu/drm/i915/Makefile      |  4 ++++
>  > >>>    drivers/gpu/drm/i915/i915_params.c | 35 
> ++++++++++++++++++++++++++++++
>  > >>
>  > >> Can this work if put under gvt/ or at least intel_gvt.h|c?
> 
> I tried this.
> I moved the code block into gvt/debug.c (new file)
> added it to Makefile GVT_SOURCES
> dunno why it wont make.
> frustratig basic err, Im not seeing.
> It does seem proper placement, will resolve...
> 
> 
>  > >>
>  > >
>  > > I thought it belonged here more, at least according to the name of the
>  > > config.var
>  >
>  > Hmm bear with me please - the categories this patch creates are intended
>  > to be used explicitly from the GVT "sub-module", or they somehow even
>  > get automatically used with no further intervention to callers required?
>  >
> 
> 2009 - v5.9.0  the only users were admins reading/echoing 
> /proc/dynamic_debug/control
> presumably cuz they wanted more info in the logs, episodically.
> v5.9.0 exported dynamic_debug_exec_queries for in-kernel use,
> reusing the stringy: echo $query_command > control  idiom.
> My intention was to let in-kernel users roll their own drm.debug type 
> interface,
> or whatever else they needed.  nobodys using it yet.

What is 2009 referring to?

I am still not quite following. In case of the GVT categories you added, 
in order for them to be used, would you or not also need to use some new 
logging macros?

> patch 1/8 implements that drm.debug interface.
> 5/8 is the primary use case
> 3/8 (this patch) & 4/8 are patches of opportunity, test cases, proof of 
> function/utility.
> its value as such is easier control of those pr-debugs than given by 
> echo > control
> 
> Sean Paul seanpaul@chromium.org <mailto:seanpaul@chromium.org> worked up 
> a patchset to do runtime steering of drm-debug stream,
> in particular watching for drm:atomic:fail: type activity (a subcategory 
> which doesnt exist yet).
> 5/8 conflicts with his patchset, I have an rfc approach to that, so his 
> concerns are mine too.

What kind of runtime steering is that - would you happen to have a link?

One idea we in the GEM team have mentioned a few time is the ability of 
making the debug log stream per DRM client. That means opening 
"something" (socket, fd, etc), enabling debug, which would only show 
debug logs belonging to one client. That can sometimes be useful (and 
more secure) than enabling a lot of debug for the system as a whole. But 
of course there isn't much overlap with your dyndbg work. So just 
mentioning this since the word "runtime steering" reminded me of it.

<snip>

>      > unsigned long __gvt_debug;
>      > EXPORT_SYMBOL(__gvt_debug);
>      >
>      >
>      >>> +
>      >>>    # Please keep these build lists sorted!
>      >>>
>      >>>    # core driver code
>      >>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>     b/drivers/gpu/drm/i915/i915_params.c
>      >>> index e07f4cfea63a..e645e149485e 100644
>      >>> --- a/drivers/gpu/drm/i915/i915_params.c
>      >>> +++ b/drivers/gpu/drm/i915/i915_params.c
>      >>> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params
>     *params)
>      >>> +                             _DD_cat_("gvt:mmio:"),
>      >>> +                             _DD_cat_("gvt:render:"),
>      >>> +                             _DD_cat_("gvt:sched:"));
>      >>> +
>      >>> +#endif
>      >>
>      >> So just the foundation - no actual use sites I mean? How would
>     these be
>      >> used from the code?
>      >>
>      >
>      > there are 120 pr_debug "users" :-)
>      >
>      > no users per se, but anyone using drm.debug
>      > /sys/module/drm/parameters/debug
>      > might use this too.
>      > its a bit easier than composing queries for
>      >/proc/dyamic_debug/control
> 
>     Same as my previous question, perhaps I am not up to speed with this
>     yet.. Even if pr_debug is used inside GVT - are the categories and
>     debug_gvt global as of this patch (or series)?
> 
> 
> they are already global in the sense that if kernel is built with 
> DYNAMIC_DEBUG,
> the admin can turn those pr_debugs on and off, and change their 
> decorations in the log (mod,func.line).
> Nor are modules protected from each other; drm-core could use 
> dd-exec-queries to enable/disable
> pr-debugs in i915 etc
> 
> This patch just adds a gvt-debug knob like drm-debug. using the existing 
> format prefixes to categorize them.
> Whether those prefixes should be bent towards consistency with the rest 
> of drm-debug
> or adapted towards some gvt community need I couldnt say.
> 
> Its no save-the-world feature, but its pretty cheap.
> 
> Id expect the same users as those who play with drm.debug, for similar 
> reasons.
> 
> does this clarify ?

Not yet I'm afraid. :) When you say "using the existing format 
prefixes", but it is not using __drm_debug but __gvt_debug (which isn't 
a modparam). So I am lost as to what is __gvt_debug for and how does it 
tie to existing DRM categories.

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Intel-gfx] [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
  2021-09-07 15:14           ` Tvrtko Ursulin
@ 2021-09-07 17:26             ` jim.cromie
  0 siblings, 0 replies; 19+ messages in thread
From: jim.cromie @ 2021-09-07 17:26 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Sean Paul, Jason Baron, Daniel Vetter, Greg KH, LKML, dri-devel,
	amd-gfx mailing list, intel-gvt-dev, Intel Graphics Development

On Tue, Sep 7, 2021 at 9:14 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 06/09/2021 18:41, jim.cromie@gmail.com wrote:
> > On Mon, Sep 6, 2021 at 6:26 AM Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com <mailto:tvrtko.ursulin@linux.intel.com>>
> > wrote:
> >  >
> >  >
> >  > On 03/09/2021 20:22, jim.cromie@gmail.com
> > <mailto:jim.cromie@gmail.com> wrote:
> >  > > On Fri, Sep 3, 2021 at 5:07 AM Tvrtko Ursulin
> >  > > <tvrtko.ursulin@linux.intel.com
> > <mailto:tvrtko.ursulin@linux.intel.com>> wrote:
> >  > >>
> >  > >>
> >  > >> On 31/08/2021 21:21, Jim Cromie wrote:
> >  > >>> The gvt component of this driver has ~120 pr_debugs, in 9 categories
> >  > >>> quite similar to those in DRM.  Following the interface model of
> >  > >>> drm.debug, add a parameter to map bits to these categorizations.
> >  > >>>
> >  > >>> DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug_gvt, __gvt_debug,
> >  > >>>        "dyndbg bitmap desc",
> >  > >>>        { "gvt:cmd: ",  "command processing" },
> >
> >  > >>> v7:
> >  > >>> . move ccflags addition up to i915/Makefile from i915/gvt
> >  > >>> ---
> >  > >>>    drivers/gpu/drm/i915/Makefile      |  4 ++++
> >  > >>>    drivers/gpu/drm/i915/i915_params.c | 35
> > ++++++++++++++++++++++++++++++
> >  > >>
> >  > >> Can this work if put under gvt/ or at least intel_gvt.h|c?
> >
> > I tried this.
> > I moved the code block into gvt/debug.c (new file)
> > added it to Makefile GVT_SOURCES
> > dunno why it wont make.
> > frustratig basic err, Im not seeing.
> > It does seem proper placement, will resolve...
> >
> >
> >  > >>
> >  > >
> >  > > I thought it belonged here more, at least according to the name of the
> >  > > config.var
> >  >
> >  > Hmm bear with me please - the categories this patch creates are intended
> >  > to be used explicitly from the GVT "sub-module", or they somehow even
> >  > get automatically used with no further intervention to callers required?
> >  >
> >
> > 2009 - v5.9.0  the only users were admins reading/echoing
> > /proc/dynamic_debug/control
> > presumably cuz they wanted more info in the logs, episodically.
> > v5.9.0 exported dynamic_debug_exec_queries for in-kernel use,
> > reusing the stringy: echo $query_command > control  idiom.
> > My intention was to let in-kernel users roll their own drm.debug type
> > interface,
> > or whatever else they needed.  nobodys using it yet.
>
> What is 2009 referring to?
>
> I am still not quite following. In case of the GVT categories you added,
> in order for them to be used, would you or not also need to use some new
> logging macros?
>
> > patch 1/8 implements that drm.debug interface.
> > 5/8 is the primary use case
> > 3/8 (this patch) & 4/8 are patches of opportunity, test cases, proof of
> > function/utility.
> > its value as such is easier control of those pr-debugs than given by
> > echo > control
> >
> > Sean Paul seanpaul@chromium.org <mailto:seanpaul@chromium.org> worked up
> > a patchset to do runtime steering of drm-debug stream,
> > in particular watching for drm:atomic:fail: type activity (a subcategory
> > which doesnt exist yet).
> > 5/8 conflicts with his patchset, I have an rfc approach to that, so his
> > concerns are mine too.
>
> What kind of runtime steering is that - would you happen to have a link?

Sean's patches
https://patchwork.freedesktop.org/series/78133/

what I think might work better
https://lore.kernel.org/dri-devel/20210822222009.2035788-11-jim.cromie@gmail.com/

> One idea we in the GEM team have mentioned a few time is the ability of
> making the debug log stream per DRM client. That means opening
> "something" (socket, fd, etc), enabling debug, which would only show
> debug logs belonging to one client. That can sometimes be useful (and
> more secure) than enabling a lot of debug for the system as a whole. But
> of course there isn't much overlap with your dyndbg work. So just
> mentioning this since the word "runtime steering" reminded me of it.
>

my rfc patch above might help. it adds
   register_dyndbg_tracer ( selector_query, handler_callback)

I think you could write a single handler to further select / steer the
debug stream
according to your pr_debug arguments.


>
> >      > unsigned long __gvt_debug;
> >      > EXPORT_SYMBOL(__gvt_debug);
> >      >
> >      >
> >      >>> +
> >      >>>    # Please keep these build lists sorted!
> >      >>>
> >      >>>    # core driver code
> >      >>> diff --git a/drivers/gpu/drm/i915/i915_params.c
> >     b/drivers/gpu/drm/i915/i915_params.c
> >      >>> index e07f4cfea63a..e645e149485e 100644
> >      >>> --- a/drivers/gpu/drm/i915/i915_params.c
> >      >>> +++ b/drivers/gpu/drm/i915/i915_params.c
> >      >>> @@ -265,3 +265,38 @@ void i915_params_free(struct i915_params
> >     *params)
> >      >>> +                             _DD_cat_("gvt:mmio:"),
> >      >>> +                             _DD_cat_("gvt:render:"),
> >      >>> +                             _DD_cat_("gvt:sched:"));
> >      >>> +
> >      >>> +#endif
> >      >>
> >      >> So just the foundation - no actual use sites I mean? How would
> >     these be
> >      >> used from the code?
> >      >>
> >      >
> >      > there are 120 pr_debug "users" :-)
> >      >
> >      > no users per se, but anyone using drm.debug
> >      > /sys/module/drm/parameters/debug
> >      > might use this too.
> >      > its a bit easier than composing queries for
> >      >/proc/dyamic_debug/control
> >
> >     Same as my previous question, perhaps I am not up to speed with this
> >     yet.. Even if pr_debug is used inside GVT - are the categories and
> >     debug_gvt global as of this patch (or series)?
> >
> >
> > they are already global in the sense that if kernel is built with
> > DYNAMIC_DEBUG,
> > the admin can turn those pr_debugs on and off, and change their
> > decorations in the log (mod,func.line).
> > Nor are modules protected from each other; drm-core could use
> > dd-exec-queries to enable/disable
> > pr-debugs in i915 etc
> >
> > This patch just adds a gvt-debug knob like drm-debug. using the existing
> > format prefixes to categorize them.
> > Whether those prefixes should be bent towards consistency with the rest
> > of drm-debug
> > or adapted towards some gvt community need I couldnt say.
> >
> > Its no save-the-world feature, but its pretty cheap.
> >
> > Id expect the same users as those who play with drm.debug, for similar
> > reasons.
> >
> > does this clarify ?
>
> Not yet I'm afraid. :)

heh - 2 blind dudes describing their side of the elephant !

When you say "using the existing format
> prefixes", but it is not using __drm_debug but __gvt_debug (which isn't
> a modparam). So I am lost as to what is __gvt_debug for and how does it
> tie to existing DRM categories.
>

we have 2 kinds of "categories":
1- proper drm categories - one of 10
2- ad-hoc categories - these are systematized - using small set of
format-prefixes
i915 has 120 of these in GVT, with 9 different prefixes, touched in patches 2,3
i915 also has ~1500 uses of drm-debug API  (with proper drm category enums)
amdgpu also has lots of both kinds of debug; 13 kinds of prefixes.

Ive probably created some confusion by stealing the "category" name,
it could have been "class", but I thought we didnt need new vocabulary with
subtle and ambiguous differences from the original term.

Long term, maybe those ad-hoc prefixes can be aligned better with proper drm
categories, but thats a separate discussion.

But we can control them now, using a well oiled idiom, a drm.debug
"style" bitmap.
Its like drm.debug's little sisters, __gvt_debug & __debug_dc.  not
identical, but related.

Anyway, patches 2,3,4 work the ad-hoc prefix categorizations so theyre
controllable.

5/8 adapts DRM-debug itself - obsoleting enum category for most of drm-debug api
this is where dyndbg's data table gets bigger - core & drivers! have
many drm-debug uses,
rivaling all builtin prdebugs in size.

Then, we have a unified foundation on dyndbg, using glorified prefix strings
for both formal DRM_DBG_CAT_* categories, and for similar existing uses.

Then we could evolve / extend / bikeshed the categories (spelling,
separator char '.' is nice too)

Sean has already suggested "drm:atomic:fail:" sub-category.
I agree - it is using the new stringy flexibility to significant
expressive benefit.
dyndbg makes new categories actionable.

istm "*:fail:" is maybe a meta-sub-category (dont read that too closely;)
maybe "*:warn:" "*:err:" ( what about warning, error ? here lies
bikeshed madness !!)

> Regards,
>
> Tvrtko

thanks
JIm

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-09-07 17:27 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 20:21 [PATCH v7 0/8] use DYNAMIC_DEBUG to implement DRM.debug Jim Cromie
2021-08-31 20:21 ` [PATCH v7 1/8] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
2021-08-31 20:21 ` [PATCH v7 2/8] dyndbg: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
2021-08-31 20:21 ` [PATCH v7 3/8] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
2021-09-03 11:07   ` [Intel-gfx] " Tvrtko Ursulin
2021-09-03 19:22     ` jim.cromie
2021-09-06 12:26       ` Tvrtko Ursulin
     [not found]         ` <CAJfuBxymoFx79kQzGw_Gxv1vk7kVaTN-V0Hn694C6kT=kP7u2A@mail.gmail.com>
2021-09-07 15:14           ` Tvrtko Ursulin
2021-09-07 17:26             ` jim.cromie
2021-08-31 20:21 ` [PATCH v7 4/8] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES Jim Cromie
2021-08-31 20:21 ` [PATCH v7 5/8] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
2021-09-03 11:15   ` [Intel-gfx] " Tvrtko Ursulin
2021-09-03 21:57     ` jim.cromie
2021-09-06 10:20       ` Tvrtko Ursulin
2021-09-06 18:24         ` jim.cromie
2021-08-31 20:21 ` [PATCH v7 6/8] drm_print: instrument drm_debug_enabled Jim Cromie
2021-09-05 18:50   ` jim.cromie
2021-08-31 20:21 ` [PATCH v7 7/8] amdgpu_ucode: reduce number of pr_debug calls Jim Cromie
2021-08-31 20:21 ` [PATCH v7 8/8] nouveau: fold multiple DRM_DEBUG_DRIVERs together Jim Cromie

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).