LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM
@ 2021-08-13 15:17 Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 1/9] drm/print: fixup spelling in a comment Jim Cromie
                   ` (8 more replies)
  0 siblings, 9 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Kevin Wang, Likun Gao, Chengming Gui, Aaron Liu, Jim Cromie,
	John Clements, Ashley Thomas, Wyatt Wood, Qingqing Zhuo,
	Aurabindo Pillai, Johan Hovold, Jessica Yu, Joe Perches,
	Nick Desaulniers, Miguel Ojeda, Andrew Morton, Masahiro Yamada,
	Peter Zijlstra, Andy Shevchenko, Paul E. McKenney, Tetsuo Handa,
	Thomas Gleixner, Vitor Massaru Iha, Sedat Dilek, Mel Gorman,
	Marco Elver, Alexander Potapenko, Brendan Higgins, Arvind Sankar,
	Matthew Wilcox (Oracle),
	Jiri Olsa, Patricia Alfonso, Palmer Dabbelt, Johannes Berg,
	Arnd Bergmann, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx

hi Jason, Greg, Daniel, dri-everyone,

drm_debug_enabled() is called a lot (by drm-debug api) 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 those
drm_debug_enabled() overheads, if CONFIG_DRM_USE_DYNAMIC_DEBUG=y.

v5: much rework

- based on Daniel Vetter's feedback, not RFC anymore. (except last one)

- move POC bit_map callback code into dynamic_debug
  add .data to struct kernel_param
  add DEFINE_DYNAMIC_DEBUG_CATEGORIES :
  a declarative interface for bits => control-queries
  this is all new functionality.

- use DEFINE_DYNAMIC_DEBUG_CATEGORIES in i915, amdgpu
  adds selectivity/control to existing categorizations

- DRM_USE_DYNAMIC_DEBUG
  replace DRM_UT_<CAT> (an enum)
  with DRM_CAT_<CAT>   (a prefix string, cpp-prepended to format)
  _UT_ still present, drm_debug_enabled() still used
  todo:
  change __drm_debug param-var to read DDD_CATEGORIES's param-var
  might suffice to keep parallel schemes coherent.
  
- RFC add tracer func as syslog alternate
  test_dynamic_debug.ko: uses tracer for observability, does selftest
  has some misuse risk; calling pr_debug recursively.

v4: (brown-bagger, various fixes after snips)
v3: fixes missed SOB, && on BOL, commit-log tweaks
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/

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 working on a diet-plan.

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

BOOT_IMAGE=(hd2,gpt2)/vmlinuz-5.14.0-rc4-d7a-00009-g5db471cba844 \
     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 90 seconds of tests, yielding
~76k prdbgs in 409 seconds, before I turned them off with:

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

Its worth noting, this changes the dyndbg-state underneath settings
applied with `echo > parameters/debug`; the latter is qualitatively
writeonly, maybe a param_get should return "NA" "-1"

this merged cleanly, on top of
commit d65ef4634e5c795a6a4df1d198992c70e9692fb3 (drm-tip/drm-tip)

Jim Cromie (9):
  drm/print: fixup spelling in a comment
  moduleparam: add data member to struct kernel_param
  dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  i915/gvt: 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 to control categorized
    pr_debugs
  drm_print: add choice to use dynamic debug in drm-debug
  amdgpu_ucode: reduce number of pr_debug calls
  dyndbg: RFC add tracer facility RFC

 drivers/gpu/drm/Kconfig                       |  13 +
 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                   |  49 ++-
 drivers/gpu/drm/i915/gvt/Makefile             |   4 +
 drivers/gpu/drm/i915/gvt/debug.h              |  18 +-
 drivers/gpu/drm/i915/i915_params.c            |  35 +++
 include/drm/drm_print.h                       | 143 +++++++--
 include/linux/dynamic_debug.h                 |  82 ++++-
 include/linux/moduleparam.h                   |  11 +-
 lib/Kconfig.debug                             |  10 +
 lib/Makefile                                  |   1 +
 lib/dynamic_debug.c                           | 171 ++++++++--
 lib/test_dynamic_debug.c                      | 247 +++++++++++++++
 14 files changed, 901 insertions(+), 220 deletions(-)
 create mode 100644 lib/test_dynamic_debug.c

-- 
2.31.1


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

* [PATCH v5 1/9] drm/print: fixup spelling in a comment
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 2/9] moduleparam: add data member to struct kernel_param Jim Cromie
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Likun Gao, Chengming Gui, John Clements, Jim Cromie, Kevin Wang,
	Ashley Thomas, Wyatt Wood, Qingqing Zhuo, Aurabindo Pillai,
	Jessica Yu, Johan Hovold, Joe Perches, Nick Desaulniers,
	Miguel Ojeda, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Andy Shevchenko, Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Changbin Du, Marco Elver,
	Brendan Higgins, Andrey Konovalov, Jarkko Sakkinen,
	Matthew Wilcox (Oracle),
	Johannes Berg, Patricia Alfonso, Arvind Sankar, Arnd Bergmann,
	Jiri Olsa, Palmer Dabbelt, linux-kernel, dri-devel, 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	[flat|nested] 16+ messages in thread

* [PATCH v5 2/9] moduleparam: add data member to struct kernel_param
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 1/9] drm/print: fixup spelling in a comment Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:43   ` Andy Shevchenko
  2021-08-13 15:17 ` [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Likun Gao, Chengming Gui, Aaron Liu, Jim Cromie, John Clements,
	Kevin Wang, Ashley Thomas, Qingqing Zhuo, Wyatt Wood,
	Aurabindo Pillai, Johan Hovold, Jessica Yu, Nick Desaulniers,
	Joe Perches, Miguel Ojeda, Andrew Morton, Masahiro Yamada,
	Peter Zijlstra, Andy Shevchenko, Paul E. McKenney, Tetsuo Handa,
	Thomas Gleixner, Vitor Massaru Iha, Sedat Dilek, Zhen Lei,
	Marco Elver, Jarkko Sakkinen, Alexander Potapenko,
	Palmer Dabbelt, Patricia Alfonso, Jiri Olsa, James Bottomley,
	Arvind Sankar, Johannes Berg, Arnd Bergmann, linux-kernel,
	dri-devel, amd-gfx, intel-gvt-dev, intel-gfx

Add a const 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,
and latter is redefined to use former.

It calls __module_param_call_wdata(), which accepts a new data param
and inits .data with it. Re-define __module_param_call() to use it.

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

---
v4+:
. const void* data - <emil.l.velikov@gmail.com>

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..878387e0b2d9 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;
 	};
+	const 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	[flat|nested] 16+ messages in thread

* [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 1/9] drm/print: fixup spelling in a comment Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 2/9] moduleparam: add data member to struct kernel_param Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:51   ` Andy Shevchenko
  2021-08-13 15:17 ` [PATCH v5 4/9] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Kevin Wang, Chengming Gui, Likun Gao, John Clements, Jim Cromie,
	Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai, Wyatt Wood,
	Johan Hovold, Jessica Yu, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Andy Shevchenko, Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Changbin Du, Marco Elver,
	Jarkko Sakkinen, Alexander Potapenko, Matthew Wilcox (Oracle),
	Zhen Lei, Albert van der Linde, Johannes Berg, Arvind Sankar,
	Patricia Alfonso, Arnd Bergmann, Palmer Dabbelt, Jiri Olsa,
	Andrey Konovalov, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx

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 3 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, All 3 are
non-static and exported.

dynamic_debug.h:

Add DEFINE_DYNAMIC_DEBUG_CATEGORIES() described above, and a do-nothing stub.

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.

This in the hope that someone can offer an auto-incrementing
label-generating macro, producing "\tbit-4 0x10\t" etc, and can show
how to apply it to __VA_ARGS__.

Also extern 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.

Adding structural query terms (func,file,lineno) could help (module is
already done), but DEFINE_DYNAMIC_DEBUG_CATEGORIES can't do that now,
adding it needs a better reason imo.

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

Here are some examples:

"1","2","3"		2 doesnt 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:"		might be handy
"A:".."Z:"		uhm, yeah

Hierarchical classes/categories are natural:

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

Some properties of a hierarchical category deserve explication:

Trailing spaces matter !

With 1..3-space ("drm: ", "drm:atomic: ", "drm:atomic:fail: "), the
":" doesnt terminate the search-space, the trailing space does.
So a "drm:" search specification will match all DRM categories &
subcategories, and will not be useful in an interface where all
categories are 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("fail: ...") renders 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 modified FOO().  Doing this would enforce a policy
decision that "debug categories will be space terminated", with an
pressure-relief valve.

Summarizing:

 - "drm:kms: " & "drm:kms:" are different
 - "drm:kms"		also different - includes drm:kms2:
 - "drm:kms:\t"		also different
 - "drm:kms:*"		doesnt 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 its 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.

There is still plenty of bikeshedding to do.

---
v4+:

. rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
. in query, 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>

ddh-helpers

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h | 50 ++++++++++++++++++++++++++++
 lib/dynamic_debug.c           | 62 ++++++++++++++++++++++++++++++++++-
 2 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index dce631e678dd..42cfb37d4870 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -52,6 +52,7 @@ struct _ddebug {
 } __attribute__((aligned(8)));
 
 
+struct kernel_param;
 
 #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
 
@@ -181,6 +182,9 @@ void __dynamic_ibdev_dbg(struct _ddebug *descriptor,
 				   KERN_DEBUG, prefix_str, prefix_type,	\
 				   rowsize, groupsize, buf, len, ascii)
 
+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,52 @@ static inline int dynamic_debug_exec_queries(const char *query, const char *modn
 	return 0;
 }
 
+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;
+};
+
+#if defined(CONFIG_DYNAMIC_DEBUG) || \
+	(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
+/**
+ * DEFINE_DYNAMIC_DEBUG_CATEGORIES() - define debug categories, bitmap, sysfs-knob
+ * @fsname: parameter basename under /sys
+ * @var:    C-identifier holding state
+ * @_desc:  string summarizing the controls provided
+ * @...:    list of struct dyndbg_bitdesc initializations
+ *
+ * Defines /sys/modules/$modname/parameters/@fsname, and @bit_descs,
+ * which maps bits 0-N to categories of pr_debugs to be controlled.
+ * This is effectively write only, because controlled callsites can be
+ * further modified via >control.
+ *
+ * Also calls MODULE_PARM_DESC(fsname, _desc), with the intent to
+ * generate the bit_legend and apply it to the given bit_descs
+ */
+#define DEFINE_DYNAMIC_DEBUG_CATEGORIES(fsname, var, _desc, ...)	\
+	MODULE_PARM_DESC(fsname, _desc);				\
+	static struct dyndbg_bitdesc const dyndbg_cats_##fsname[] =	\
+		{ __VA_ARGS__, { NULL, NULL } };			\
+	module_param_cbd(fsname, &param_ops_dyndbg, &var, 0644,		\
+			 &dyndbg_cats_##fsname);
+
+#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;
+#else
+#define DEFINE_DYNAMIC_DEBUG_CATEGORIES(fsname, var, bitmap_desc, ...) \
+	MODULE_PARM_DESC(fsname, "auto: " bitmap_desc);
+#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..6bc645a663e7 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\" %s\n", i, query, (modname) ? modname : "");
 
 		rc = ddebug_exec_query(query, modname);
 		if (rc < 0) {
@@ -1154,3 +1154,63 @@ 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 */
+
+/**
+ * param_set_dyndbg() - drm.debug style bitmap to format-prefix categories
+ * @instr: string echo>d to sysfs
+ * @kp:    struct kernel_param* ->data has bitmap
+ * Exported to support DEFINE_DYNAMIC_DEBUG_CATEGORIES
+ */
+int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
+{
+	unsigned long inbits;
+	int rc, i, chgct = 0, totct = 0;
+	char query[OUR_QUERY_SIZE];
+	struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
+
+	rc = kstrtoul(instr, 0, &inbits);
+	if (rc) {
+		pr_err("set_dyndbg: failed\n");
+		return -EINVAL;
+	}
+	vpr_info("set_dyndbg: input 0x%lx\n", inbits);
+
+	for (i = 0; !!bitmap[i].prefix; i++) {
+
+		sprintf(query, "format '^%s' %cp", bitmap[i].prefix,
+			test_bit(i, &inbits) ? '+' : '-');
+
+		chgct = dynamic_debug_exec_queries(query, kp->mod->name);
+
+		v2pr_info("bit-%d: %d changes by '%s'\n", i, chgct, query);
+		totct += chgct;
+	}
+	vpr_info("total changes: %d\n", totct);
+	return 0;
+}
+EXPORT_SYMBOL(param_set_dyndbg);
+
+/**
+ * param_get_dyndbg() - drm.debug style bitmap to format-prefix categories
+ * @buffer: string returned to user via sysfs
+ * @kp:     struct kernel_param*
+ * Exported to support DEFINE_DYNAMIC_DEBUG_CATEGORIES interfaces, not
+ * useful; pr_debugs may be altered after setting via >control
+ */
+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,
+};
+/* support DEFINE_DYNAMIC_DEBUG_CATEGORIES users */
+EXPORT_SYMBOL(param_ops_dyndbg);
-- 
2.31.1


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

* [PATCH v5 4/9] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
                   ` (2 preceding siblings ...)
  2021-08-13 15:17 ` [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 5/9] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Chengming Gui, Likun Gao, Dennis Li, Jim Cromie, John Clements,
	Kevin Wang, Ashley Thomas, Qingqing Zhuo, Wyatt Wood,
	Aurabindo Pillai, Johan Hovold, Jessica Yu, Miguel Ojeda,
	Joe Perches, Nick Desaulniers, Andrew Morton, Masahiro Yamada,
	Peter Zijlstra, Andy Shevchenko, Paul E. McKenney, Tetsuo Handa,
	Thomas Gleixner, Vitor Massaru Iha, Sedat Dilek, Julian Braha,
	Marco Elver, Jarkko Sakkinen, Andrey Konovalov,
	Alexander Potapenko, Johannes Berg, Richard Fitzgerald,
	Arvind Sankar, Patricia Alfonso, Palmer Dabbelt, Arnd Bergmann,
	Jiri Olsa, linux-kernel, dri-devel, amd-gfx, intel-gvt-dev,
	intel-gfx

Taking embedded spaces out of existing prefixes makes them better
class-prefixes; simplifying the nested 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:".

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] 16+ messages in thread

* [PATCH v5 5/9] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
                   ` (3 preceding siblings ...)
  2021-08-13 15:17 ` [PATCH v5 4/9] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 6/9] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to control categorized pr_debugs Jim Cromie
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Le Ma, Chengming Gui, Likun Gao, Dennis Li, Jim Cromie,
	John Clements, Kevin Wang, Ashley Thomas, Aurabindo Pillai,
	Qingqing Zhuo, Wyatt Wood, Johan Hovold, Jessica Yu,
	Miguel Ojeda, Joe Perches, Nick Desaulniers, Andrew Morton,
	Masahiro Yamada, Peter Zijlstra, Andy Shevchenko,
	Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Changbin Du, Marco Elver,
	Andrey Konovalov, Alexander Potapenko, Brendan Higgins,
	Oliver Glitta, James Bottomley, Arnd Bergmann, Patricia Alfonso,
	Arvind Sankar, Jiri Olsa, Palmer Dabbelt, linux-kernel,
	dri-devel, amd-gfx, intel-gvt-dev, intel-gfx

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.

---
v4+:
. static decl of vector of bit->class descriptors - Emil.V
. relocate gvt-makefile chunk from elsewhere

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/i915/gvt/Makefile  |  4 ++++
 drivers/gpu/drm/i915/i915_params.c | 35 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

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..683e942a074e 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 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] 16+ messages in thread

* [PATCH v5 6/9] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to control categorized pr_debugs
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
                   ` (4 preceding siblings ...)
  2021-08-13 15:17 ` [PATCH v5 5/9] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 7/9] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Kevin Wang, Chengming Gui, Likun Gao, Jim Cromie, John Clements,
	Ashley Thomas, Aurabindo Pillai, Qingqing Zhuo, Wyatt Wood,
	Johan Hovold, Jessica Yu, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Andy Shevchenko, Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Changbin Du, Marco Elver,
	Brendan Higgins, Andrey Konovalov, Albert van der Linde,
	Zhen Lei, Arvind Sankar, Patricia Alfonso, Arnd Bergmann,
	Jiri Olsa, Johannes Berg, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx

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..69e68d721512 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 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] 16+ messages in thread

* [PATCH v5 7/9] drm_print: add choice to use dynamic debug in drm-debug
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
                   ` (5 preceding siblings ...)
  2021-08-13 15:17 ` [PATCH v5 6/9] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to control categorized pr_debugs Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 8/9] amdgpu_ucode: reduce number of pr_debug calls Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 9/9] dyndbg: RFC add tracer facility RFC Jim Cromie
  8 siblings, 0 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Kevin Wang, Likun Gao, Chengming Gui, Jim Cromie, John Clements,
	Ashley Thomas, Wyatt Wood, Aurabindo Pillai, Qingqing Zhuo,
	Jessica Yu, Johan Hovold, Miguel Ojeda, Joe Perches,
	Nick Desaulniers, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Andy Shevchenko, Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Changbin Du, Marco Elver,
	Andrey Konovalov, Brendan Higgins, Jarkko Sakkinen,
	James Bottomley, Palmer Dabbelt, Albert van der Linde,
	Arvind Sankar, Patricia Alfonso, Jiri Olsa, Arnd Bergmann,
	linux-kernel, dri-devel, 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.

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.

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 the whole category 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 "basic" -> "dyndbg" switchover is layered into the macro scheme

A. use DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
		"DRM debug category-per-bit control",
   	  	{ "drm:core:", "enable CORE debug messages" },
   	  	{ "drm:kms:", "enable KMS debug messages" }, ...);

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

   DRM_DBG_CLASS_<CATs> was proposed, I had agreed, but reconsidered;
   CATEGORY is already DRM's term-of-art, and adding a near-synonym
   'CLASS' only adds ambiguity.

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

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

C. 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

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

D. API[1] uses DRM_DBG_CAT_<CAT>s
   these already use (C), now they use (B) too,
   to get the correct token type for "basic" and "dyndbg" configs.

NOTES:

Code Review is expected to catch 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.

	pr_debug("Entry: ...")		// +fml gives useful log-info
	pr_debug("Exit: ...")		// hard to catch them all

But "func foo" added to query-command would work, should it be useful
enough to justify extending the declarative interface.

---
v4+:

. 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
. add kernel doc

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/Kconfig     |  13 ++++
 drivers/gpu/drm/drm_print.c |  49 +++++++++----
 include/drm/drm_print.h     | 141 ++++++++++++++++++++++++++++--------
 3 files changed, 159 insertions(+), 44 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/drm_print.c b/drivers/gpu/drm/drm_print.c
index 111b932cf2a9..72b940a30779 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>
@@ -43,16 +44,36 @@
 unsigned int __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)");
+#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, int, 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 15a089a87c22..aca56b456277 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -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,85 @@ 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 */
+
+#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 "
+
+#endif /* !CONFIG_DRM_USE_DYNAMIC_DEBUG */
+
 static inline bool drm_debug_enabled(enum drm_debug_category category)
 {
 	return unlikely(__drm_debug & category);
@@ -334,8 +413,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 +462,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 +470,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 +478,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 +522,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 +550,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 +579,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 +610,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] 16+ messages in thread

* [PATCH v5 8/9] amdgpu_ucode: reduce number of pr_debug calls
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
                   ` (6 preceding siblings ...)
  2021-08-13 15:17 ` [PATCH v5 7/9] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  2021-08-13 15:17 ` [PATCH v5 9/9] dyndbg: RFC add tracer facility RFC Jim Cromie
  8 siblings, 0 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Le Ma, Likun Gao, Chengming Gui, Jim Cromie, John Clements,
	Kevin Wang, Ashley Thomas, Aurabindo Pillai, Wyatt Wood,
	Qingqing Zhuo, Johan Hovold, Jessica Yu, Miguel Ojeda,
	Joe Perches, Nick Desaulniers, Andrew Morton, Masahiro Yamada,
	Peter Zijlstra, Andy Shevchenko, Paul E. McKenney, Tetsuo Handa,
	Thomas Gleixner, Vitor Massaru Iha, Sedat Dilek, Changbin Du,
	Marco Elver, Andrey Konovalov, Jarkko Sakkinen, Brendan Higgins,
	Arvind Sankar, Patricia Alfonso, Jiri Olsa, Palmer Dabbelt,
	Johannes Berg, Arnd Bergmann, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx

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

no functional changes.

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] 16+ messages in thread

* [PATCH v5 9/9] dyndbg: RFC add tracer facility RFC
  2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
                   ` (7 preceding siblings ...)
  2021-08-13 15:17 ` [PATCH v5 8/9] amdgpu_ucode: reduce number of pr_debug calls Jim Cromie
@ 2021-08-13 15:17 ` Jim Cromie
  8 siblings, 0 replies; 16+ messages in thread
From: Jim Cromie @ 2021-08-13 15:17 UTC (permalink / raw)
  To: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Likun Gao, Chengming Gui, Luben Tuikov, Jim Cromie,
	John Clements, Kevin Wang, Ashley Thomas, Aurabindo Pillai,
	Qingqing Zhuo, Wyatt Wood, Jessica Yu, Johan Hovold,
	Nick Desaulniers, Joe Perches, Miguel Ojeda, Andrew Morton,
	Masahiro Yamada, Peter Zijlstra, Andy Shevchenko,
	Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Julian Braha, Marco Elver,
	Alexander Potapenko, Andrey Konovalov, Oliver Glitta,
	Richard Fitzgerald, Albert van der Linde, Arvind Sankar,
	Patricia Alfonso, Arnd Bergmann, Johannes Berg, Jiri Olsa,
	linux-kernel, dri-devel, amd-gfx, intel-gvt-dev, intel-gfx

Sean Paul seanpaul@chromium.org proposed, in
https://patchwork.freedesktop.org/series/78133/
drm/trace: Mirror DRM debug logs to tracefs

That patchset's goal is to be able to duplicate the debug stream to a
tracing destination, by splitting drm_debug_enabled() into syslog &
trace flavors, and enabling them separately.  That clashes rather
deeply with this patchset's goal; to avoid drm_debug_enabled() using
dyndbg.

Instead, this puts the print-to-trace decision in dyndbg, after the
is-it-enabled test (which is a noop), so it has near zero additional
cost (other than memory increase); the print-to-trace test is only
done on enabled callsites.

The basic elements:

 - add a new struct _ddebug member: (*tracer)(char *format, ...)
 - add a new T flag to enable tracer
 - adjust the static-key-enable/disable condition for (p|T)
 - if (p) before printk, since T enables too.
 - if (T) call tracer if its true

 = int dynamic_debug_register_tracer(query, modname, tracer);
 = int dynamic_debug_unregister_tracer(query, modname, cookie);

This new interface lets clients set/unset a tracer function on each
callsite matching the query, for example: "drm:atomic:fail:".

Clients are expected to unregister the same callsites they register (a
cookie), allowing protection of each client's dyndbg-state setup
against overwrites by others.

Intended Behavior: (things are in flux, RFC)

- register sets empty slot, doesnt overwrite
  the query selects callsites, and sets +T (grammar requires +-action)

- register allows same-tracer over-set wo warn
  2nd register can then enable superset, subset, disjoint set

- unregister clears slot if it matches cookie/tracer
  query selects set, -T (as tested)
  tolerates over-clears

- dd-exec-queries(+/-T) can modify the enablements
  not sure its needed, but it falls out..

The code is currently in-line in ddebug_change(), to be moved to
separate fn, rc determines flow, may also veto/alter changes by
altering flag-settings - tbd.

TBD: Im not sure what happens when exec-queries(+T) hits a site wo a
tracer (silence I think. maybe not ideal).

internal call-chain gets a tracer param:
					New arg:
public: dynamic_debug_exec_queries
	ro-string copy moved ...
1	ddebug_exec_queries		tracer=NULL
	... to here
2	ddebug_exec_query		tracer=NULL

call-chain gets (re)used: with !NULL

public: dynamic_debug_register_tracer	tracer=client's
	w ro-string
1	ddebug_exec_queries		tracer
...

SELFTEST: test_dynamic_debug.ko:

Uses the tracer facility to do a selftest:

- A custom tracer counts the number of calls (of T-enabled pr_debugs),
- do_debugging(x) calls a set of categorized pr_debugs x times

- test registers the tracer on the function,
  then iteratively:
  manipulates dyndbg states via query-cmds
  runs do_debugging()
  counts enabled callsite executions
  reports mismatches

- modprobe test_dynamic_debug use_bad_tracer=1
  attaches a bad/recursive tracer
  Bad Things Happen.
  has thrown me interesting panics.

NOTES:

This needs more work. RFC.

ERRORS (or WARNINGS):

It should be an error to +T a callsite which has no aux_print set (ie
already registered with a query that selected that callsite).  This
tacitly enforces registration.

Then +T,-T can toggle those aux_print callsites (or subsets of them)
to tailor the debug-stream for the purpose.  Controlling flow is the
best work limiter.

---
v4+: (this patch sent after (on top of) v4)

. fix "too many arguments to function", and name the args:
  int (*aux_print)(const char *fmt, char *prefix, char *label, void *);
   prefix : is a slot for dynamic_emit_prefix, or for custom buffer insert
   label  : for builtin-caller used by drm-trace-print
   void*  : vaf, add type constraint later.

. fix printk (to syslog) needs if (+p), since +T also enables
. add prototypes for un/register_aux_print
. change iface names: s/aux_print/tracer/
. also s/trace_print/tracer/
. struct va_format *vaf - tighten further ?

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 include/linux/dynamic_debug.h |  32 ++++-
 lib/Kconfig.debug             |  10 ++
 lib/Makefile                  |   1 +
 lib/dynamic_debug.c           | 109 +++++++++++----
 lib/test_dynamic_debug.c      | 247 ++++++++++++++++++++++++++++++++++
 5 files changed, 372 insertions(+), 27 deletions(-)
 create mode 100644 lib/test_dynamic_debug.c

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 42cfb37d4870..cbcb1c94cec3 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -20,6 +20,7 @@ struct _ddebug {
 	const char *function;
 	const char *filename;
 	const char *format;
+	int (*tracer)(const char *fmt, char *prefix, char *label, struct va_format *vaf);
 	unsigned int lineno:18;
 	/*
 	 * The flags field controls the behaviour at the callsite.
@@ -27,7 +28,11 @@ struct _ddebug {
 	 * writes commands to <debugfs>/dynamic_debug/control
 	 */
 #define _DPRINTK_FLAGS_NONE	0
-#define _DPRINTK_FLAGS_PRINT	(1<<0) /* printk() a message using the format */
+#define _DPRINTK_FLAGS_PRINT		(1<<0) /* printk() a message */
+#define _DPRINTK_FLAGS_PRINT_TRACE	(1<<5) /* call (*tracer) */
+
+#define _DPRINTK_ENABLED (_DPRINTK_FLAGS_PRINT | _DPRINTK_FLAGS_PRINT_TRACE)
+
 #define _DPRINTK_FLAGS_INCL_MODNAME	(1<<1)
 #define _DPRINTK_FLAGS_INCL_FUNCNAME	(1<<2)
 #define _DPRINTK_FLAGS_INCL_LINENO	(1<<3)
@@ -279,4 +284,29 @@ extern const struct kernel_param_ops param_ops_dyndbg;
 #define _DD_cat_help_(pfx)
 #endif
 
+/**
+ * dynamic_debug_register_tracer - register a "printer" function
+ * @query: query-command string to select callsites getting the function
+ * @tracer: &vprintf-ish accepting 3 char* ptrs & a vaf
+ *
+ * Attach a printer function to callsites selected by query.
+ * If another printer is already attached, warn and skip, applying the
+ * rest of the query.  This protects existing setups, while allowing
+ * maximal coexistence of (mostly) non-competing listeners. RFC.
+ */
+int dynamic_debug_register_tracer(const char *query, const char *mod,
+	int (*tracer) (const char *fmt, char *prefix, char *label, struct va_format *vaf));
+/**
+ * dynamic_debug_unregister_tracer - unregister your "printer" function
+ * @query: query-command string to select callsites to reset
+ * @cookie: reserved to validate unregisters against pirates
+ *
+ * Detach this printer function (@cookie) from callsites selected by
+ * the query, when @cookie == callsite.  This protects existing
+ * setups, while allowing maximal coexistence of (mostly)
+ * non-competing listeners. RFC.
+ */
+int dynamic_debug_unregister_tracer(const char *query, const char *mod,
+	int (*cookie) (const char *fmt, char *prefix, char *label, struct va_format *vaf));
+
 #endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ddd575159fb..5038f81d3fc6 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2486,6 +2486,16 @@ config TEST_STATIC_KEYS
 
 	  If unsure, say N.
 
+config TEST_DYNAMIC_DEBUG
+	tristate "Test DYNAMIC_DEBUG"
+	depends on m
+	depends on DYNAMIC_DEBUG
+	help
+	  Test the print_aux registration with a counting fn,
+	  then test expected counts during the execution.
+
+	  If unsure, say N.
+
 config TEST_KMOD
 	tristate "kmod stress tester"
 	depends on m
diff --git a/lib/Makefile b/lib/Makefile
index 5efd1b435a37..01c3c76980ba 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_TEST_SORT) += test_sort.o
 obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
 obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
+obj-$(CONFIG_TEST_DYNAMIC_DEBUG) += test_dynamic_debug.o
 obj-$(CONFIG_TEST_PRINTF) += test_printf.o
 obj-$(CONFIG_TEST_SCANF) += test_scanf.o
 obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 6bc645a663e7..9ba180976636 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -85,6 +85,7 @@ static inline const char *trim_prefix(const char *path)
 
 static struct { unsigned flag:8; char opt_char; } opt_array[] = {
 	{ _DPRINTK_FLAGS_PRINT, 'p' },
+	{ _DPRINTK_FLAGS_PRINT_TRACE, 'T' },
 	{ _DPRINTK_FLAGS_INCL_MODNAME, 'm' },
 	{ _DPRINTK_FLAGS_INCL_FUNCNAME, 'f' },
 	{ _DPRINTK_FLAGS_INCL_LINENO, 'l' },
@@ -146,7 +147,8 @@ static void vpr_info_dq(const struct ddebug_query *query, const char *msg)
  * logs the changes.  Takes ddebug_lock.
  */
 static int ddebug_change(const struct ddebug_query *query,
-			 struct flag_settings *modifiers)
+			 struct flag_settings *modifiers,
+			 int (*tracer)(const char *, char *, char *, struct va_format *))
 {
 	int i;
 	struct ddebug_table *dt;
@@ -205,11 +207,43 @@ static int ddebug_change(const struct ddebug_query *query,
 			newflags = (dp->flags & modifiers->mask) | modifiers->flags;
 			if (newflags == dp->flags)
 				continue;
+
+			/* handle T flag */
+			if (newflags & _DPRINTK_FLAGS_PRINT_TRACE) {
+				if (!tracer)
+					v2pr_info("tracer enable\n");
+				else {
+					/* register attempt */
+					if (!dp->tracer) {
+						v2pr_info("register tracer\n");
+						dp->tracer = tracer;
+					}
+					else if (tracer == dp->tracer)
+						v2pr_info("tracer equal, ok\n");
+					else
+						pr_warn("tracer register error\n");
+				}
+			} else if (dp->flags & _DPRINTK_FLAGS_PRINT_TRACE) {
+				if (!tracer)
+					v2pr_info("ok: disabling\n");
+				else {
+					/* only unregister has a !!tracer */
+					if (!dp->tracer)
+						pr_warn("nok: tracer already unset\n");
+
+					else if (dp->tracer == tracer) {
+						v2pr_info("ok: cookie match, unregistering\n");
+						dp->tracer = NULL;
+					}
+					else
+						pr_warn("nok: tracer cookie match fail\n");
+				}
+			}
 #ifdef CONFIG_JUMP_LABEL
-			if (dp->flags & _DPRINTK_FLAGS_PRINT) {
-				if (!(modifiers->flags & _DPRINTK_FLAGS_PRINT))
+			if (dp->flags & _DPRINTK_ENABLED) {
+				if (!(modifiers->flags & _DPRINTK_ENABLED))
 					static_branch_disable(&dp->key.dd_key_true);
-			} else if (modifiers->flags & _DPRINTK_FLAGS_PRINT)
+			} else if (modifiers->flags & _DPRINTK_ENABLED)
 				static_branch_enable(&dp->key.dd_key_true);
 #endif
 			dp->flags = newflags;
@@ -482,7 +516,7 @@ static int ddebug_parse_flags(const char *str, struct flag_settings *modifiers)
 	return 0;
 }
 
-static int ddebug_exec_query(char *query_string, const char *modname)
+static int ddebug_exec_query(char *query_string, const char *modname, void *tracer)
 {
 	struct flag_settings modifiers = {};
 	struct ddebug_query query = {};
@@ -505,7 +539,7 @@ static int ddebug_exec_query(char *query_string, const char *modname)
 		return -EINVAL;
 	}
 	/* actually go and implement the change */
-	nfound = ddebug_change(&query, &modifiers);
+	nfound = ddebug_change(&query, &modifiers, tracer);
 	vpr_info_dq(&query, nfound ? "applied" : "no-match");
 
 	return nfound;
@@ -515,10 +549,19 @@ static int ddebug_exec_query(char *query_string, const char *modname)
    last error or number of matching callsites.  Module name is either
    in param (for boot arg) or perhaps in query string.
 */
-static int ddebug_exec_queries(char *query, const char *modname)
+static int ddebug_exec_queries(const char *query_in, const char *modname, void *tracer)
 {
 	char *split;
 	int i, errs = 0, exitcode = 0, rc, nfound = 0;
+	char *query; /* writable copy of query_in */
+
+	if (!query_in) {
+		pr_err("non-null query/command string expected\n");
+		return -EINVAL;
+	}
+	query = kstrndup(query_in, PAGE_SIZE, GFP_KERNEL);
+	if (!query)
+		return -ENOMEM;
 
 	for (i = 0; query; query = split) {
 		split = strpbrk(query, ";\n");
@@ -531,7 +574,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
 
 		vpr_info("query %d: \"%s\" %s\n", i, query, (modname) ? modname : "");
 
-		rc = ddebug_exec_query(query, modname);
+		rc = ddebug_exec_query(query, modname, tracer);
 		if (rc < 0) {
 			errs++;
 			exitcode = rc;
@@ -540,6 +583,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
 		}
 		i++;
 	}
+	kfree(query);
 	vpr_info("processed %d queries, with %d matches, %d errs\n",
 		 i, nfound, errs);
 
@@ -560,20 +604,7 @@ static int ddebug_exec_queries(char *query, const char *modname)
  */
 int dynamic_debug_exec_queries(const char *query, const char *modname)
 {
-	int rc;
-	char *qry; /* writable copy of query */
-
-	if (!query) {
-		pr_err("non-null query/command string expected\n");
-		return -EINVAL;
-	}
-	qry = kstrndup(query, PAGE_SIZE, GFP_KERNEL);
-	if (!qry)
-		return -ENOMEM;
-
-	rc = ddebug_exec_queries(qry, modname);
-	kfree(qry);
-	return rc;
+	return ddebug_exec_queries(query, modname, NULL);
 }
 EXPORT_SYMBOL_GPL(dynamic_debug_exec_queries);
 
@@ -637,8 +668,20 @@ void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...)
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
-	printk(KERN_DEBUG "%s%pV", dynamic_emit_prefix(descriptor, buf), &vaf);
+	if (descriptor->flags & _DPRINTK_ENABLED)
+		dynamic_emit_prefix(descriptor, buf);
+
+	if (descriptor->flags & _DPRINTK_FLAGS_PRINT)
+		printk(KERN_DEBUG "%s%pV", buf, &vaf);
 
+	if (descriptor->flags & _DPRINTK_FLAGS_PRINT_TRACE) {
+
+		if (descriptor->tracer) {
+			(*descriptor->tracer)("%s:%ps %pV", buf,
+						 __builtin_return_address(0), &vaf);
+		}
+		/* else shouldnt matter, but maybe for consistency */
+	}
 	va_end(args);
 }
 EXPORT_SYMBOL(__dynamic_pr_debug);
@@ -783,7 +826,7 @@ static ssize_t ddebug_proc_write(struct file *file, const char __user *ubuf,
 		return PTR_ERR(tmpbuf);
 	vpr_info("read %d bytes from userspace\n", (int)len);
 
-	ret = ddebug_exec_queries(tmpbuf, NULL);
+	ret = ddebug_exec_queries(tmpbuf, NULL, NULL);
 	kfree(tmpbuf);
 	if (ret < 0)
 		return ret;
@@ -989,7 +1032,7 @@ static int ddebug_dyndbg_param_cb(char *param, char *val,
 	if (strcmp(param, "dyndbg"))
 		return on_err; /* determined by caller */
 
-	ddebug_exec_queries((val ? val : "+p"), modname);
+	ddebug_exec_queries((val ? val : "+p"), modname, NULL);
 
 	return 0; /* query failure shouldn't stop module load */
 }
@@ -1124,7 +1167,7 @@ static int __init dynamic_debug_init(void)
 	/* apply ddebug_query boot param, dont unload tables on err */
 	if (ddebug_setup_string[0] != '\0') {
 		pr_warn("ddebug_query param name is deprecated, change it to dyndbg\n");
-		ret = ddebug_exec_queries(ddebug_setup_string, NULL);
+		ret = ddebug_exec_queries(ddebug_setup_string, NULL, NULL);
 		if (ret < 0)
 			pr_warn("Invalid ddebug boot param %s\n",
 				ddebug_setup_string);
@@ -1214,3 +1257,17 @@ const struct kernel_param_ops param_ops_dyndbg = {
 };
 /* support DEFINE_DYNAMIC_DEBUG_CATEGORIES users */
 EXPORT_SYMBOL(param_ops_dyndbg);
+
+int dynamic_debug_register_tracer(const char *query, const char *mod,
+	int (*tracer) (const char *fmt, char *prefix, char *label, struct va_format *vaf))
+{
+	return ddebug_exec_queries(query, mod, tracer);
+}
+EXPORT_SYMBOL(dynamic_debug_register_tracer);
+
+int dynamic_debug_unregister_tracer(const char *query, const char *mod,
+	int (*tracer) (const char *fmt, char *prefix, char *label, struct va_format *vaf))
+{
+	return ddebug_exec_queries(query, mod, tracer);
+}
+EXPORT_SYMBOL(dynamic_debug_unregister_tracer);
diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
new file mode 100644
index 000000000000..e837398bd885
--- /dev/null
+++ b/lib/test_dynamic_debug.c
@@ -0,0 +1,247 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Kernel module for testing dynamic_debug
+ *
+ * Authors:
+ *      Jim Cromie	<jim.cromie@gmail.com>
+ */
+
+/*
+ * test-setup: use trace_print attachment interface as a test harness,
+ * define a custom trace_printer which counts invocations, and a
+ * pr_debug event generator function which calls a set of categorized
+ * pr_debugs.
+ * test-run: manipulate the pr_debug's enablement, run the event
+ * generator, and check for the expected side effects.
+ */
+
+#include <linux/module.h>
+
+static int __bad_tracer;
+
+static int trace_ct = 0;
+static int test_ct = 0;
+static int errors = 0;
+static int verbose = 0;
+
+
+module_param_named(use_bad_tracer, __bad_tracer, int, 0644);
+MODULE_PARM_DESC(use_bad_tracer,
+		 "use broken tracer, recursing with pr_debug\n"
+		 "\tonly works at modprobe time\n");
+
+static int (*my_tracer)(const char *decorator, char *prefix, char *label, struct va_format *vaf);
+
+static int good_tracer(const char *decorator, char *prefix, char *label, struct va_format *vaf)
+{
+	trace_ct++;
+	if (verbose)
+		pr_notice("my_tracer: %pV", vaf);
+	return 0;
+}
+
+static int bad_tracer(const char *decorator, char *prefix, char *label, struct va_format *vaf)
+{
+	/* dont try pr_debug, it recurses back here */
+	pr_debug("oops! recursion, crash?\n");
+	return 0;
+}
+
+static void pick_tracer(void)
+{
+	if (__bad_tracer)
+		my_tracer = bad_tracer;
+	else
+		my_tracer = good_tracer;
+}
+
+static int expect_count(int want, const char *story)
+{
+	test_ct++;
+	if (want != trace_ct) {
+		pr_err("expect_count: want %d, got %d: %s\n", want, trace_ct, story);
+		errors++;
+		trace_ct = 0;
+		return 1;
+	}
+	pr_info("pass %d, hits %d, on \"%s\"\n", test_ct, want, story);
+	trace_ct = 0;
+	return 0;
+}
+
+/* call pr_debug (4 * reps) + 2 times, for tracer side-effects */
+static void do_debugging(int reps)
+{
+	int i;
+
+	pr_debug("Entry:\n");
+	pr_info(" do_debugging %d time(s)\n", reps);
+	for (i = 0; i < reps; i++) {
+		pr_debug("hi: %d\n", i);
+		pr_debug("mid: %d\n", i);
+		pr_debug("low: %d\n", i);
+		pr_debug("low:lower: %d subcategory test\n", i);
+	}
+	pr_debug("Exit:\n");
+}
+
+static void expect_matches(int want, int got, const char *story)
+{
+	// todo: got <0 are errors, bubbled up
+	if (got != want)
+		pr_warn(" match_count wrong: want %d got %d on \"%s\"\n", want, got, story);
+	else
+		pr_info(" ok: %d matches by \"%s\"\n", want, story);
+
+	// ? count errs ? separately ?
+}
+
+static int report(char *who)
+{
+	if (errors)
+		pr_err("%s failed %d of %d tests\n", who, errors, test_ct);
+	else
+		pr_info("%s passed %d tests\n", who, test_ct);
+	return errors;
+}
+
+struct exec_test {
+	int matches;
+	int loops;
+	int hits;
+	const char *mod;
+	const char *qry;
+};
+
+static void do_exec_test(struct exec_test *tst)
+{
+	int match_count;
+
+	match_count = dynamic_debug_exec_queries(tst->qry, tst->mod);
+	expect_matches(tst->matches, match_count, tst->qry);
+	do_debugging(tst->loops);
+	expect_count(tst->hits, tst->qry);
+}
+
+static const char my_mod[] = "test_dynamic_debug";
+
+/* these tests rely on register stuff having been done ?? */
+struct exec_test exec_tests[] = {
+
+	/* standard use is my_mod, for `modprobe $module dyndbg=+p` */
+
+	/* no modification probe */
+	{ 6, 2, 0, my_mod, "func do_debugging +_" },
+
+	/* use original single string query style */
+	{ 6, 3, 0, NULL, "module test_dynamic_debug func do_debugging -T" },
+
+	/* this is mildly preferred */
+	{ 6, 3, 0, my_mod, "func do_debugging -T" },
+
+	/* enable all DUT */
+	{ 6, 4, 18, my_mod, "func do_debugging +T" },
+
+	/* disable 1 call */
+	{ 1, 4, 14, my_mod, "format '^hi:' -T" },
+
+	/* disable 1 call */
+	{ 1, 4, 10, my_mod, "format '^mid:' -T" },
+
+	/* repeat same disable */
+	{ 1, 4, 10, my_mod, "format '^mid:' -T" },
+
+	/* repeat same disable, diff run ct */
+	{ 1, 5, 12, my_mod, "format '^mid:' -T" },
+
+	/* include subclass */
+	{ 2, 4, 2, my_mod, "format '^low:' -T" },
+
+	/* re-disable, exclude subclass */
+	{ 1, 4, 2, my_mod, "format '^low: ' -T" },
+
+	/* enable, exclude subclass */
+	{ 1, 4, 6, my_mod, "format '^low: ' +T" },
+
+	/* enable the subclass */
+	{ 1, 4, 10, my_mod, "format '^low:lower:' +T" },
+
+	/* enable the subclass */
+	{ 1, 6, 14, my_mod, "format '^low:lower:' +T" },
+};
+
+struct register_test {
+	int matches;
+	int loops;
+	int hits;
+	const char *mod;
+	const char *qry;
+};
+
+static void do_register_test(struct register_test *tst)
+{
+	int match_count;
+
+	match_count = dynamic_debug_register_tracer(tst->qry, tst->mod, my_tracer);
+	expect_matches(tst->matches, match_count, tst->qry);
+	do_debugging(tst->loops);
+	expect_count(tst->hits, tst->qry);
+}
+
+struct register_test register_tests[] = {
+
+	{ 6, 3, 14, my_mod, "func do_debugging +T" },
+
+	{ 10, 3, 0, my_mod, "+_" }, //, my_tracer },
+	{ 11, 3, 0, my_mod, "+T" }, //, my_tracer },
+};
+
+static int __init test_dynamic_debug_init(void)
+{
+	int match_count; /* rc from ddebug_exec_queries. meh. */
+	int i;
+
+	pick_tracer();
+
+	pr_debug("Entry:\n");
+	do_debugging(3);
+	expect_count(0, "nothing unless dyndbg=+T at modprobe");
+
+	for (i = 0; i < ARRAY_SIZE(register_tests); i++)
+		do_register_test(&register_tests[i]);
+
+	do_debugging(2);
+	expect_count(10, "do_debugging 2 times after +T");
+
+	for (i = 0; i < ARRAY_SIZE(exec_tests); i++)
+		do_exec_test(&exec_tests[i]);
+
+	match_count = dynamic_debug_unregister_tracer(
+		"func do_debugging -T", "test_dynamic_debug", my_tracer);
+
+	expect_matches(6, match_count,
+		       "unregister do_debugging()s tracers");
+	do_debugging(4);
+	expect_count(0, "everything is off");
+
+	match_count = dynamic_debug_unregister_tracer(
+		"func do_debugging -T", "test_dynamic_debug", my_tracer);
+
+	expect_matches(6, match_count,
+		       "re-unregister, same count, not a change count");
+	report("init");
+	pr_debug("Exit:\n");
+	return 0;
+}
+
+static void __exit test_dynamic_debug_exit(void)
+{
+	report("exit");
+	pr_debug("Exit:");
+}
+
+module_init(test_dynamic_debug_init);
+module_exit(test_dynamic_debug_exit);
+
+MODULE_AUTHOR("Jim Cromie <jim.cromie@gmail.com>");
+MODULE_LICENSE("GPL");
-- 
2.31.1


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

* Re: [PATCH v5 2/9] moduleparam: add data member to struct kernel_param
  2021-08-13 15:17 ` [PATCH v5 2/9] moduleparam: add data member to struct kernel_param Jim Cromie
@ 2021-08-13 15:43   ` Andy Shevchenko
  2021-08-13 17:14     ` jim.cromie
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2021-08-13 15:43 UTC (permalink / raw)
  To: Jim Cromie
  Cc: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Likun Gao, Chengming Gui, Aaron Liu, John Clements, Kevin Wang,
	Ashley Thomas, Qingqing Zhuo, Wyatt Wood, Aurabindo Pillai,
	Johan Hovold, Jessica Yu, Nick Desaulniers, Joe Perches,
	Miguel Ojeda, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Zhen Lei, Marco Elver,
	Jarkko Sakkinen, Alexander Potapenko, Palmer Dabbelt,
	Patricia Alfonso, Jiri Olsa, James Bottomley, Arvind Sankar,
	Johannes Berg, Arnd Bergmann, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx

On Fri, Aug 13, 2021 at 09:17:10AM -0600, Jim Cromie wrote:
> Add a const 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,
> and latter is redefined to use former.
> 
> It calls __module_param_call_wdata(), which accepts a new data param
> and inits .data with it. Re-define __module_param_call() to use it.
> 
> Use of this new data member will be rare, it might be worth redoing
> this as a separate/sub-type to de-bloat the base case.

...

> +#define module_param_cbd(name, ops, arg, perm, data)				\
> +	__module_param_call_wdata(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0, data)

Cryptic name. Moreover, inconsistent with the rest.
What about module_param_cb_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)

(above left for the above comment)

...

> +#define __module_param_call_wdata(prefix, name, ops, arg, perm, level, flags, data) \

Similar __module_param_call_with_data()

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  2021-08-13 15:17 ` [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
@ 2021-08-13 15:51   ` Andy Shevchenko
  2021-08-13 16:08     ` Matthew Wilcox
  2021-08-13 17:49     ` jim.cromie
  0 siblings, 2 replies; 16+ messages in thread
From: Andy Shevchenko @ 2021-08-13 15:51 UTC (permalink / raw)
  To: Jim Cromie
  Cc: gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Kevin Wang, Chengming Gui, Likun Gao, John Clements,
	Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai, Wyatt Wood,
	Johan Hovold, Jessica Yu, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Changbin Du, Marco Elver,
	Jarkko Sakkinen, Alexander Potapenko, Matthew Wilcox (Oracle),
	Zhen Lei, Albert van der Linde, Johannes Berg, Arvind Sankar,
	Patricia Alfonso, Arnd Bergmann, Palmer Dabbelt, Jiri Olsa,
	Andrey Konovalov, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx

On Fri, Aug 13, 2021 at 09:17:11AM -0600, Jim Cromie wrote:
> 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 3 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, All 3 are
> non-static and exported.
> 
> dynamic_debug.h:
> 
> Add DEFINE_DYNAMIC_DEBUG_CATEGORIES() described above, and a do-nothing stub.
> 
> 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.
> 
> This in the hope that someone can offer an auto-incrementing
> label-generating macro, producing "\tbit-4 0x10\t" etc, and can show
> how to apply it to __VA_ARGS__.
> 
> Also extern 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.
> 
> Adding structural query terms (func,file,lineno) could help (module is
> already done), but DEFINE_DYNAMIC_DEBUG_CATEGORIES can't do that now,
> adding it needs a better reason imo.
> 
> Dyndbg is completely agnostic wrt the categorization scheme used, to
> play well with any prefix convention already in use.  Ad-hoc
> categories and sub-categories are implicitly allowed, author
> discipline and review is expected.
> 
> Here are some examples:
> 
> "1","2","3"		2 doesnt 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:"		might be handy
> "A:".."Z:"		uhm, yeah
> 
> Hierarchical classes/categories are natural:
> 
> "drm:<CAT>:"		is used in later commit
> "drm:<CAT>:<SUB>:"	is a natural extension.
> "drm:atomic:fail:"	has been proposed, sounds directly useful
> 
> Some properties of a hierarchical category deserve explication:
> 
> Trailing spaces matter !
> 
> With 1..3-space ("drm: ", "drm:atomic: ", "drm:atomic:fail: "), the
> ":" doesnt terminate the search-space, the trailing space does.
> So a "drm:" search specification will match all DRM categories &
> subcategories, and will not be useful in an interface where all
> categories are 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("fail: ...") renders 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 modified FOO().  Doing this would enforce a policy
> decision that "debug categories will be space terminated", with an
> pressure-relief valve.
> 
> Summarizing:
> 
>  - "drm:kms: " & "drm:kms:" are different
>  - "drm:kms"		also different - includes drm:kms2:
>  - "drm:kms:\t"		also different
>  - "drm:kms:*"		doesnt 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 its 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.
> 
> There is still plenty of bikeshedding to do.

> ---
> v4+:
> 
> . rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
> . in query, 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>
> 
> ddh-helpers

> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

So, it is signed or not? I didn't get (perhaps due to misplaced changlog?).

...

>  } __attribute__((aligned(8)));
>  
>  

Do we need two blank lines here?

> +struct kernel_param;

...

> +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> +{
> +	unsigned long inbits;
> +	int rc, i, chgct = 0, totct = 0;
> +	char query[OUR_QUERY_SIZE];
> +	struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;

So you need space after ')' ?

> +	rc = kstrtoul(instr, 0, &inbits);
> +	if (rc) {
> +		pr_err("set_dyndbg: failed\n");

> +		return -EINVAL;

Why not to return rc?

> +	}
> +	vpr_info("set_dyndbg: input 0x%lx\n", inbits);
> +
> +	for (i = 0; !!bitmap[i].prefix; i++) {

Hmm... Why not simply

	for (bitmap = ...; bitmap->prefix; bitmap++) {

?

> +

Redundant blank line.

> +		sprintf(query, "format '^%s' %cp", bitmap[i].prefix,
> +			test_bit(i, &inbits) ? '+' : '-');

snprintf() ?

> +
> +		chgct = dynamic_debug_exec_queries(query, kp->mod->name);
> +
> +		v2pr_info("bit-%d: %d changes by '%s'\n", i, chgct, query);
> +		totct += chgct;
> +	}
> +	vpr_info("total changes: %d\n", totct);
> +	return 0;
> +}

...

> +	return scnprintf(buffer, PAGE_SIZE, "%u\n",
> +			 *((unsigned int *)kp->arg));

One line.

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  2021-08-13 15:51   ` Andy Shevchenko
@ 2021-08-13 16:08     ` Matthew Wilcox
  2021-08-13 17:49     ` jim.cromie
  1 sibling, 0 replies; 16+ messages in thread
From: Matthew Wilcox @ 2021-08-13 16:08 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jim Cromie, gregkh, seanpaul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Kevin Wang, Chengming Gui, Likun Gao, John Clements,
	Ashley Thomas, Qingqing Zhuo, Aurabindo Pillai, Wyatt Wood,
	Johan Hovold, Jessica Yu, Joe Perches, Miguel Ojeda,
	Nick Desaulniers, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Changbin Du, Marco Elver,
	Jarkko Sakkinen, Alexander Potapenko, Zhen Lei,
	Albert van der Linde, Johannes Berg, Arvind Sankar,
	Patricia Alfonso, Arnd Bergmann, Palmer Dabbelt, Jiri Olsa,
	Andrey Konovalov, linux-kernel, dri-devel, amd-gfx,
	intel-gvt-dev, intel-gfx

On Fri, Aug 13, 2021 at 06:51:05PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 13, 2021 at 09:17:11AM -0600, Jim Cromie wrote:
> > +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> > +{
> > +	unsigned long inbits;
> > +	int rc, i, chgct = 0, totct = 0;
> > +	char query[OUR_QUERY_SIZE];
> > +	struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
> 
> So you need space after ')' ?

More importantly, if ->data is of type 'void *', it is bad style to
cast the pointer at all.  I can't tell what type 'data' has; if it
is added to kernel_param as part of this series, I wasn't cc'd on
the patch that did that.


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

* Re: [PATCH v5 2/9] moduleparam: add data member to struct kernel_param
  2021-08-13 15:43   ` Andy Shevchenko
@ 2021-08-13 17:14     ` jim.cromie
  0 siblings, 0 replies; 16+ messages in thread
From: jim.cromie @ 2021-08-13 17:14 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Greg KH, Sean Paul, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Daniel Vetter, Alex Deucher,
	Christian König, Pan, Xinhui, Harry Wentland, Leo Li,
	Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
	Rodrigo Vivi, Jason Baron, Hawking Zhang, Tao Zhou, Huang Rui,
	Likun Gao, Chengming Gui, Aaron Liu, John Clements, Kevin Wang,
	Ashley Thomas, Qingqing Zhuo, Wyatt Wood, Aurabindo Pillai,
	Johan Hovold, Jessica Yu, Nick Desaulniers, Joe Perches,
	Miguel Ojeda, Andrew Morton, Masahiro Yamada, Peter Zijlstra,
	Paul E. McKenney, Tetsuo Handa, Thomas Gleixner,
	Vitor Massaru Iha, Sedat Dilek, Zhen Lei, Marco Elver,
	Jarkko Sakkinen, Alexander Potapenko, Palmer Dabbelt,
	Patricia Alfonso, Jiri Olsa, James Bottomley, Arvind Sankar,
	Johannes Berg, Arnd Bergmann, LKML, dri-devel,
	amd-gfx mailing list, intel-gvt-dev, Intel Graphics Development

On Fri, Aug 13, 2021 at 9:44 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 13, 2021 at 09:17:10AM -0600, Jim Cromie wrote:
> > Add a const 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,
> > and latter is redefined to use former.
> >
> > It calls __module_param_call_wdata(), which accepts a new data param
> > and inits .data with it. Re-define __module_param_call() to use it.
> >
> > Use of this new data member will be rare, it might be worth redoing
> > this as a separate/sub-type to de-bloat the base case.
>
> ...
>
> > +#define module_param_cbd(name, ops, arg, perm, data)                         \
> > +     __module_param_call_wdata(MODULE_PARAM_PREFIX, name, ops, arg, perm, -1, 0, data)
>
> Cryptic name. Moreover, inconsistent with the rest.
> What about module_param_cb_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)
>
> (above left for the above comment)
>
> ...
>
> > +#define __module_param_call_wdata(prefix, name, ops, arg, perm, level, flags, data) \
>
> Similar __module_param_call_with_data()
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

yes to all renames, revised.
thanks

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

* Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  2021-08-13 15:51   ` Andy Shevchenko
  2021-08-13 16:08     ` Matthew Wilcox
@ 2021-08-13 17:49     ` jim.cromie
  2021-08-13 19:33       ` Andy Shevchenko
  1 sibling, 1 reply; 16+ messages in thread
From: jim.cromie @ 2021-08-13 17:49 UTC (permalink / raw)
  To: Andy Shevchenko, LKML

> >
> > There is still plenty of bikeshedding to do.
>
> > ---
> > v4+:
> >
> > . rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
> > . in query, 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>
> >
> > ddh-helpers
>
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
>
> So, it is signed or not? I didn't get (perhaps due to misplaced changlog?).
>

It might be my --- snip, and reliance on format-patch -s

would it work if I used --  2char snip ?

> ...
>
> >  } __attribute__((aligned(8)));
> >
> >
>
> Do we need two blank lines here?
>
> > +struct kernel_param;
>
> ...
>
> > +int param_set_dyndbg(const char *instr, const struct kernel_param *kp)
> > +{
> > +     unsigned long inbits;
> > +     int rc, i, chgct = 0, totct = 0;
> > +     char query[OUR_QUERY_SIZE];
> > +     struct dyndbg_bitdesc *bitmap = (struct dyndbg_bitdesc *) kp->data;
>
> So you need space after ')' ?
>
> > +     rc = kstrtoul(instr, 0, &inbits);
> > +     if (rc) {
> > +             pr_err("set_dyndbg: failed\n");
>
> > +             return -EINVAL;
>
> Why not to return rc?
>
> > +     }
> > +     vpr_info("set_dyndbg: input 0x%lx\n", inbits);
> > +
> > +     for (i = 0; !!bitmap[i].prefix; i++) {
>
> Hmm... Why not simply
>
>         for (bitmap = ...; bitmap->prefix; bitmap++) {
>
> ?
>
> > +
>
> Redundant blank line.
>
> > +             sprintf(query, "format '^%s' %cp", bitmap[i].prefix,
> > +                     test_bit(i, &inbits) ? '+' : '-');
>
> snprintf() ?
>
> > +
> > +             chgct = dynamic_debug_exec_queries(query, kp->mod->name);
> > +
> > +             v2pr_info("bit-%d: %d changes by '%s'\n", i, chgct, query);
> > +             totct += chgct;
> > +     }
> > +     vpr_info("total changes: %d\n", totct);
> > +     return 0;
> > +}
>
> ...
>
> > +     return scnprintf(buffer, PAGE_SIZE, "%u\n",
> > +                      *((unsigned int *)kp->arg));
>
> One line.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

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

* Re: [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks
  2021-08-13 17:49     ` jim.cromie
@ 2021-08-13 19:33       ` Andy Shevchenko
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2021-08-13 19:33 UTC (permalink / raw)
  To: Jim Cromie; +Cc: Andy Shevchenko, LKML

On Fri, Aug 13, 2021 at 8:54 PM <jim.cromie@gmail.com> wrote:
> > > There is still plenty of bikeshedding to do.
> >
> > > ---
> > > v4+:
> > >
> > > . rename to DEFINE_DYNAMIC_DEBUG_CATEGORIES from DEFINE_DYNDBG_BITMAP
> > > . in query, 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>
> > >
> > > ddh-helpers
> >
> > > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> >
> > So, it is signed or not? I didn't get (perhaps due to misplaced changlog?).
> >
>
> It might be my --- snip, and reliance on format-patch -s
>
> would it work if I used --  2char snip ?

The right thing to do is to have

SoB ...
'--- ' line
Changelog is _here_


-- 
With Best Regards,
Andy Shevchenko

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

end of thread, other threads:[~2021-08-13 19:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 15:17 [PATCH v5 0/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and use in DRM Jim Cromie
2021-08-13 15:17 ` [PATCH v5 1/9] drm/print: fixup spelling in a comment Jim Cromie
2021-08-13 15:17 ` [PATCH v5 2/9] moduleparam: add data member to struct kernel_param Jim Cromie
2021-08-13 15:43   ` Andy Shevchenko
2021-08-13 17:14     ` jim.cromie
2021-08-13 15:17 ` [PATCH v5 3/9] dyndbg: add DEFINE_DYNAMIC_DEBUG_CATEGORIES and callbacks Jim Cromie
2021-08-13 15:51   ` Andy Shevchenko
2021-08-13 16:08     ` Matthew Wilcox
2021-08-13 17:49     ` jim.cromie
2021-08-13 19:33       ` Andy Shevchenko
2021-08-13 15:17 ` [PATCH v5 4/9] i915/gvt: remove spaces in pr_debug "gvt: core:" etc prefixes Jim Cromie
2021-08-13 15:17 ` [PATCH v5 5/9] i915/gvt: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to create "gvt:core:" etc categories Jim Cromie
2021-08-13 15:17 ` [PATCH v5 6/9] amdgpu: use DEFINE_DYNAMIC_DEBUG_CATEGORIES to control categorized pr_debugs Jim Cromie
2021-08-13 15:17 ` [PATCH v5 7/9] drm_print: add choice to use dynamic debug in drm-debug Jim Cromie
2021-08-13 15:17 ` [PATCH v5 8/9] amdgpu_ucode: reduce number of pr_debug calls Jim Cromie
2021-08-13 15:17 ` [PATCH v5 9/9] dyndbg: RFC add tracer facility RFC 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).