1. Patchew
  2. linux
  3. [PATCH v7 0/9] dyndbg: drm.debug adaptation
  4. View patch

[PATCH v7 2/9] drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.

Jim Cromie posted 9 patches 3 years, 6 months ago
[PATCH v7 2/9] drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.
Posted by Jim Cromie 3 years, 6 months ago 
Use DECLARE_DYNDBG_CLASSMAP across DRM:

 - in .c files, since macro defines/initializes a record

 - in drivers, $mod_{drv,drm,param}.c
   ie where param setup is done, since a classmap is param related

 - in drm/drm_print.c
   since existing __drm_debug param is defined there,
   and we ifdef it, and provide an elaborated alternative.

 - in drm_*_helper modules:
   dp/drm_dp - 1st item in makefile target
   drivers/gpu/drm/drm_crtc_helper.c - random pick iirc.

Since these modules all use identical CLASSMAP declarations (ie: names
and .class_id's) they will all respond together to "class DRM_UT_*"
query-commands:

  :#> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control

NOTES:

This changes __drm_debug from int to ulong, so BIT() is usable on it.

DRM's enum drm_debug_category values need to sync with the index of
their respective class-names here.  Then .class_id == category, and
dyndbg's class FOO mechanisms will enable drm_dbg(DRM_UT_KMS, ...).

Though DRM needs consistent categories across all modules, thats not
generally needed; modules X and Y could define FOO differently (ie a
different NAME => class_id mapping), changes are made according to
each module's private class-map.

No callsites are actually selected by this patch, since none are
class'd yet.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 +++++++++++++
 drivers/gpu/drm/display/drm_dp_helper.c | 13 ++++++++++++
 drivers/gpu/drm/drm_crtc_helper.c       | 13 ++++++++++++
 drivers/gpu/drm/drm_print.c             | 27 +++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_params.c      | 12 +++++++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c   | 13 ++++++++++++
 include/drm/drm_print.h                 |  3 ++-
 7 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 429fcdf28836..5f091cb52de2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -38,6 +38,8 @@
 #include <linux/mmu_notifier.h>
 #include <linux/suspend.h>
 #include <linux/cc_platform.h>
+#include <linux/fb.h>
+#include <linux/dynamic_debug.h>
 
 #include "amdgpu.h"
 #include "amdgpu_irq.h"
@@ -185,6 +187,18 @@ int amdgpu_vcnfw_log;
 
 static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work);
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+			"DRM_UT_CORE",
+			"DRM_UT_DRIVER",
+			"DRM_UT_KMS",
+			"DRM_UT_PRIME",
+			"DRM_UT_ATOMIC",
+			"DRM_UT_VBL",
+			"DRM_UT_STATE",
+			"DRM_UT_LEASE",
+			"DRM_UT_DP",
+			"DRM_UT_DRMRES");
+
 struct amdgpu_mgpu_info mgpu_info = {
 	.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
 	.delayed_reset_work = __DELAYED_WORK_INITIALIZER(
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index e5bab236b3ae..196dfb1e8d87 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -30,6 +30,7 @@
 #include <linux/sched.h>
 #include <linux/seq_file.h>
 #include <linux/string_helpers.h>
+#include <linux/dynamic_debug.h>
 
 #include <drm/display/drm_dp_helper.h>
 #include <drm/display/drm_dp_mst_helper.h>
@@ -40,6 +41,18 @@
 
 #include "drm_dp_helper_internal.h"
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+			"DRM_UT_CORE",
+			"DRM_UT_DRIVER",
+			"DRM_UT_KMS",
+			"DRM_UT_PRIME",
+			"DRM_UT_ATOMIC",
+			"DRM_UT_VBL",
+			"DRM_UT_STATE",
+			"DRM_UT_LEASE",
+			"DRM_UT_DP",
+			"DRM_UT_DRMRES");
+
 struct dp_aux_backlight {
 	struct backlight_device *base;
 	struct drm_dp_aux *aux;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 8a6d54515f92..a8cee6694cf6 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -32,6 +32,7 @@
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
+#include <linux/dynamic_debug.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -51,6 +52,18 @@
 
 #include "drm_crtc_helper_internal.h"
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+			"DRM_UT_CORE",
+			"DRM_UT_DRIVER",
+			"DRM_UT_KMS",
+			"DRM_UT_PRIME",
+			"DRM_UT_ATOMIC",
+			"DRM_UT_VBL",
+			"DRM_UT_STATE",
+			"DRM_UT_LEASE",
+			"DRM_UT_DP",
+			"DRM_UT_DRMRES");
+
 /**
  * DOC: overview
  *
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index f783d4963d4b..ec32df35a3e3 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -40,7 +40,7 @@
  * __drm_debug: Enable debug output.
  * Bitmask of DRM_UT_x. See include/drm/drm_print.h for details.
  */
-unsigned int __drm_debug;
+unsigned long __drm_debug;
 EXPORT_SYMBOL(__drm_debug);
 
 MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
@@ -52,7 +52,30 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
 "\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
 "\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
 "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
-module_param_named(debug, __drm_debug, int, 0600);
+
+#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+module_param_named(debug, __drm_debug, ulong, 0600);
+#else
+/* classnames must match vals of enum drm_debug_category */
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+			"DRM_UT_CORE",
+			"DRM_UT_DRIVER",
+			"DRM_UT_KMS",
+			"DRM_UT_PRIME",
+			"DRM_UT_ATOMIC",
+			"DRM_UT_VBL",
+			"DRM_UT_STATE",
+			"DRM_UT_LEASE",
+			"DRM_UT_DP",
+			"DRM_UT_DRMRES");
+
+static struct ddebug_class_param drm_debug_bitmap = {
+	.bits = &__drm_debug,
+	.flags = "p",
+	.map = &drm_debug_classes,
+};
+module_param_cb(debug, &param_ops_dyndbg_classes, &drm_debug_bitmap, 0600);
+#endif
 
 void __drm_puts_coredump(struct drm_printer *p, const char *str)
 {
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 6fc475a5db61..d1e4d528cb17 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -29,6 +29,18 @@
 #include "i915_params.h"
 #include "i915_drv.h"
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+			"DRM_UT_CORE",
+			"DRM_UT_DRIVER",
+			"DRM_UT_KMS",
+			"DRM_UT_PRIME",
+			"DRM_UT_ATOMIC",
+			"DRM_UT_VBL",
+			"DRM_UT_STATE",
+			"DRM_UT_LEASE",
+			"DRM_UT_DP",
+			"DRM_UT_DRMRES");
+
 #define i915_param_named(name, T, perm, desc) \
 	module_param_named(name, i915_modparams.name, T, perm); \
 	MODULE_PARM_DESC(name, desc)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 561309d447e0..fd99ec0f4257 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -28,6 +28,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/vga_switcheroo.h>
 #include <linux/mmu_notifier.h>
+#include <linux/dynamic_debug.h>
 
 #include <drm/drm_aperture.h>
 #include <drm/drm_crtc_helper.h>
@@ -70,6 +71,18 @@
 #include "nouveau_svm.h"
 #include "nouveau_dmem.h"
 
+DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
+			"DRM_UT_CORE",
+			"DRM_UT_DRIVER",
+			"DRM_UT_KMS",
+			"DRM_UT_PRIME",
+			"DRM_UT_ATOMIC",
+			"DRM_UT_VBL",
+			"DRM_UT_STATE",
+			"DRM_UT_LEASE",
+			"DRM_UT_DP",
+			"DRM_UT_DRMRES");
+
 MODULE_PARM_DESC(config, "option string to pass to driver core");
 static char *nouveau_config;
 module_param_named(config, nouveau_config, charp, 0400);
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index b3b470440e46..668273e36c2c 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -35,7 +35,7 @@
 #include <drm/drm.h>
 
 /* Do *not* use outside of drm_print.[ch]! */
-extern unsigned int __drm_debug;
+extern unsigned long __drm_debug;
 
 /**
  * DOC: print
@@ -275,6 +275,7 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
  *
  */
 enum drm_debug_category {
+	/* These names must match those in DYNAMIC_DEBUG_CLASSBITS */
 	/**
 	 * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c,
 	 * drm_memory.c, ...
-- 
2.37.3
Re: [PATCH v7 2/9] drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.
Posted by Jani Nikula 3 years, 6 months ago 
On Sun, 11 Sep 2022, Jim Cromie <jim.cromie@gmail.com> wrote:
> Use DECLARE_DYNDBG_CLASSMAP across DRM:
>
>  - in .c files, since macro defines/initializes a record
>
>  - in drivers, $mod_{drv,drm,param}.c
>    ie where param setup is done, since a classmap is param related
>
>  - in drm/drm_print.c
>    since existing __drm_debug param is defined there,
>    and we ifdef it, and provide an elaborated alternative.
>
>  - in drm_*_helper modules:
>    dp/drm_dp - 1st item in makefile target
>    drivers/gpu/drm/drm_crtc_helper.c - random pick iirc.
>
> Since these modules all use identical CLASSMAP declarations (ie: names
> and .class_id's) they will all respond together to "class DRM_UT_*"
> query-commands:
>
>   :#> echo class DRM_UT_KMS +p > /proc/dynamic_debug/control
>
> NOTES:
>
> This changes __drm_debug from int to ulong, so BIT() is usable on it.
>
> DRM's enum drm_debug_category values need to sync with the index of
> their respective class-names here.  Then .class_id == category, and
> dyndbg's class FOO mechanisms will enable drm_dbg(DRM_UT_KMS, ...).
>
> Though DRM needs consistent categories across all modules, thats not
> generally needed; modules X and Y could define FOO differently (ie a
> different NAME => class_id mapping), changes are made according to
> each module's private class-map.
>
> No callsites are actually selected by this patch, since none are
> class'd yet.

The commit message could start off by saying each module needs to define
DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, ...). That is, IIUC.

Where's DECLARE_DYNDBG_CLASSMAP defined? linux-next? What's it do? What
if multiple modules with that are actually builtin?

The duplication and requirement that they're identical seems like an
error prone combo.

Finally, the choice of placement in e.g. i915_params.c seems completely
arbitrary, and makes you wonder "what here requires this, nothing?".

BR,
Jani.


>
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 14 +++++++++++++
>  drivers/gpu/drm/display/drm_dp_helper.c | 13 ++++++++++++
>  drivers/gpu/drm/drm_crtc_helper.c       | 13 ++++++++++++
>  drivers/gpu/drm/drm_print.c             | 27 +++++++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_params.c      | 12 +++++++++++
>  drivers/gpu/drm/nouveau/nouveau_drm.c   | 13 ++++++++++++
>  include/drm/drm_print.h                 |  3 ++-
>  7 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 429fcdf28836..5f091cb52de2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -38,6 +38,8 @@
>  #include <linux/mmu_notifier.h>
>  #include <linux/suspend.h>
>  #include <linux/cc_platform.h>
> +#include <linux/fb.h>
> +#include <linux/dynamic_debug.h>
>  
>  #include "amdgpu.h"
>  #include "amdgpu_irq.h"
> @@ -185,6 +187,18 @@ int amdgpu_vcnfw_log;
>  
>  static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work);
>  
> +DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
> +			"DRM_UT_CORE",
> +			"DRM_UT_DRIVER",
> +			"DRM_UT_KMS",
> +			"DRM_UT_PRIME",
> +			"DRM_UT_ATOMIC",
> +			"DRM_UT_VBL",
> +			"DRM_UT_STATE",
> +			"DRM_UT_LEASE",
> +			"DRM_UT_DP",
> +			"DRM_UT_DRMRES");
> +
>  struct amdgpu_mgpu_info mgpu_info = {
>  	.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
>  	.delayed_reset_work = __DELAYED_WORK_INITIALIZER(
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index e5bab236b3ae..196dfb1e8d87 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -30,6 +30,7 @@
>  #include <linux/sched.h>
>  #include <linux/seq_file.h>
>  #include <linux/string_helpers.h>
> +#include <linux/dynamic_debug.h>
>  
>  #include <drm/display/drm_dp_helper.h>
>  #include <drm/display/drm_dp_mst_helper.h>
> @@ -40,6 +41,18 @@
>  
>  #include "drm_dp_helper_internal.h"
>  
> +DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
> +			"DRM_UT_CORE",
> +			"DRM_UT_DRIVER",
> +			"DRM_UT_KMS",
> +			"DRM_UT_PRIME",
> +			"DRM_UT_ATOMIC",
> +			"DRM_UT_VBL",
> +			"DRM_UT_STATE",
> +			"DRM_UT_LEASE",
> +			"DRM_UT_DP",
> +			"DRM_UT_DRMRES");
> +
>  struct dp_aux_backlight {
>  	struct backlight_device *base;
>  	struct drm_dp_aux *aux;
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 8a6d54515f92..a8cee6694cf6 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -32,6 +32,7 @@
>  #include <linux/export.h>
>  #include <linux/kernel.h>
>  #include <linux/moduleparam.h>
> +#include <linux/dynamic_debug.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -51,6 +52,18 @@
>  
>  #include "drm_crtc_helper_internal.h"
>  
> +DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
> +			"DRM_UT_CORE",
> +			"DRM_UT_DRIVER",
> +			"DRM_UT_KMS",
> +			"DRM_UT_PRIME",
> +			"DRM_UT_ATOMIC",
> +			"DRM_UT_VBL",
> +			"DRM_UT_STATE",
> +			"DRM_UT_LEASE",
> +			"DRM_UT_DP",
> +			"DRM_UT_DRMRES");
> +
>  /**
>   * DOC: overview
>   *
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index f783d4963d4b..ec32df35a3e3 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -40,7 +40,7 @@
>   * __drm_debug: Enable debug output.
>   * Bitmask of DRM_UT_x. See include/drm/drm_print.h for details.
>   */
> -unsigned int __drm_debug;
> +unsigned long __drm_debug;
>  EXPORT_SYMBOL(__drm_debug);
>  
>  MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n"
> @@ -52,7 +52,30 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug cat
>  "\t\tBit 5 (0x20)  will enable VBL messages (vblank code)\n"
>  "\t\tBit 7 (0x80)  will enable LEASE messages (leasing code)\n"
>  "\t\tBit 8 (0x100) will enable DP messages (displayport code)");
> -module_param_named(debug, __drm_debug, int, 0600);
> +
> +#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
> +module_param_named(debug, __drm_debug, ulong, 0600);
> +#else
> +/* classnames must match vals of enum drm_debug_category */
> +DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
> +			"DRM_UT_CORE",
> +			"DRM_UT_DRIVER",
> +			"DRM_UT_KMS",
> +			"DRM_UT_PRIME",
> +			"DRM_UT_ATOMIC",
> +			"DRM_UT_VBL",
> +			"DRM_UT_STATE",
> +			"DRM_UT_LEASE",
> +			"DRM_UT_DP",
> +			"DRM_UT_DRMRES");
> +
> +static struct ddebug_class_param drm_debug_bitmap = {
> +	.bits = &__drm_debug,
> +	.flags = "p",
> +	.map = &drm_debug_classes,
> +};
> +module_param_cb(debug, &param_ops_dyndbg_classes, &drm_debug_bitmap, 0600);
> +#endif
>  
>  void __drm_puts_coredump(struct drm_printer *p, const char *str)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 6fc475a5db61..d1e4d528cb17 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -29,6 +29,18 @@
>  #include "i915_params.h"
>  #include "i915_drv.h"
>  
> +DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
> +			"DRM_UT_CORE",
> +			"DRM_UT_DRIVER",
> +			"DRM_UT_KMS",
> +			"DRM_UT_PRIME",
> +			"DRM_UT_ATOMIC",
> +			"DRM_UT_VBL",
> +			"DRM_UT_STATE",
> +			"DRM_UT_LEASE",
> +			"DRM_UT_DP",
> +			"DRM_UT_DRMRES");
> +
>  #define i915_param_named(name, T, perm, desc) \
>  	module_param_named(name, i915_modparams.name, T, perm); \
>  	MODULE_PARM_DESC(name, desc)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 561309d447e0..fd99ec0f4257 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -28,6 +28,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/vga_switcheroo.h>
>  #include <linux/mmu_notifier.h>
> +#include <linux/dynamic_debug.h>
>  
>  #include <drm/drm_aperture.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -70,6 +71,18 @@
>  #include "nouveau_svm.h"
>  #include "nouveau_dmem.h"
>  
> +DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
> +			"DRM_UT_CORE",
> +			"DRM_UT_DRIVER",
> +			"DRM_UT_KMS",
> +			"DRM_UT_PRIME",
> +			"DRM_UT_ATOMIC",
> +			"DRM_UT_VBL",
> +			"DRM_UT_STATE",
> +			"DRM_UT_LEASE",
> +			"DRM_UT_DP",
> +			"DRM_UT_DRMRES");
> +
>  MODULE_PARM_DESC(config, "option string to pass to driver core");
>  static char *nouveau_config;
>  module_param_named(config, nouveau_config, charp, 0400);
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> index b3b470440e46..668273e36c2c 100644
> --- a/include/drm/drm_print.h
> +++ b/include/drm/drm_print.h
> @@ -35,7 +35,7 @@
>  #include <drm/drm.h>
>  
>  /* Do *not* use outside of drm_print.[ch]! */
> -extern unsigned int __drm_debug;
> +extern unsigned long __drm_debug;
>  
>  /**
>   * DOC: print
> @@ -275,6 +275,7 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
>   *
>   */
>  enum drm_debug_category {
> +	/* These names must match those in DYNAMIC_DEBUG_CLASSBITS */
>  	/**
>  	 * @DRM_UT_CORE: Used in the generic drm code: drm_ioctl.c, drm_mm.c,
>  	 * drm_memory.c, ...

-- 
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v7 2/9] drm: POC drm on dyndbg - use in core, 2 helpers, 3 drivers.
Posted by jim.cromie@gmail.com 3 years, 6 months ago 
On Mon, Sep 12, 2022 at 4:29 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Sun, 11 Sep 2022, Jim Cromie <jim.cromie@gmail.com> wrote:
> > Use DECLARE_DYNDBG_CLASSMAP across DRM:
> >
> >  - in .c files, since macro defines/initializes a record
> >
> >  - in drivers, $mod_{drv,drm,param}.c
> >    ie where param setup is done, since a classmap is param related
> >
> >  - in drm/drm_print.c
> >    since existing __drm_debug param is defined there,
> >    and we ifdef it, and provide an elaborated alternative.
> >
> >  - in drm_*_helper modules:
> >    dp/drm_dp - 1st item in makefile target
> >    drivers/gpu/drm/drm_crtc_helper.c - random pick iirc.
> >
> > Since these modules all use identical CLASSMAP declarations (ie: names
> > and .class_id's) they will all respond together to "class DRM_UT_*"
> > query-commands:


>
> The commit message could start off by saying each module needs to define
> DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, ...). That is, IIUC.
>

Yes, I see your point.
All the explanations missing here are in preceding commits,
now in GregKHs  driver-core/driver-core-next tree,
so I didnt resend them.


> Where's DECLARE_DYNDBG_CLASSMAP defined? linux-next? What's it do? What
> if multiple modules with that are actually builtin?

The commit that adds the macro is at
https://lore.kernel.org/lkml/20220904214134.408619-15-jim.cromie@gmail.com/

there are many combos of builtin, Ive done at least several:
with caveat that >98% of testing is on virtme (thanks for that tool)

- test_dynamic_debug as module, and builtin.
it has multiple macro uses, showing 1 kind of sharing

- drm as builtin, drivers as modules
that surely pulled in other drm-helpers as builtins

- all loadable modules mostly.


>
> The duplication and requirement that they're identical seems like an
> error prone combo.

I freely acknowledge(d) this is sub-optimal.
There might be a best place for a single declaration that is in-scope
across multiple modules, but I dont know the drm core/driver lifetime
well enough to just drop this into that place.

I may have complicated things by starting with a static struct holding
the classmap, that choice was driven by:

- static declaration into a section solved a problem where the class
definitions
were "registered" (term used in patchset, -v2-3?) too late to be available for
     modprobe i915 dyndbg='class DRM_UT_CORE +p'
but worked afterwards
(also true for builtins and boot-time $mod.dyndbg='class notworking +p')

Another subtlety - the "sharing" is due more to: drm_dbg(DRM_UT_*, "")
Im not sure precisely how this might matter.

I also had an "incompleteness" argument circling in my head - something like;
you cant simultaneously allow a drm-wanna-be module to declare "DRM_UT_CORE"
but disallow "DRM_UT_ILL_CONSIDERED".   I kind-of stopped there.

Theres also an issue where multiple declarations in a module must
avoid range overlap.
I had no idea how to put that into a BUILD_BUG_ON.
Its done manually, with commentary, in test-dynamic-debug.

Maybe both issues can be improved somewhat by changing the macro
to expect real ENUM symbols, (and stringify _VA_ARGS_ to init the classnames),
not the quoted "DRM_UT_*"s it gets now.  That would also obsolete the _base,
since its the value of DRM_UT_CORE (the 1st enum val).
But that still leaves the enum vals enumerated, with possibility of
omission or mixup,
which unlike a spelling error wouldnt get caught, and would be wrong.

I fiddled with the 1st part of that for a while, I lack the macro-fu,
and punted.

Im happy to try an alternative approach, particularly with elaborated
suggestions.


>
> Finally, the choice of placement in e.g. i915_params.c seems completely
> arbitrary, and makes you wonder "what here requires this, nothing?".

acknowledged - I put it there because the access to it is via a parameter,
namely one that already affects it from a distance:
   /sys/module/drm/parameters/debug - ie drm.dbg

And its not even i915's parameter.

>
> BR,
> Jani.
>

thanks,

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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.