Re: [PATCH 5/7] drm/xe: Define verbose debug macros

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Sep 04, 2025 at 12:19:33AM +0200, Michal Wajdeczko wrote:
> 
> 
> On 9/4/2025 12:06 AM, Lucas De Marchi wrote:
> > On Wed, Sep 03, 2025 at 11:36:10PM +0200, Michal Wajdeczko wrote:
> >> We might want to add (and in some cases already have) some debug
> >> level messages that are too verbose and need to be guarded by the
> >> CONFIG_DRM_XE_DEBUG. To simplify that define verbose variant of
> >> our printk debug macros that will be compiled out on non-debug
> >> builds, in same way like we already did with xe_asserts() macros.
> > 
> > thinking out loud... aren't we going too overboard with this?
> > Looking at other places in the kernel we have pr_debug() and that's
> > awesome as we can simply enable it via dyndbg when we want, with the
> > granularity that matters for the issue we are looking at.
> 
> even if some dbg logs are disabled by default and not printing
> anything until explicitly enabled by dyndbg or drm.debug flags,
> they still add some code/text to the driver footprint.
> 
> and this patch just allows to add more logs for debug builds
> and strip them in production where we know no one will use them
> 
> but still, this is just a proposal

I'm honestly on the fence here... I like the end result of the log messages
with gt and tile very clear. But I don't like the macros very much and
would be totally in favor of some conversion over the pr_debug to follow
the rest of the kernel and avoid the NIH, like i915_utils...

> 
> > 
> > Lucas De Marchi
> > 
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> ---
> >> drivers/gpu/drm/xe/xe_gt_printk.h   | 6 ++++++
> >> drivers/gpu/drm/xe/xe_printk.h      | 6 ++++++
> >> drivers/gpu/drm/xe/xe_tile_printk.h | 6 ++++++
> >> 3 files changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_printk.h b/drivers/gpu/drm/xe/xe_gt_printk.h
> >> index 8dc4217ddb4e..69c4553d0f80 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_printk.h
> >> +++ b/drivers/gpu/drm/xe/xe_gt_printk.h
> >> @@ -35,6 +35,12 @@
> >> #define xe_gt_dbg(_gt, _fmt, ...) \
> >>     xe_gt_printk((_gt), dbg, _fmt, ##__VA_ARGS__)
> >>
> >> +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> >> +#define xe_gt_dbg_verbose        xe_gt_dbg
> >> +#else
> >> +#define xe_gt_dbg_verbose(_gt, ...)    typecheck(struct xe_gt *, (_gt))
> >> +#endif
> >> +
> >> #define xe_gt_WARN_type(_gt, _type, _condition, _fmt, ...) \
> >>     xe_tile_WARN##_type((_gt)->tile, _condition, _fmt, ## __VA_ARGS__)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_printk.h b/drivers/gpu/drm/xe/xe_printk.h
> >> index 691d3611b889..9ffa26c869da 100644
> >> --- a/drivers/gpu/drm/xe/xe_printk.h
> >> +++ b/drivers/gpu/drm/xe/xe_printk.h
> >> @@ -36,6 +36,12 @@
> >> #define xe_dbg(_xe, _fmt, ...) \
> >>     xe_printk((_xe), dbg, _fmt, ##__VA_ARGS__)
> >>
> >> +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> >> +#define xe_dbg_verbose            xe_dbg
> >> +#else
> >> +#define xe_dbg_verbose(_xe, ...)    typecheck(struct xe_device *, (_xe))
> >> +#endif
> >> +
> >> #define xe_WARN_type(_xe, _type, _condition, _fmt, ...) \
> >>     drm_WARN##_type(&(_xe)->drm, _condition, _fmt, ## __VA_ARGS__)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_tile_printk.h b/drivers/gpu/drm/xe/xe_tile_printk.h
> >> index ed951cdb08cb..33ebfd9722d3 100644
> >> --- a/drivers/gpu/drm/xe/xe_tile_printk.h
> >> +++ b/drivers/gpu/drm/xe/xe_tile_printk.h
> >> @@ -34,6 +34,12 @@
> >> #define xe_tile_dbg(_tile, _fmt, ...) \
> >>     xe_tile_printk((_tile), dbg, _fmt, ##__VA_ARGS__)
> >>
> >> +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> >> +#define xe_tile_dbg_verbose        xe_tile_dbg
> >> +#else
> >> +#define xe_tile_dbg_verbose(_tile, ...)    typecheck(struct xe_tile *, (_tile))
> >> +#endif
> >> +
> >> #define xe_tile_WARN_type(_tile, _type, _condition, _fmt, ...) \
> >>     xe_WARN##_type((_tile)->xe, _condition, _fmt, ## __VA_ARGS__)
> >>
> >> -- 
> >> 2.47.1
> >>
>