[PATCH v11 59/65] accel/ivpu: implement IVPU_DBG_* as a dyndbg classmap

Jim Cromie posted 65 patches 3 weeks, 3 days ago
[PATCH v11 59/65] accel/ivpu: implement IVPU_DBG_* as a dyndbg classmap
Posted by Jim Cromie 3 weeks, 3 days ago
Invoke DRM_CLASSMAP_DEFINE to create a classmap of
class-ids/categories for ivpu_dbg().

This brings static-key optimized-off benefits to the ivpu_dbg()
callsites.  DRM_CLASSMAP_PARAM_REF wires the sysfs kparam to the
classmap.

This is the 1st real-world example of a module avoiding another
module's classmap's class_id range reservation.

Its also something of an oddity: it "is" a DRM module only cuz

1. CONFIG_DRM_ACCEL_IVPU exists.

2. code therefore uses CONFIG_DRM_USE_DYNAMIC_DEBUG, so must also use
   it's dependent wrappers: DRM_CLASSMAP_*

accel/amdxdna is already using drm.debug via dev_dbg(), so it is more
fully DRM but iirc its a single call.

Anyway, to play nice with DRM, we change all the constants, from
macros calling BIT(X), to an explicit "enum ivpu_dbg_category"
starting at 16 to avoid DRM_UT_CORE..RES.  This is all in an indef to
avoid changing the constants for the non-dyndbg case.

Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/accel/ivpu/ivpu_drv.c | 27 +++++++++++++++++++--
 drivers/accel/ivpu/ivpu_drv.h | 45 ++++++++++++++++++++++++++++++-----
 2 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
index dd3a486df5f1..2d7d290eb8bb 100644
--- a/drivers/accel/ivpu/ivpu_drv.c
+++ b/drivers/accel/ivpu/ivpu_drv.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2020-2025 Intel Corporation
  */
 
+#include <linux/dynamic_debug.h>
 #include <linux/firmware.h>
 #include <linux/module.h>
 #include <linux/pci.h>
@@ -37,8 +38,30 @@
 #define DRIVER_VERSION_STR "1.0.0 " UTS_RELEASE
 #endif
 
-int ivpu_dbg_mask;
-module_param_named(dbg_mask, ivpu_dbg_mask, int, 0644);
+unsigned long ivpu_dbg_mask;
+
+#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+
+module_param_named(dbg_mask, ivpu_dbg_mask, ulong, 0644);
+#else
+DRM_CLASSMAP_DEFINE(ivpu_dbg_classes, DD_CLASS_TYPE_DISJOINT_BITS,
+		IVPU_DBG_REG,
+		"IVPU_DBG_REG",
+		"IVPU_DBG_IRQ",
+		"IVPU_DBG_MMU",
+		"IVPU_DBG_FILE",
+		"IVPU_DBG_MISC",
+		"IVPU_DBG_FW_BOOT",
+		"IVPU_DBG_PM",
+		"IVPU_DBG_IPC",
+		"IVPU_DBG_BO",
+		"IVPU_DBG_JOB",
+		"IVPU_DBG_JSM",
+		"IVPU_DBG_KREF",
+		"IVPU_DBG_RPM",
+		"IVPU_DBG_MMU_MAP");
+DRM_CLASSMAP_PARAM_REF(dbg_mask, ivpu_dbg_mask, ivpu_dbg_classes, p);
+#endif
 MODULE_PARM_DESC(dbg_mask, "Driver debug mask. See IVPU_DBG_* macros.");
 
 int ivpu_test_mode;
diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
index 6378e23e0c97..b59aa0759e26 100644
--- a/drivers/accel/ivpu/ivpu_drv.h
+++ b/drivers/accel/ivpu/ivpu_drv.h
@@ -63,6 +63,10 @@
 
 #define IVPU_SCHED_MODE_AUTO -1
 
+extern unsigned long ivpu_dbg_mask;
+
+#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
+
 #define IVPU_DBG_REG	 BIT(0)
 #define IVPU_DBG_IRQ	 BIT(1)
 #define IVPU_DBG_MMU	 BIT(2)
@@ -79,6 +83,41 @@
 #define IVPU_DBG_MMU_MAP BIT(13)
 #define IVPU_DBG_IOCTL   BIT(14)
 
+#define ivpu_dbg(vdev, type, fmt, args...) do {				\
+	if (unlikely(IVPU_DBG_##type & ivpu_dbg_mask))			\
+		dev_dbg((vdev)->drm.dev, "[%s] " fmt, #type, ##args);	\
+} while (0)
+
+#else /* !!CONFIG_DRM_USE_DYNAMIC_DEBUG */
+
+enum ivpu_dbg_category {
+	/*
+	 * since accels are drm-devices (CONFIG_DRM_ACCEL_*), adjust
+	 * IVPU_DBG_* to avoid DRMs 0..10 class_id reservations.
+	 */
+	IVPU_DBG_REG = 16,
+	IVPU_DBG_IRQ,
+	IVPU_DBG_MMU,
+	IVPU_DBG_FILE,
+	IVPU_DBG_MISC,
+	IVPU_DBG_FW_BOOT,
+	IVPU_DBG_PM,
+	IVPU_DBG_IPC,
+	IVPU_DBG_BO,
+	IVPU_DBG_JOB,
+	IVPU_DBG_JSM,
+	IVPU_DBG_KREF,
+	IVPU_DBG_RPM,
+	IVPU_DBG_MMU_MAP,
+	IVPU_DBG_IOCTL
+};
+
+#define ivpu_dbg(vdev, type, fmt, ...)                    \
+	_dynamic_func_call_cls(IVPU_DBG_##type, fmt, __dynamic_dev_dbg,	\
+			       (vdev)->drm.dev, fmt, ##__VA_ARGS__)
+
+#endif /* !!CONFIG_DRM_USE_DYNAMIC_DEBUG */
+
 #define ivpu_err(vdev, fmt, ...) \
 	drm_err(&(vdev)->drm, "%s(): " fmt, __func__, ##__VA_ARGS__)
 
@@ -93,11 +132,6 @@
 
 #define ivpu_info(vdev, fmt, ...) drm_info(&(vdev)->drm, fmt, ##__VA_ARGS__)
 
-#define ivpu_dbg(vdev, type, fmt, args...) do {                                \
-	if (unlikely(IVPU_DBG_##type & ivpu_dbg_mask))                         \
-		dev_dbg((vdev)->drm.dev, "[%s] " fmt, #type, ##args);          \
-} while (0)
-
 #define IVPU_WA(wa_name) (vdev->wa.wa_name)
 
 #define IVPU_PRINT_WA(wa_name) do {					\
@@ -208,7 +242,6 @@ struct ivpu_file_priv {
 	bool aborted;
 };
 
-extern int ivpu_dbg_mask;
 extern u8 ivpu_pll_min_ratio;
 extern u8 ivpu_pll_max_ratio;
 extern int ivpu_sched_mode;
-- 
2.53.0
Re: [PATCH v11 59/65] accel/ivpu: implement IVPU_DBG_* as a dyndbg classmap
Posted by Louis Chauvet 2 weeks, 3 days ago
On Fri, 13 Mar 2026 07:20:24 -0600, Jim Cromie <jim.cromie@gmail.com> wrote:
> [...]
> Anyway, to play nice with DRM, we change all the constants, from
> macros calling BIT(X), to an explicit "enum ivpu_dbg_category"
> starting at 16 to avoid DRM_UT_CORE..RES.  This is all in an indef to
> avoid changing the constants for the non-dyndbg case.
> 
> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>

Hello,

I have a maybe dumb question: if you enable CONFIG_DRM_USE_DYNAMIC_DEBUG,
then the meaning of ivpu_dbg_mask will change no?

In one case you will have to use ivpu_dbg_mask=0x01 and in the other case
you need ivpu_dbg_mask=0x10. I think this is very missleading.

In this case, I think it will be way easier to completly change the
expected value for ivpu_dbg_mask to have the same behavior with or without
dyndbg.

In addition, I think this could be nice to have a "rule" like: "cores"
should use LSB for their classes, "drivers" should use MSB for theirs
clases.

This way, if DRM decide to create a new class there is less chance of
conflicts.

>
>
> diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
> index 5b34b6f50e69..ef5a96b961fc 100644
> --- a/drivers/accel/ivpu/ivpu_drv.h
> +++ b/drivers/accel/ivpu/ivpu_drv.h
> @@ -81,6 +85,41 @@
> [ ... skip 9 lines ... ]
> +
> +enum ivpu_dbg_category {
> +	/*
> +	 * since accels are drm-devices (CONFIG_DRM_ACCEL_*), adjust
> +	 * IVPU_DBG_* to avoid DRMs 0..10 class_id reservations.
> +	 */

Here that could be nice to start the enum at 48, so there is a real hole
between "core" and "drivers" categories.

-- 
Louis Chauvet <louis.chauvet@bootlin.com>
Re: [PATCH v11 59/65] accel/ivpu: implement IVPU_DBG_* as a dyndbg classmap
Posted by jim.cromie@gmail.com 2 weeks, 3 days ago
On Fri, Mar 20, 2026 at 10:43 AM Louis Chauvet
<louis.chauvet@bootlin.com> wrote:
>
> On Fri, 13 Mar 2026 07:20:24 -0600, Jim Cromie <jim.cromie@gmail.com> wrote:
> > [...]
> > Anyway, to play nice with DRM, we change all the constants, from
> > macros calling BIT(X), to an explicit "enum ivpu_dbg_category"
> > starting at 16 to avoid DRM_UT_CORE..RES.  This is all in an indef to
> > avoid changing the constants for the non-dyndbg case.
> >
> > Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
>
> Hello,
>
> I have a maybe dumb question: if you enable CONFIG_DRM_USE_DYNAMIC_DEBUG,
> then the meaning of ivpu_dbg_mask will change no?
>
> In one case you will have to use ivpu_dbg_mask=0x01 and in the other case
> you need ivpu_dbg_mask=0x10. I think this is very missleading.
>
> In this case, I think it will be way easier to completly change the
> expected value for ivpu_dbg_mask to have the same behavior with or without
> dyndbg.

yes, it takes a bit more explaining certainly.

The reason I kept the old numbers for the old way was to preserve obviousness.

The /sys/modules/ivpu/parameter/<name> maps bits 0..N, not 16..N+16
I didnt want to fiddle with the conversion, and possibly get it wrong
(miss something).

> In addition, I think this could be nice to have a "rule" like: "cores"
> should use LSB for their classes, "drivers" should use MSB for theirs
> clases.
>
> This way, if DRM decide to create a new class there is less chance of
> conflicts.
>

some guidance makes sense, Im not so sure a hard rule wouldnt just over-specify.

a simple rule would be reserve-in-8-bit-chunks.
drm.debug has 10-12 bits (not sure exactly) leaving 4-6 for "growth".
So I started IVPU_DBG_* at 16 iirc.

Theres also some question if ACCEL should have their own set of debug-classes,
or should be reusing DRM_UT_*.  currently we have both,
1 or 2 accel/* drivers use DRM_UT_CORE (but no others IIRC),

ivpu had many, so I took it as a real-world example of the need
for multiple classes (not just the test-dynamic-debug* scenario).

This maybe isnt optimal.  But it might be premature to fit the strait-jacket.

> >
> >
> > diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
> > index 5b34b6f50e69..ef5a96b961fc 100644
> > --- a/drivers/accel/ivpu/ivpu_drv.h
> > +++ b/drivers/accel/ivpu/ivpu_drv.h
> > @@ -81,6 +85,41 @@
> > [ ... skip 9 lines ... ]
> > +
> > +enum ivpu_dbg_category {
> > +     /*
> > +      * since accels are drm-devices (CONFIG_DRM_ACCEL_*), adjust
> > +      * IVPU_DBG_* to avoid DRMs 0..10 class_id reservations.
> > +      */
>
> Here that could be nice to start the enum at 48, so there is a real hole
> between "core" and "drivers" categories.

Im unconvinced the distinction is that clear -
DRM_UT_CORE is used in drivers, as are KMS and ATOMIC ( iirc)
And is ACCEL classes core or drivers ? probably a bit of both ??


>
> --
> Louis Chauvet <louis.chauvet@bootlin.com>