linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/7] drm: use dyndbg in drm_print
@ 2021-07-31 21:41 Jim Cromie
  2021-07-31 21:41 ` [PATCH v4 1/7] drm/print: fixup spelling in a comment Jim Cromie
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:41 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Wyatt Wood, Qingqing Zhuo,
	Aurabindo Pillai, Jim Cromie, Johan Hovold, Jessica Yu,
	Miguel Ojeda, Nick Desaulniers, Joe Perches, dri-devel,
	linux-kernel, amd-gfx, intel-gvt-dev, intel-gfx

hi all,

Apologies for broad --to, but it touches a wide range, if casually.

In order to avoid runtime costs of drm_debug_enabled(), this patchset
re-implements drm.debug to use dyndbg, after extending dyndbg with
kernel_param_ops to implement the bitmap->dydnbg-query and expose it
for use.

To define your pr_debugs categorization (that you want to control):

DEFINE_DYNDBG_BITMAP(debug_gvt, __gvt_debug,
	"i915/gvt dyndbg bitmap desc",
	/* bits 0-N are initialized with these exprs */
	{ "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" });

Then, you can change whole categories of pr_debugs:

  echo 0x5A > /sys/modules/i915/parameters/debug_gvt

BIT($i++) is implied by order, can it be autogend with macros and
stuffed into MODULE_PARAM_DESC automagically ?

v4:
 - extend kernel_param with void* .data (2/7)
 - move v3 kernel_param_ops code into dyndbg (3/7)
 - add DEFINE_DYNDBG_BITMAP (3/7)
 - use DEFINE_DYNDBG_BITMAP in drm-core, i915 (5/7), amdgpu (7/7)
 - lots of changes per @DanVet feedback, thanks!
   no doc yet, content more in commit-log refinements

v3: https://lore.kernel.org/lkml/20210714175138.319514-1-jim.cromie@gmail.com/
    main element is now patch 6/7, 
v2: https://lore.kernel.org/lkml/20210711055003.528167-1-jim.cromie@gmail.com/
v1: https://lore.kernel.org/lkml/20201204035318.332419-1-jim.cromie@gmail.com/

NOTES:

https://github.com/jimc/linux/tree/dd-drm-v4 should be seen and tested
by robots soon.

Using dyndbg to implement categories/classes of pr_debugs requires
that the class-prefix is compiled into the format string, and not
written at runtime with pr_debug("%s %pV", class_str, vaf).

That caveat is reflected in (6/7) where category is catenated with the
format, and ceases to exist as a separate arg for DRM_USE_DD=y builds.
ISTM this also means drm_debug_enabled() isnt going away just yet.

The categories arent atomic in any sense; callsites controlled via
drm.debug can still be tweaked arbitrarily with echo $query > control.

dyndbg's search is global by default; format ^$prefix cuts across
code modules, and anyone can add a pr_debug("drm:foobar: yada")
classification.  Rules against this are policy, and belong in the
realm of author taste and code review, not in the tool.

Maybe modname's already available via kp, if we need it.  Other query
terms are possible, if struct dyndbg_bitdesc is elaborated.

The dyndbg-queries built by the callback don't include modname.  This
allows drm.dbg to control pr-debugs inside i915, despite being done
from a different module.  This is ok because i915 used the drm_debug
api for those messages.

i915/gvt uses a different way to prepend its classes, but it shares
the classification space.  Like a good namespace citizen, it has
"gvt:" as its top-level name, with ~9 2nd level names.

sean@chromium.org posited "drm:atomic:fail: " as a new category, to
leverage the additional selectivity of dyndbg.

Allowing for N-level namespaces, ' ' terminates the prefix, not ':'.
So "drm:atomic:" & "drm:atomic: " are different searches, 1st
including pr_debug("drm:atomic:fail: ")

DEFINE_DYNDBG_BITMAP() bits and masks are dependent on order of
bit-declarations.  Since bits are applied 0-N, later bits could
override (ie sub-categorize) earlier ones.  IOW, "drm:atomic:fail: "
just needs to be after "drm:atomic: ".



dyndbg is extremely agnostic about how you use format ^$prefix, your
classification scheme just needs to work within the limitation that
its a simple anchored literal substring match, no regex stuff.

In particular, commit 578b1e0701af ("dynamic_debug: add wildcard
support to filter files/functions/modules") left format out, because
it was already special (floating substr match, no need for '*').

Patch 6/7 has many more "advices" on classification / categorization.

and im restating things.

Much more to say/do instead of what follows, which is v3 prose.
I wanted to send, see how its received by robots and people.


drm_debug_enabled() is called a lot to do unlikely bit-tests to
selectively enable debug printing; this is a good job for
dynamic-debug, IFF it is built with JUMP_LABEL.
 
This patchset enables the use of dynamic-debug to avoid all those
drm_debug_enabled() overheads, if CONFIG_DRM_USE_DYNAMIC_DEBUG=y.


Doing so creates many new pr_debug callsites,
otherwise i915 has ~120 prdbgs, and drm has just 1;

  bash-5.1# modprobe i915
  dyndbg:   8 debug prints in module video
  dyndbg: 305 debug prints in module drm
  dyndbg: 207 debug prints in module drm_kms_helper
  dyndbg:   2 debug prints in module ttm
  dyndbg: 1720 debug prints in module i915

On amdgpu, enabling it adds ~3200 prdbgs, currently at 56 bytes each.
So CONFIG_DRM_USE_DYNAMIC_DEBUG=y affects resource requirements.

Im running this patchset bare-metal on an i915 laptop & an amdgpu
desktop (both as loadable modules).  I booted the amdgpu box with:

BOOT_IMAGE=(hd2,gpt2)/vmlinuz-5.13.0-dd7-13692-g8def25788f56 \
     root=UUID=mumble ro \
     rootflags=subvol=root00 rhgb \
     dynamic_debug.verbose=3 main.dyndbg=+p \
     amdgpu.debug=1 amdgpu.test=1 \
     "amdgpu.dyndbg=format ^[ +p"

That last line enables ~1700 prdbg callsites with a format like '[DML'
etc at boot, and amdgpu.test=1 triggers 3 minutes of tests, causing
~76k prdbgs in 409 seconds, before I turned them off with:

  echo module amdgpu -p > /proc/dynamic_debug/control

This is on top of master @ v5.14-rc3.

Jim Cromie (7):
  drm/print: fixup spelling in a comment (already in drm-mumble)
  moduleparam: add data member to struct kernel_param
  dyndbg: add dyndbg-bitmap definer and callbacks
  i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes
  i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt
  drm/print: add choice to use dynamic debug in drm-debug
  amdgpu: define a dydbg-bitmap to control categorized pr_debugs

 drivers/gpu/drm/Kconfig                       | 13 +++
 .../gpu/drm/amd/display/dc/core/dc_debug.c    | 42 +++++++-
 drivers/gpu/drm/drm_print.c                   | 15 ++-
 drivers/gpu/drm/i915/gvt/Makefile             |  4 +
 drivers/gpu/drm/i915/gvt/debug.h              | 18 ++--
 drivers/gpu/drm/i915/i915_params.c            | 34 +++++++
 include/drm/drm_print.h                       | 99 ++++++++++++++-----
 include/linux/dynamic_debug.h                 | 36 +++++++
 include/linux/moduleparam.h                   | 11 ++-
 lib/dynamic_debug.c                           | 55 +++++++++++
 10 files changed, 284 insertions(+), 43 deletions(-)

-- 
2.31.1


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

* [PATCH v4 1/7] drm/print: fixup spelling in a comment
  2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
@ 2021-07-31 21:41 ` Jim Cromie
  2021-07-31 21:41 ` [PATCH v4 2/7] moduleparam: add data member to struct kernel_param Jim Cromie
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:41 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Qingqing Zhuo, Wyatt Wood,
	Aurabindo Pillai, Jim Cromie, Johan Hovold, Jessica Yu,
	Nick Desaulniers, Joe Perches, Miguel Ojeda, dri-devel,
	linux-kernel, amd-gfx, intel-gvt-dev, intel-gfx

s/prink/printk/ - no functional changes

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/drm/drm_print.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 9b66be54dd16..15a089a87c22 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -327,7 +327,7 @@ static inline bool drm_debug_enabled(enum drm_debug_category category)
 /*
  * struct device based logging
  *
- * Prefer drm_device based logging over device or prink based logging.
+ * Prefer drm_device based logging over device or printk based logging.
  */
 
 __printf(3, 4)
-- 
2.31.1


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

* [PATCH v4 2/7] moduleparam: add data member to struct kernel_param
  2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
  2021-07-31 21:41 ` [PATCH v4 1/7] drm/print: fixup spelling in a comment Jim Cromie
@ 2021-07-31 21:41 ` Jim Cromie
  2021-08-02 16:18   ` Emil Velikov
  2021-07-31 21:42 ` [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks Jim Cromie
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:41 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Aurabindo Pillai, Qingqing Zhuo,
	Wyatt Wood, Jim Cromie, Jessica Yu, Johan Hovold, Joe Perches,
	Miguel Ojeda, Nick Desaulniers, dri-devel, linux-kernel, amd-gfx,
	intel-gvt-dev, intel-gfx

Add a void* data member to the struct, to allow attaching private data
that will be used soon by a setter method (via kp->data) to perform
more elaborate actions.

To attach the data at compile time, add new macros:
module_param_cbd() derives from module_param_cb(), adding data param.
It calls __module_param_call_wdata(), which has accepts new data
param and inits .data with it. Re-define __module_param_call() using it.

Use of this new data member will be rare, it might be worth redoing
this as a separate/sub-type to keep the base case.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/moduleparam.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index eed280fae433..e9495b1e794d 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -78,6 +78,7 @@ struct kernel_param {
 		const struct kparam_string *str;
 		const struct kparam_array *arr;
 	};
+	void *data;
 };
 
 extern const struct kernel_param __start___param[], __stop___param[];
@@ -175,6 +176,9 @@ struct kparam_array
 #define module_param_cb(name, ops, arg, perm)				      \
 	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0)
 
+#define module_param_cbd(name, ops, arg, perm, data)				\
+	__module_param_call_wdata(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0, data)
+
 #define module_param_cb_unsafe(name, ops, arg, perm)			      \
 	__module_param_call(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1,    \
 			    KERNEL_PARAM_FL_UNSAFE)
@@ -284,14 +288,17 @@ struct kparam_array
 
 /* This is the fundamental function for registering boot/module
    parameters. */
-#define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
+#define __module_param_call(prefix, name, ops, arg, perm, level, flags) \
+	__module_param_call_wdata(prefix, name, ops, arg, perm, level, flags, NULL)
+
+#define __module_param_call_wdata(prefix, name, ops, arg, perm, level, flags, data) \
 	/* Default value instead of permissions? */			\
 	static const char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used __section("__param")					\
 	__aligned(__alignof__(struct kernel_param))			\
 	= { __param_str_##name, THIS_MODULE, ops,			\
-	    VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg } }
+	    VERIFY_OCTAL_PERMISSIONS(perm), level, flags, { arg }, data }
 
 /* Obsolete - use module_param_cb() */
 #define module_param_call(name, _set, _get, arg, perm)			\
-- 
2.31.1


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

* [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks
  2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
  2021-07-31 21:41 ` [PATCH v4 1/7] drm/print: fixup spelling in a comment Jim Cromie
  2021-07-31 21:41 ` [PATCH v4 2/7] moduleparam: add data member to struct kernel_param Jim Cromie
@ 2021-07-31 21:42 ` Jim Cromie
  2021-08-02 16:24   ` [Intel-gfx] " Emil Velikov
  2021-07-31 21:42 ` [PATCH v4 4/7] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:42 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai,
	Wyatt Wood, Jim Cromie, Johan Hovold, Jessica Yu, Joe Perches,
	Miguel Ojeda, Nick Desaulniers, dri-devel, linux-kernel, amd-gfx,
	intel-gvt-dev, intel-gfx

Add DEFINE_DYNDBG_BITMAP(name, var, bitmap_desc, @bit_descs) to allow
users to define a /sys/module/*/parameter/name, and a mapping from
bits[0-N] to the debug-class-prefixes that the author wishes to
control.

DEFINE_DYNDBG_BITMAP(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" });

dynamic_debug.c: add 3 new elements:

- int param_set_dyndbg() - not working yet, // __gvt_debug
- int param_get_dyndbg()
- struct param_ops_dyndbg

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

dynamic_debug.h:

extern the struct param_ops_dyndbg prototype.  This appears to be
needed to reference the var, forex in i915_params.c

TBD: set_dyndbg() works to enable categories, but fails to disable
them.  This is because the code relied on having an old copy of the
param (__gvt_debug) to detect +/- changes.  Rewriting the loop is
probably easier than stashing the old state for change detection.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h | 36 +++++++++++++++++++++++
 lib/dynamic_debug.c           | 55 +++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index dce631e678dd..677ad176b167 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -2,6 +2,8 @@
 #ifndef _DYNAMIC_DEBUG_H
 #define _DYNAMIC_DEBUG_H
 
+#include <linux/moduleparam.h>
+
 #if defined(CONFIG_JUMP_LABEL)
 #include <linux/jump_label.h>
 #endif
@@ -227,6 +229,40 @@ static inline int dynamic_debug_exec_queries(const char *query, const char *modn
 	return 0;
 }
 
+static int param_set_dyndbg(const char *instr, struct kernel_param *kp)
+{ return 0; }
+static int param_get_dyndbg(char *buffer, struct kernel_param *kp)
+{ return 0; }
+
 #endif /* !CONFIG_DYNAMIC_DEBUG_CORE */
 
+struct dyndbg_bitdesc {
+	/* bitpos is inferred from index in containing array */
+	char *prefix;
+	char *help;
+};
+
+/**
+ * DYNDBG_BITMAP_DESC(name, var, bitmap_desc, @bit_descs)
+ * @name: basename under /sys
+ * @var: C identifier to map
+ * @bitmap_desc: string summarizing dyndbg categories
+ * @bit_descs: list of struct dydbg_bitdesc initializations
+ *
+ * Defines the mapping of bits 0-N to categories/prefixes of
+ * debug-callsites to be controlled.
+ *
+ * Users should also call MODULE_PARM_DESC(name, bitmap_desc).
+ * Maybe we can invoke it on their behalf, but we want MOD-NAME to be
+ * correct, test soon.  may also need modname in name - "debug" will
+ * not be unique.
+ */
+#define DEFINE_DYNDBG_BITMAP(name, value, bitmap_desc, ...)	\
+	struct dyndbg_bitdesc dyndbg_classes_##name[] =		\
+	{ __VA_ARGS__, { NULL, NULL } };			\
+	module_param_cbd(name, &param_ops_dyndbg, value, 0644,	\
+			 &dyndbg_classes_##name);
+
+extern const struct kernel_param_ops param_ops_dyndbg;
+
 #endif
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index cb5abb42c16a..045e1cf92c44 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1154,3 +1154,58 @@ early_initcall(dynamic_debug_init);
 
 /* Debugfs setup must be done later */
 fs_initcall(dynamic_debug_init_control);
+
+#include <linux/moduleparam.h>
+
+#define OUR_QUERY_SIZE 128 /* typically need <40 */
+
+int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
+{
+	unsigned int val;
+	unsigned long changes, result;
+	int rc, chgct = 0, totct = 0, bitpos, bitsmax;
+	char query[OUR_QUERY_SIZE];
+	struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
+
+	// pr_info("set_dyndbg: instr: %s curr: %d\n", instr, *kp->arg);
+
+	rc = kstrtouint(instr, 0, &val);
+	if (rc) {
+		pr_err("set_dyndbg: failed\n");
+		return -EINVAL;
+	}
+	result = val;
+	pr_info("set_dyndbg: result:0x%lx from %s\n", result, instr);
+
+	changes = result; // ^ __gvt_debug;
+
+	for (bitsmax = 0; bitmap[bitsmax].prefix; bitsmax++);
+
+	for_each_set_bit(bitpos, &changes, min(--bitsmax, 64)) {
+
+		sprintf(query, "format '^%s' %cp", bitmap[bitpos].prefix,
+			test_bit(bitpos, &result) ? '+' : '-');
+
+		chgct = dynamic_debug_exec_queries(query, "i915");
+
+		pr_info("bit-%d: %d changes on: %s\n", bitpos, chgct, query);
+		totct += chgct;
+	}
+	pr_info("total changes: %d\n", totct);
+	// __gvt_debug = result;
+	return 0;
+}
+EXPORT_SYMBOL(param_set_dyndbg);
+
+int param_get_dyndbg(char *buffer, const struct kernel_param *kp)
+{
+	return scnprintf(buffer, PAGE_SIZE, "%u\n",
+			 *((unsigned int *)kp->arg));
+}
+EXPORT_SYMBOL(param_get_dyndbg);
+
+const struct kernel_param_ops param_ops_dyndbg = {
+	.set = param_set_dyndbg,
+	.get = param_get_dyndbg,
+};
+EXPORT_SYMBOL(param_ops_dyndbg);
-- 
2.31.1


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

* [PATCH v4 4/7] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes
  2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
                   ` (2 preceding siblings ...)
  2021-07-31 21:42 ` [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks Jim Cromie
@ 2021-07-31 21:42 ` Jim Cromie
  2021-07-31 21:42 ` [PATCH v4 5/7] i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt Jim Cromie
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:42 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Aurabindo Pillai, Wyatt Wood,
	Qingqing Zhuo, Jim Cromie, Jessica Yu, Johan Hovold,
	Miguel Ojeda, Joe Perches, Nick Desaulniers, dri-devel,
	linux-kernel, amd-gfx, intel-gvt-dev, intel-gfx

Collapsing "gvt: core: " to "gvt:core: " is a better class-prefix;
dropping the internal spaces means a trailing space in a query will
more clearly terminate the prefix.

Consider a generic drm-debug example:

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

  # turn off all ATOMIC:* reports
  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:".

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 related	[flat|nested] 13+ messages in thread

* [PATCH v4 5/7] i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt
  2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
                   ` (3 preceding siblings ...)
  2021-07-31 21:42 ` [PATCH v4 4/7] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
@ 2021-07-31 21:42 ` Jim Cromie
  2021-08-02 16:27   ` Emil Velikov
  2021-07-31 21:42 ` [PATCH v4 6/7] drm/print: add choice to use dynamic debug in drm-debug Jim Cromie
  2021-07-31 21:42 ` [PATCH v4 7/7] amdgpu: define a dydbg-bitmap to control categorized pr_debugs Jim Cromie
  6 siblings, 1 reply; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:42 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai,
	Wyatt Wood, Jim Cromie, Jessica Yu, Johan Hovold, Miguel Ojeda,
	Nick Desaulniers, Joe Perches, dri-devel, linux-kernel, amd-gfx,
	intel-gvt-dev, intel-gfx

The gvt component of this driver has ~120 pr_debugs, in 9 "classes".
Following the interface model of drm.debug, add a parameter to map
bits to these classes.

If CONFIG_DRM_USE_DYNAMIC_DEBUG=y (and CONFIG_DYNAMIC_DEBUG_CORE), add
-DDYNAMIC_DEBUG_MODULE into Makefile.  TBD: maybe add a separate
CONFIG_I915_USE_DYNAMIC_DEBUG to more fully optionalize this.

this is close to our target:

DYNDBG_BITMAP_DESC(__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" });

actual patch has a few details different, helper macros mainly.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/i915/gvt/Makefile  |  4 ++++
 drivers/gpu/drm/i915/i915_params.c | 34 ++++++++++++++++++++++++++++++
 include/drm/drm_print.h            |  3 ++-
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile
index ea8324abc784..846ba73b8de6 100644
--- a/drivers/gpu/drm/i915/gvt/Makefile
+++ b/drivers/gpu/drm/i915/gvt/Makefile
@@ -7,3 +7,7 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \
 
 ccflags-y				+= -I $(srctree)/$(src) -I $(srctree)/$(src)/$(GVT_DIR)/
 i915-y					+= $(addprefix $(GVT_DIR)/, $(GVT_SOURCE))
+
+#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
+ccflags-y	+= -DDYNAMIC_DEBUG_MODULE
+#endif
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index e07f4cfea63a..c951ef76454f 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -265,3 +265,37 @@ void i915_params_free(struct i915_params *params)
 	I915_PARAMS_FOR_EACH(FREE);
 #undef FREE
 }
+
+/* POC for callback -> dynamic_debug_exec_queries */
+unsigned long __gvt_debug;
+EXPORT_SYMBOL(__gvt_debug);
+
+#define _help(key)	"\t\"" key "\"\t: help for " key "\n"
+#define cmd_help(key)	{ .prefix = key, .help = key ": help for " key }
+
+#define I915_DYNDBG_PARM_DESC(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:")
+
+MODULE_PARM_DESC(debug_gvt, I915_DYNDBG_PARM_DESC(debug_gvt));
+
+DEFINE_DYNDBG_BITMAP(debug_gvt, &__gvt_debug,
+		   I915_DYNDBG_PARM_DESC(debug_gvt),
+		   cmd_help("gvt:cmd:"),
+		   cmd_help("gvt:core:"),
+		   cmd_help("gvt:dpy:"),
+		   cmd_help("gvt:el:"),
+		   cmd_help("gvt:irq:"),
+		   cmd_help("gvt:mm:"),
+		   cmd_help("gvt:mmio:"),
+		   cmd_help("gvt:render:"),
+		   cmd_help("gvt:sched:"));
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 15a089a87c22..47803c64b144 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -530,7 +530,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_CLASS_ ## category, fmt, ##__VA_ARGS__);			\
 })
 
 #define drm_dbg_kms_ratelimited(drm, fmt, ...) \
-- 
2.31.1


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

* [PATCH v4 6/7] drm/print: add choice to use dynamic debug in drm-debug
  2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
                   ` (4 preceding siblings ...)
  2021-07-31 21:42 ` [PATCH v4 5/7] i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt Jim Cromie
@ 2021-07-31 21:42 ` Jim Cromie
  2021-07-31 21:42 ` [PATCH v4 7/7] amdgpu: define a dydbg-bitmap to control categorized pr_debugs Jim Cromie
  6 siblings, 0 replies; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:42 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai,
	Wyatt Wood, Jim Cromie, Jessica Yu, Johan Hovold, Joe Perches,
	Miguel Ojeda, Nick Desaulniers, dri-devel, linux-kernel, amd-gfx,
	intel-gvt-dev, intel-gfx

drm's debug system writes 10 distinct categories of messages to syslog
using a small API[1]: drm_dbg*(10 names), DRM_DEBUG*(8 names),
DRM_DEV_DEBUG*(3 names).  There are thousands of these callsites, each
categorized by their authors.  ~2100 are on my laptop.

These callsites are 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 each time an API[1] call is executed; while cheap individually,
the costs accumulate.

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.

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 every pr_debug("drm:core: ...") with one query.

This conversion yields ~2100 new callsites on my i7/i915 laptop:

  dyndbg: 195 debug prints in module drm_kms_helper
  dyndbg: 298 debug prints in module drm
  dyndbg: 1630 debug prints in module i915

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 indirection/switchover is layered into the macro scheme:

0. A new callback on drm.debug (in a previous patch)
   does: dynamic_debug_exec_queries("format ^drm:kms: +p", "drm*");
   DYNDBG_BITMAP_DESC(__drm_debug, "bla blah bitmap yada",
   	  	    { "drm:core:", "enable CORE debug messages" },
   	  	    { "drm:kms:", "enable KMS debug messages" });

1. A "converted" or "classy" DRM_UT_* map

   based on:  DRM_UT_* ( symbol => bit-mask )
   named it:  DRM_DBG_CLASS_* ( symbol => format-class-prefix-string )

   So DRM_DBG_CLASS_* is either:
   basic: DRM_DBG_CLASS_* <-- DRM_UT_*  identity map
   enabled:
    #define DRM_DBG_CLASS_KMS    "drm:kms: "
    #define DRM_DBG_CLASS_PRIME  "drm:prime: "
    #define DRM_DBG_CLASS_ATOMIC "drm:atomic: "

   DRM_UT_* are unchanged for now, since theyre used in
   drm_debug_enabled() and elsewhere.

2. drm_dev_dbg & drm_debug are renamed (prefixed with '__')

   original names are now macros, calling either:
     basic:   -> to renamed fn
     enabled: -> dev_dbg & pr_debug, with DRM_DBG_CLASS_* prefix # format.

   this is where drm_debug_enabled() is avoided.
   prefix is prepended at compile-time.

3. names in (2) are invoked by API[1]

   all these macros now use DRM_DBG_CLASS_* to get right token type
   for both !/!! DRM_USE_DYNAMIC_DEBUG cases

NOTES:

dyndbg does require that the prefix be in the compiled-in format
string; run-time prefixing would look like "format ^%s" in a query
(obviously not useful).

dyndbg is completely agnostic wrt the categorization scheme used; but
the query selectivity is completely dependent upon how the scheme fits
with a simple anchored literal substring match (no regex).

ad-hoc categories are implicitly allowed, author discipline and review
is expected.

 - "1","2","3" are allowed, 1-9 is effective max. 2 doesnt imply 1.
 - "1:","2:","3:" are better, avoiding [1-9]\d: etc
 - "todo:", "rfc:" might be handy.
 - "drm:*:" is used here.
 - "drm:*:*:" is a natural extension.
 - "drm:atomic:fail:" has been suggested.

A hierarchical category space is an obvious frontrunner, so it
deserves a little more exploration/scrutiny.

 - "drm:kms: " & "drm:kms:" are different
   (2nd, w/o trailing space, matches subcats)
 - "drm:kms" is also different
   (matches "drm:kmsmart", whatever that would be)
 - "drm:kms" & "drm:kms*" are different
   the latter does not work as you might reasonably expect.
   wildcarding not added for format, which was already special

Literal prefixes (as encoded in DRM_DBG_CLASS_* symbols) should
include the trailing space, both to terminate the class prefix, and to
read naturally in logs with plain #catenation.

Prefixes given in args to DYNDBG_BITMAP_DESC() determine the bit-query
map; so to insure the map is stable, new categories or 3rd level
categories must be added to the end.  The prefixes may have trailing
spaces, depending upon desired search results.

Since bits are applied 0-N, the later categories can countermand the
earlier ones. IOW, "drm:atomic:fail:" would override "drm:atomic:",
but "drm:atomic: " would never touch "drm:atomic:fail:" callsites.

There are plenty of bikeshed rabbit holes available.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/Kconfig     | 13 +++++
 drivers/gpu/drm/drm_print.c | 15 ++++--
 include/drm/drm_print.h     | 94 +++++++++++++++++++++++++++----------
 3 files changed, 93 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 7ff89690a976..e4524ccba040 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 n
+	depends on DRM
+	depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE
+	depends on JUMP_LABEL
+	help
+	  The drm debug category facility does a lot of unlikely bit-field
+	  tests at runtime; while cheap individually, the cost accumulates.
+	  This option uses dynamic debug facility (if configured and
+	  using jump_label) to avoid those runtime checks, patching
+	  the kernel when those debugs are desired.
+
 config DRM_DEBUG_SELFTEST
 	tristate "kselftests for DRM"
 	depends on DRM
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 111b932cf2a9..086e7ea6823f 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -52,7 +52,12 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
 "\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)");
+
+#ifndef CONFIG_DRM_USE_DYNAMIC_DEBUG
 module_param_named(debug, __drm_debug, int, 0600);
+#else
+module_param_cb(debug, &param_ops_dyndbg, &__drm_debug, 0644);
+#endif
 
 void __drm_puts_coredump(struct drm_printer *p, const char *str)
 {
@@ -256,8 +261,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 +283,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 +302,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 47803c64b144..b5256d59ac55 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -319,6 +319,51 @@ enum drm_debug_category {
 	DRM_UT_DRMRES		= 0x200,
 };
 
+#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+
+/* Use legacy drm-debug functions, and drm_debug_enabled().
+ * For DRM_DBG_CLASS_*, identity map to DRM_UT_*
+ */
+#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_CLASS_CORE	DRM_UT_CORE
+#define DRM_DBG_CLASS_DRIVER	DRM_UT_DRIVER
+#define DRM_DBG_CLASS_KMS	DRM_UT_KMS
+#define DRM_DBG_CLASS_PRIME	DRM_UT_PRIME
+#define DRM_DBG_CLASS_ATOMIC	DRM_UT_ATOMIC
+#define DRM_DBG_CLASS_VBL	DRM_UT_VBL
+#define DRM_DBG_CLASS_STATE	DRM_UT_STATE
+#define DRM_DBG_CLASS_LEASE	DRM_UT_LEASE
+#define DRM_DBG_CLASS_DP	DRM_UT_DP
+#define DRM_DBG_CLASS_DRMRES	DRM_UT_DRMRES
+
+#else /* !CONFIG_DRM_USE_DYNAMIC_DEBUG */
+
+/* use dynamic_debug to avoid drm_debug_enabled().
+ * dyndbg has no category, so we prefix the format with a "class"
+ * string; DRM_DBG_CLASS_* maps to those class strings
+ */
+#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_CLASS_CORE	"drm:core: "
+#define DRM_DBG_CLASS_DRIVER	"drm:drvr: "
+#define DRM_DBG_CLASS_KMS	"drm:kms: "
+#define DRM_DBG_CLASS_PRIME	"drm:prime: "
+#define DRM_DBG_CLASS_ATOMIC	"drm:atomic: "
+#define DRM_DBG_CLASS_VBL	"drm:vbl: "
+#define DRM_DBG_CLASS_STATE	"drm:state: "
+#define DRM_DBG_CLASS_LEASE	"drm:lease: "
+#define DRM_DBG_CLASS_DP	"drm:dp: "
+#define DRM_DBG_CLASS_DRMRES	"drm:res "
+
+#endif /* !CONFIG_DRM_USE_DYNAMIC_DEBUG */
+
 static inline bool drm_debug_enabled(enum drm_debug_category category)
 {
 	return unlikely(__drm_debug & category);
@@ -334,8 +379,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 +428,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_CLASS_CORE, fmt, ##__VA_ARGS__)
 /**
  * DRM_DEV_DEBUG_DRIVER() - Debug output for vendor specific part of the driver
  *
@@ -391,7 +436,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_CLASS_DRIVER, fmt, ##__VA_ARGS__)
 /**
  * DRM_DEV_DEBUG_KMS() - Debug output for modesetting code
  *
@@ -399,7 +444,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_CLASS_KMS, fmt, ##__VA_ARGS__)
 
 /*
  * struct drm_device based logging
@@ -443,25 +488,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_CLASS_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_CLASS_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_CLASS_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_CLASS_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_CLASS_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_CLASS_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_CLASS_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_CLASS_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_CLASS_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_CLASS_DRMRES, fmt, ##__VA_ARGS__)
 
 
 /*
@@ -471,7 +516,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 +545,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_CLASS_CORE, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_DRIVER(fmt, ...)					\
-	__drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CLASS_DRIVER, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_KMS(fmt, ...)						\
-	__drm_dbg(DRM_UT_KMS, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CLASS_KMS, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_PRIME(fmt, ...)					\
-	__drm_dbg(DRM_UT_PRIME, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CLASS_PRIME, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_ATOMIC(fmt, ...)					\
-	__drm_dbg(DRM_UT_ATOMIC, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CLASS_ATOMIC, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_VBL(fmt, ...)						\
-	__drm_dbg(DRM_UT_VBL, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CLASS_VBL, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_LEASE(fmt, ...)					\
-	__drm_dbg(DRM_UT_LEASE, fmt, ##__VA_ARGS__)
+	__drm_dbg(DRM_DBG_CLASS_LEASE, fmt, ##__VA_ARGS__)
 
 #define DRM_DEBUG_DP(fmt, ...)						\
-	__drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
+	__drm_dbg(DRM_DBG_CLASS_DP, fmt, ## __VA_ARGS__)
 
 #define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)					\
 ({												\
-- 
2.31.1


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

* [PATCH v4 7/7] amdgpu: define a dydbg-bitmap to control categorized pr_debugs
  2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
                   ` (5 preceding siblings ...)
  2021-07-31 21:42 ` [PATCH v4 6/7] drm/print: add choice to use dynamic debug in drm-debug Jim Cromie
@ 2021-07-31 21:42 ` Jim Cromie
  6 siblings, 0 replies; 13+ messages in thread
From: Jim Cromie @ 2021-07-31 21:42 UTC (permalink / raw)
  To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Aurabindo Pillai, Wyatt Wood,
	Qingqing Zhuo, Jim Cromie, Johan Hovold, Jessica Yu, Joe Perches,
	Miguel Ojeda, Nick Desaulniers, dri-devel, linux-kernel, amd-gfx,
	intel-gvt-dev, intel-gfx

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

Use DYNDBG_BITMAP_DESC() to create a parameter/debug_dc, and to define
bits to control those pr_debugs by their category.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 .../gpu/drm/amd/display/dc/core/dc_debug.c    | 42 ++++++++++++++++++-
 1 file changed, 41 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..3041e0c3d726 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,48 @@
 
 #include "resource.h"
 
-#define DC_LOGGER_INIT(logger)
+/* define a drm.debug style dyndbg pr-debug control point */
+unsigned long __debug_dc;
+EXPORT_SYMBOL(__debug_dc);
+
+#define _help(key)	"\t\t" key " : help for " key "\n"
+#define cmd_help(key)	{ .prefix = key, .help = "help for " key }
+
+/* Id like to do these inside DEFINE_DYNDBG_BITMAP, later */
+#define MY_DYNDBG_PARM_DESC(name)					\
+	"Enable debug output via /sys/module/amdgpu/parameters/" #name	\
+	", where each bit enables 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]:")
+MODULE_PARM_DESC(debug_dc, MY_DYNDBG_PARM_DESC(name));
+
+DEFINE_DYNDBG_BITMAP(debug_dc, &__debug_dc,
+		     MY_DYNDBG_PARM_DESC(debug_dc),
+		     cmd_help("[CURSOR]:"),
+		     cmd_help("[PFLIP]:"),
+		     cmd_help("[VBLANK]:"),
+		     cmd_help("[HW_LINK_TRAINING]:"),
+		     cmd_help("[HW_AUDIO]:"),
+		     cmd_help("[SCALER]:"),
+		     cmd_help("[BIOS]:"),
+		     cmd_help("[BANDWIDTH_CALCS]:"),
+		     cmd_help("[DML]:"),
+		     cmd_help("[IF_TRACE]:"),
+		     cmd_help("[GAMMA]:"),
+		     cmd_help("[SMU_MSG]:"));
 
+#define DC_LOGGER_INIT(logger)
 
 #define SURFACE_TRACE(...) do {\
 		if (dc->debug.surface_trace) \
-- 
2.31.1


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

* Re: [PATCH v4 2/7] moduleparam: add data member to struct kernel_param
  2021-07-31 21:41 ` [PATCH v4 2/7] moduleparam: add data member to struct kernel_param Jim Cromie
@ 2021-08-02 16:18   ` Emil Velikov
  2021-08-02 18:36     ` jim.cromie
  0 siblings, 1 reply; 13+ messages in thread
From: Emil Velikov @ 2021-08-02 16:18 UTC (permalink / raw)
  To: Jim Cromie
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Aurabindo Pillai, Qingqing Zhuo,
	Wyatt Wood, Jessica Yu, Johan Hovold, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, ML dri-devel, Linux-Kernel@Vger. Kernel. Org,
	amd-gfx mailing list, intel-gvt-dev, Intel Graphics Development

Hi Jim,

On Sat, 31 Jul 2021 at 22:42, Jim Cromie <jim.cromie@gmail.com> wrote:

> Use of this new data member will be rare, it might be worth redoing
> this as a separate/sub-type to keep the base case.
>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  include/linux/moduleparam.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index eed280fae433..e9495b1e794d 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -78,6 +78,7 @@ struct kernel_param {
>                 const struct kparam_string *str;
>                 const struct kparam_array *arr;
>         };
> +       void *data;

Might as well make this "const void *" since it is a compile-time constant?

-Emil

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

* Re: [Intel-gfx] [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks
  2021-07-31 21:42 ` [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks Jim Cromie
@ 2021-08-02 16:24   ` Emil Velikov
  2021-08-02 18:40     ` jim.cromie
  0 siblings, 1 reply; 13+ messages in thread
From: Emil Velikov @ 2021-08-02 16:24 UTC (permalink / raw)
  To: Jim Cromie
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai,
	Wyatt Wood, Johan Hovold, Jessica Yu, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, ML dri-devel, Linux-Kernel@Vger. Kernel. Org,
	amd-gfx mailing list, intel-gvt-dev, Intel Graphics Development

Hi Jim,

On Sat, 31 Jul 2021 at 22:42, Jim Cromie <jim.cromie@gmail.com> wrote:

> +struct dyndbg_bitdesc {
> +       /* bitpos is inferred from index in containing array */
> +       char *prefix;
> +       char *help;
AFAICT these two should also be constant, right?


> +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> +{
> +       unsigned int val;
> +       unsigned long changes, result;
> +       int rc, chgct = 0, totct = 0, bitpos, bitsmax;
> +       char query[OUR_QUERY_SIZE];
> +       struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
> +
> +       // pr_info("set_dyndbg: instr: %s curr: %d\n", instr, *kp->arg);
Left-over debug code, here and below?

-Emil

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

* Re: [PATCH v4 5/7] i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt
  2021-07-31 21:42 ` [PATCH v4 5/7] i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt Jim Cromie
@ 2021-08-02 16:27   ` Emil Velikov
  0 siblings, 0 replies; 13+ messages in thread
From: Emil Velikov @ 2021-08-02 16:27 UTC (permalink / raw)
  To: Jim Cromie
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai,
	Wyatt Wood, Jessica Yu, Johan Hovold, Miguel Ojeda,
	Nick Desaulniers, Joe Perches, ML dri-devel,
	Linux-Kernel@Vger. Kernel. Org, amd-gfx mailing list,
	intel-gvt-dev, Intel Graphics Development

Hi Jim,

On Sat, 31 Jul 2021 at 22:42, Jim Cromie <jim.cromie@gmail.com> wrote:

> DYNDBG_BITMAP_DESC(__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" });
>
Previous commit removed the space after the colon. The above example
needs updating.

This concludes a casual read-through on my end. Hope it helps o/
-Emil

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

* Re: [PATCH v4 2/7] moduleparam: add data member to struct kernel_param
  2021-08-02 16:18   ` Emil Velikov
@ 2021-08-02 18:36     ` jim.cromie
  0 siblings, 0 replies; 13+ messages in thread
From: jim.cromie @ 2021-08-02 18:36 UTC (permalink / raw)
  To: Emil Velikov
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Aurabindo Pillai, Qingqing Zhuo,
	Wyatt Wood, Jessica Yu, Johan Hovold, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, ML dri-devel, Linux-Kernel@Vger. Kernel. Org,
	amd-gfx mailing list, intel-gvt-dev, Intel Graphics Development

On Mon, Aug 2, 2021 at 10:18 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Jim,
>
> On Sat, 31 Jul 2021 at 22:42, Jim Cromie <jim.cromie@gmail.com> wrote:
>
> > Use of this new data member will be rare, it might be worth redoing
> > this as a separate/sub-type to keep the base case.
> >
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> > ---
> >  include/linux/moduleparam.h | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> > index eed280fae433..e9495b1e794d 100644
> > --- a/include/linux/moduleparam.h
> > +++ b/include/linux/moduleparam.h
> > @@ -78,6 +78,7 @@ struct kernel_param {
> >                 const struct kparam_string *str;
> >                 const struct kparam_array *arr;
> >         };
> > +       void *data;
>
> Might as well make this "const void *" since it is a compile-time constant?
>

yes indeed. revising.  thanks

> -Emil

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

* Re: [Intel-gfx] [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks
  2021-08-02 16:24   ` [Intel-gfx] " Emil Velikov
@ 2021-08-02 18:40     ` jim.cromie
  0 siblings, 0 replies; 13+ messages in thread
From: jim.cromie @ 2021-08-02 18:40 UTC (permalink / raw)
  To: Emil Velikov
  Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Daniel Vetter, Harry Wentland, Leo Li,
	Alex Deucher, Christian König, Pan, Xinhui, Zhenyu Wang,
	Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Jason Baron, Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai,
	Wyatt Wood, Johan Hovold, Jessica Yu, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, ML dri-devel, Linux-Kernel@Vger. Kernel. Org,
	amd-gfx mailing list, intel-gvt-dev, Intel Graphics Development

On Mon, Aug 2, 2021 at 10:24 AM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> Hi Jim,
>
> On Sat, 31 Jul 2021 at 22:42, Jim Cromie <jim.cromie@gmail.com> wrote:
>
> > +struct dyndbg_bitdesc {
> > +       /* bitpos is inferred from index in containing array */
> > +       char *prefix;
> > +       char *help;
> AFAICT these two should also be constant, right?
>
>
> > +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> > +{
> > +       unsigned int val;
> > +       unsigned long changes, result;
> > +       int rc, chgct = 0, totct = 0, bitpos, bitsmax;
> > +       char query[OUR_QUERY_SIZE];
> > +       struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
> > +
> > +       // pr_info("set_dyndbg: instr: %s curr: %d\n", instr, *kp->arg);
> Left-over debug code, here and below?

yup, all fixed up locally, with a version that fully works.
thanks.

>
> -Emil

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

end of thread, other threads:[~2021-08-02 18:41 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-31 21:41 [PATCH v4 0/7] drm: use dyndbg in drm_print Jim Cromie
2021-07-31 21:41 ` [PATCH v4 1/7] drm/print: fixup spelling in a comment Jim Cromie
2021-07-31 21:41 ` [PATCH v4 2/7] moduleparam: add data member to struct kernel_param Jim Cromie
2021-08-02 16:18   ` Emil Velikov
2021-08-02 18:36     ` jim.cromie
2021-07-31 21:42 ` [PATCH v4 3/7] dyndbg: add dyndbg-bitmap definer and callbacks Jim Cromie
2021-08-02 16:24   ` [Intel-gfx] " Emil Velikov
2021-08-02 18:40     ` jim.cromie
2021-07-31 21:42 ` [PATCH v4 4/7] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
2021-07-31 21:42 ` [PATCH v4 5/7] i915/gvt: control pr_debug("gvt:")s with bits in parameters/debug_gvt Jim Cromie
2021-08-02 16:27   ` Emil Velikov
2021-07-31 21:42 ` [PATCH v4 6/7] drm/print: add choice to use dynamic debug in drm-debug Jim Cromie
2021-07-31 21:42 ` [PATCH v4 7/7] amdgpu: define a dydbg-bitmap to control categorized pr_debugs 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).